Check for invalid abbreviation operators in munged bitcode.

lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp

 //===- NaClBitcodeMungeWriter.cpp - Write munged bitcode --------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // Implements method NaClMungedBitcode.write(), which writes out a munged
@@ skipping 100 matching lines @@
 
   // Marks that an abbreviation definition is being omitted (i.e. not
   // written) for the current block.
   void markCurrentBlockWithOmittedAbbreviations() {
     assert(!ScopeStack.empty());
     ScopeStack.back().OmittedAbbreviations = true;
     if (getCurWriteBlockID() == naclbitc::BLOCKINFO_BLOCK_ID)
       BlocksWithOmittedAbbrevs.insert(SetBID);
   }
 
+  // Returns true if abbreviation operand is legal. If not, generates

[jvoung (off chromium), 2015/05/27 20:09:37]

Maybe "logs" instead of "generates"? It's not returning a new abbrev that works
or anything as part of the recovery. The recovery is handled by the caller, and
the caller just drops the abbrev instead of building it.

[Karl, 2015/05/28 20:50:31]

Ok. Updated the comment to better reflect this.

+  // a recoverable error and returns false.
+  bool verifyAbbrevOp(NaClBitCodeAbbrevOp::Encoding Encoding, uint64_t Value,
+                      const NaClBitcodeAbbrevRecord &Record);
+
   // Converts the abbreviation record to the corresponding abbreviation.
   // Returns nullptr if unable to build abbreviation.
   NaClBitCodeAbbrev *buildAbbrev(const NaClBitcodeAbbrevRecord &Record);
 
   // Emits the given record to the bitcode file. Returns true if
   // successful.
   bool LLVM_ATTRIBUTE_UNUSED_RESULT
   emitRecord(NaClBitstreamWriter &Writer,
              const NaClBitcodeAbbrevRecord &Record);
 
@@ skipping 327 matching lines @@
     break;
   }
   return true;
 }
 
 static NaClBitCodeAbbrev *deleteAbbrev(NaClBitCodeAbbrev *Abbrev) {
   Abbrev->dropRef();
   return nullptr;
 }
 
+bool WriteState::verifyAbbrevOp(NaClBitCodeAbbrevOp::Encoding Encoding,
+                                uint64_t Value,
+                                const NaClBitcodeAbbrevRecord &Record) {
+  if (NaClBitCodeAbbrevOp::isValid(Encoding, Value))
+    return true;
+  RecoverableError() << "Invalid abbreviation "
+                     << NaClBitCodeAbbrevOp::getEncodingName(Encoding)
+                     << "(" << static_cast<int64_t>(Value)
+                     << ") in: " << Record << "\n";
+  return false;
+}
+
 NaClBitCodeAbbrev *WriteState::buildAbbrev(
     const NaClBitcodeAbbrevRecord &Record) {
   // Note: Recover by removing abbreviation.
   NaClBitCodeAbbrev *Abbrev = new NaClBitCodeAbbrev();
   size_t Index = 0;
   size_t NumValues = Record.Values.size();
   if (NumValues == 0) {
     RecoverableError() << "Empty abbreviation record not allowed: "
                        << Record << "\n";
     return deleteAbbrev(Abbrev);
   }
   size_t NumAbbreviations = Record.Values[Index++];

[jvoung (off chromium), 2015/05/27 20:09:37]

Sorry a few random cleanup suggestions as I'm re-reading this...

How about "NumAbbrevOps" instead of "NumAbbreviations" ? This is building one
abbreviation, out of several operands. It's not building multiple abbrevs.

[Karl, 2015/05/28 20:50:31]

Good point. Changing.

   if (NumAbbreviations == 0) {
     RecoverableError() << "Abbreviation must contain at least one operator: "
                        << Record << "\n";
     return deleteAbbrev(Abbrev);
   }
   for (size_t Count = 0; Count < NumAbbreviations; ++Count) {
     if (Index >= NumValues) {
       RecoverableError()
           << "Malformed abbreviation found. Expects " << NumAbbreviations
           << " operands but found " << Count << ": " << Record << "\n";
       return deleteAbbrev(Abbrev);
     }
     switch (Record.Values[Index++]) {

[jvoung (off chromium), 2015/05/27 20:09:37]

Maybe stash Record.Values[Index++] in a named variable "IsLiteral" or something
to make it clear what the switch is for (just matching 1/0/other is less clear I
think).

[Karl, 2015/05/28 20:50:31]

Done.

-    case 1:
+    case 1: {
       if (Index >= NumValues) {
         RecoverableError() << "Malformed literal abbreviation: "
                            << Record << "\n";
         return deleteAbbrev(Abbrev);
       }
-      Abbrev->Add(NaClBitCodeAbbrevOp(Record.Values[Index++]));
+      uint64_t Value = Record.Values[Index++];
+      if (!verifyAbbrevOp(NaClBitCodeAbbrevOp::Literal, Value, Record))
+        return deleteAbbrev(Abbrev);
+      Abbrev->Add(NaClBitCodeAbbrevOp(Value));
       break;
+    }
     case 0: {
       if (Index >= NumValues) {
         RecoverableError() << "Malformed abbreviation found: "
                            << Record << "\n";
         return deleteAbbrev(Abbrev);
       }
       unsigned Kind = Record.Values[Index++];
       switch (Kind) {
-      case NaClBitCodeAbbrevOp::Fixed:
+      case NaClBitCodeAbbrevOp::Fixed: {
         if (Index >= NumValues) {
           RecoverableError() << "Malformed fixed abbreviation found: "
                              << Record << "\n";
           return deleteAbbrev(Abbrev);
         }
-        Abbrev->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Fixed,
-                                        Record.Values[Index++]));
+        uint64_t Value = Record.Values[Index++];
+        if (!verifyAbbrevOp(NaClBitCodeAbbrevOp::Fixed, Value, Record))
+          return deleteAbbrev(Abbrev);
+        Abbrev->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Fixed, Value));
         break;
-      case NaClBitCodeAbbrevOp::VBR:
+      }
+      case NaClBitCodeAbbrevOp::VBR: {
         if (Index >= NumValues) {
           RecoverableError() << "Malformed vbr abbreviation found: "
                              << Record << "\n";
           return deleteAbbrev(Abbrev);
         }
-        Abbrev->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::VBR,
-                                        Record.Values[Index++]));
+        uint64_t Value = Record.Values[Index++];
+        if (!verifyAbbrevOp(NaClBitCodeAbbrevOp::VBR, Value, Record))
+          return deleteAbbrev(Abbrev);
+        Abbrev->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::VBR, Value));
         break;
+      }
       case NaClBitCodeAbbrevOp::Array:
         if (Index >= NumValues) {

[jvoung (off chromium), 2015/05/27 20:09:37]

fwiw, in the other cases that have this check against Index, the code is about
to pop another value out of Record.Values. In this case it's not about to do
that, so seems closer to what happens with Char6. After the loop there's already
a check of Index vs NumValues, so this may be redundant?

[Karl, 2015/05/28 20:50:31]

I agree. Fixing.

Also, since getting values and checking size of Record.Values is so common,
factored that out into a method.

           RecoverableError() << "Malformed array abbreviation found: "
                              << Record << "\n";
           return deleteAbbrev(Abbrev);
         }
+        if (Count + 2 != NumAbbreviations) {
+          RecoverableError() << "Array abbreviation must be second to last: "
+                             << Record << "\n";
+          return deleteAbbrev(Abbrev);
+        }
         Abbrev->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Array));
         break;
       case NaClBitCodeAbbrevOp::Char6:
         Abbrev->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Char6));
         break;
       default:
         RecoverableError() << "Unknown abbreviation kind " << Kind
                            << ": " << Record << "\n";
         return deleteAbbrev(Abbrev);
       }
       break;
     }
     default:
-      RecoverableError() << "Error: Bad abbreviation operand encoding "
+      RecoverableError() << "Bad abbreviation operand encoding "
                          << Record.Values[Index-1] << ": " << Record << "\n";
       return deleteAbbrev(Abbrev);
     }
   }
+  if (Index != NumValues) {
+    RecoverableError() << "Error: Too many values for number of operands ("
+                       << NumAbbreviations << "): "
+                       << Record << "\n";
+    return deleteAbbrev(Abbrev);
+  }
+  if (!Abbrev->isValid()) {
+    raw_ostream &Out = RecoverableError();
+    Abbrev->Print(Out << "Abbreviation ");
+    Out << " not valid: " << Record << "\n";
+    return deleteAbbrev(Abbrev);
+  }
   return Abbrev;
 }
 
 } // end of anonymous namespace.
 
 NaClMungedBitcode::WriteResults NaClMungedBitcode::writeMaybeRepair(
     SmallVectorImpl<char> &Buffer, bool AddHeader,
     const WriteFlags &Flags) const {
   NaClBitstreamWriter Writer(Buffer);
   WriteState State(Flags);
   if (AddHeader) {
     NaClWriteHeader(Writer, true);
   }
   for (const NaClBitcodeAbbrevRecord &Record : *this) {
     if (!State.emitRecord(Writer, Record))
       break;
   }
   bool RecoverSilently =
       State.Results.NumErrors > 0 && !Flags.getTryToRecover();
   return State.finish(Writer, RecoverSilently);
 }