Test case for bug 20485

[david2050]

Summary

Test case for Bug 20485 (fixed in commit 293d3de)

[• rafael]

lib/Bitcode/Reader/BitstreamReader.cpp

252

Please include a diagnostic.

302–303

clang-format the patch please.

test/Bitcode/bad-abbrev-record.txt.stderr

1 ↗

(On Diff #18240)

Why do you need this to be in another file?

test/ExecutionEngine/MCJIT/eh-lg-pic.ll

2 ↗

(On Diff #18240)

Unrelated?

tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp

415

Ah, so you have a diagnostic here. Please move it down to where the problem is first detected. That should allow you to print a better message.

[david2050]

tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp

415

I chose to put the diagnostic here because that was more consistent with prior use of the API and in particular there are other clients in llvm/tools/clang (with a separate revision) that have different diagnostic needs.

[david2050]

Eliminated some inadvertent files modified on the branch.
Cleaned up the test case as suggested.
Addressed code format.

[majnemer]

lib/Bitcode/Reader/BitstreamReader.cpp

251

Please insert a space between the 'if' and the parenthesis.

tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp

414

Here as well.

415

Perhaps a richer error message? Something like: "malformed bitcode file: bad abbreviation record"

[• rafael]

I chose to put the diagnostic here because that was more consistent with prior use of the API and in particular there are other clients in llvm/tools/clang (with a separate revision) that have different diagnostic needs.

The code was just changed to even have support for diagnostics. Please
move it to where it is found. With that you can give an error like

"Value XX is not a valid encoding"

Cheers,
Rafael

[• rafael]

Ah, and about the formatting. You can just run "git-clang-format
master" in your branch to have it format the patch for you.

[filcab]

This looks good, minus the formatting in some places. Rafael's hint should help :-)
Please also change the report_fatal_error back into llvm_unreachable in include/llvm/Bitcode/BitCodes.h:128, since we're not supposed to get to that point anymore (You'll also have to remove the invalid.test Bitcode test, and test/Bitcode/Inputs/invalid-pr20485.bc)

Thanks for improving this!

include/llvm/Bitcode/BitCodes.h

99

Please run clang-format here too.

test/Bitcode/bad-abbrev-record.txt

2

Shouldn't it be "malformed bitcode file"?

[filcab]

Ping. David: Are you still working on this revision?

[david2050]

The existing trunk already has a smaller change to fix the bug so I intend
to abandon this revision.
I will propose a test case though and then close the bug.
david

[david2050]

narrow revision to test case

[filcab]

Hi David,

Can you do what I've been doing and add the run lines to
test/Bitcode/invalid.test, and the invalid testcase with a name matching
invalid-*.bc to test/Bitcode/Inputs/?
That way, the invalid test cases are all in the same place.

Thanks,

Filipe

include/llvm/Bitcode/BitCodes.h

 public:
   enum Encoding {
     Fixed = 1,  // A fixed width field, Val specifies number of bits.
     VBR   = 2,  // A VBR field where Val specifies the width of each chunk.
     Array = 3,  // A sequence of fields, next field species elt encoding.
     Char6 = 4,  // A 6-bit fixed field which maps to [a-zA-Z0-9._].
     Blob  = 5   // 32-bit aligned array of 8-bit characters.
   };
+  static bool isValidEncoding(Encoding Enc) { return  0 < (int) Enc && (int)Enc <= (int)Blob; }
 
   explicit BitCodeAbbrevOp(uint64_t V) :  Val(V), IsLiteral(true) {}
   explicit BitCodeAbbrevOp(Encoding E, uint64_t Data = 0)
     : Val(Data), IsLiteral(false), Enc(E) {}
 
   bool isLiteral() const  { return IsLiteral; }
   bool isEncoding() const { return !IsLiteral; }
 

include/llvm/Bitcode/BitstreamReader.h

public:
   void skipRecord(unsigned AbbrevID);
 
   unsigned readRecord(unsigned AbbrevID, SmallVectorImpl<uint64_t> &Vals,
                       StringRef *Blob = nullptr);
 
   //===--------------------------------------------------------------------===//
   // Abbrev Processing
   //===--------------------------------------------------------------------===//
-  void ReadAbbrevRecord();
+  bool ReadAbbrevRecord();
 
   bool ReadBlockInfoBlock();
 };
 
 } // End llvm namespace
 
 #endif

lib/Bitcode/Reader/BitstreamReader.cpp

for (unsigned i = 1, e = Abbv->getNumOperandInfos(); i != e; ++i) {
     // Skip over tail padding.
     JumpToBit(NewEnd);
   }
 
   return Code;
 }
 
 
-void BitstreamCursor::ReadAbbrevRecord() {
+bool BitstreamCursor::ReadAbbrevRecord() {
   BitCodeAbbrev *Abbv = new BitCodeAbbrev();
   unsigned NumOpInfo = ReadVBR(5);
   for (unsigned i = 0; i != NumOpInfo; ++i) {
     bool IsLiteral = Read(1) ? true : false;
     if (IsLiteral) {
       Abbv->Add(BitCodeAbbrevOp(ReadVBR64(8)));
       continue;
     }
 
     BitCodeAbbrevOp::Encoding E = (BitCodeAbbrevOp::Encoding)Read(3);
+    if(!BitCodeAbbrevOp::isValidEncoding(E))
+      return true;
+
     if (BitCodeAbbrevOp::hasEncodingData(E)) {
       unsigned Data = ReadVBR64(5);
 
       // As a special case, handle fixed(0) (i.e., a fixed field with zero bits)
       // and vbr(0) as a literal zero.  This is decoded the same way, and avoids
       // a slow path in Read() to have to handle reading zero bits.
       if ((E == BitCodeAbbrevOp::Fixed || E == BitCodeAbbrevOp::VBR) &&
           Data == 0) {
         Abbv->Add(BitCodeAbbrevOp(0));
         continue;
       }
 
       Abbv->Add(BitCodeAbbrevOp(E, Data));
     } else
       Abbv->Add(BitCodeAbbrevOp(E));
   }
   CurAbbrevs.push_back(Abbv);
+  return false;
 }
 
 bool BitstreamCursor::ReadBlockInfoBlock() {
   // If this is the second stream to get to the block info block, skip it.
   if (BitStream->hasBlockInfoRecords())
     return SkipBlock();
 
   if (EnterSubBlock(bitc::BLOCKINFO_BLOCK_ID)) return true;
while (1) {
     case llvm::BitstreamEntry::Record:
       // The interesting case.
       break;
     }
 
     // Read abbrev records, associate them with CurBID.
     if (Entry.ID == bitc::DEFINE_ABBREV) {
       if (!CurBlockInfo) return true;
-      ReadAbbrevRecord();
+      if (ReadAbbrevRecord()) 
+        return true;
 
       // ReadAbbrevRecord installs the abbrev in CurAbbrevs.  Move it to the
       // appropriate BlockInfo.
       CurBlockInfo->Abbrevs.push_back(std::move(CurAbbrevs.back()));
       CurAbbrevs.pop_back();
       continue;
     }
 
switch (readRecord(Entry.ID, Record)) {
           Name += (char)Record[i];
         CurBlockInfo->RecordNames.push_back(std::make_pair((unsigned)Record[0],
                                                            Name));
         break;
       }
     }
   }
 }
-

test/Bitcode/bad-abbrev-record.txt

+; RUN: not llvm-dis < %s.bc 2>&1 | FileCheck %s
+; CHECK: llvm-dis: Malformed block
+
+http://llvm.org/bugs/show_bug.cgi?id=20485

tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp

static bool ParseBlock(BitstreamCursor &Stream, unsigned BlockID,
   while (1) {
     if (Stream.AtEndOfStream())
       return Error("Premature end of bitstream");
 
     uint64_t RecordStartBit = Stream.GetCurrentBitNo();
 
     BitstreamEntry Entry =
       Stream.advance(BitstreamCursor::AF_DontAutoprocessAbbrevs);
-    
     switch (Entry.Kind) {
     case BitstreamEntry::Error:
       return Error("malformed bitcode file");
     case BitstreamEntry::EndBlock: {
       uint64_t BlockBitEnd = Stream.GetCurrentBitNo();
       BlockStats.NumBits += BlockBitEnd-BlockBitStart;
       if (Dump) {
         outs() << Indent << "</";
         if (BlockName)
           outs() << BlockName << ">\n";
         else
           outs() << "UnknownBlock" << BlockID << ">\n";
       }
       return false;
     }
-        
     case BitstreamEntry::SubBlock: {
       uint64_t SubBlockBitStart = Stream.GetCurrentBitNo();
       if (ParseBlock(Stream, Entry.ID, IndentLevel+1, CurStreamType))
         return true;
       ++BlockStats.NumSubBlocks;
       uint64_t SubBlockBitEnd = Stream.GetCurrentBitNo();
-      
       // Don't include subblock sizes in the size of this block.
       BlockBitStart += SubBlockBitEnd-SubBlockBitStart;
       continue;
     }
     case BitstreamEntry::Record:
       // The interesting case.
       break;
     }
 
     if (Entry.ID == bitc::DEFINE_ABBREV) {
-      Stream.ReadAbbrevRecord();
+      if(Stream.ReadAbbrevRecord())
+        return Error("malformed bitcode file");
+
       ++BlockStats.NumAbbrevs;
       continue;
     }
-    
     Record.clear();
 
     ++BlockStats.NumRecords;
 
     StringRef Blob;
     unsigned Code = Stream.readRecord(Entry.ID, Record, &Blob);
 
     // Increment the # occurrences of this code.