Check for invalid abbreviation operators in munged bitcode.

lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp

Index: lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp
diff --git a/lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp b/lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp
index da2907b1a9ee44af02ac208e9d5321f59451f7af..3996e136ae101e84790b23143104097ca7ba59d3 100644
--- a/lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp
+++ b/lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp
@@ -118,6 +118,11 @@ struct WriteState {
       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);
@@ -465,6 +470,18 @@ static NaClBitCodeAbbrev *deleteAbbrev(NaClBitCodeAbbrev *Abbrev) {
   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.
@@ -490,14 +507,18 @@ NaClBitCodeAbbrev *WriteState::buildAbbrev(
       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: "
@@ -506,30 +527,41 @@ NaClBitCodeAbbrev *WriteState::buildAbbrev(
       }
       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:
@@ -543,11 +575,23 @@ NaClBitCodeAbbrev *WriteState::buildAbbrev(
       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;
 }