[Bitcode reader] Fix a few assertions when reading invalid files

[filcab]

Summary

When creating {insert,extract}value instructions from a BitcodeReader, we
weren't verifying the fields were valid.

Bugs found with afl-fuzz

[silvas]

are you ever planning on switching over to a more "diag" style interface like Rafael suggested? That would allow making these errors much more informative.

[filcab]

For now I'm still making sure we don't assert on stuff.
We can eventually add the Diag-style error handling, but that'll end up
“just” transforming these fixes I'm doing, it won't change them, so I'm
continuing to go through my set of inputs that crash the reader.

These ones also end up using the error reporting that was already there, so
it's not like they're report_fatal_error() calls. They actually tell the
caller that there was an error and it can do something (but not that much)
about it.

I might end up starting doing the Diag-style patch soon, but it depends on
the other work I have to do. In the meantime I'll keep going over the crash
fixes, since most of those are small-ish patches. (Unless there are
objections to me doing it now)

Filipe

[• rafael]

I am all for having a separate patch just replacing the fatal errors with diag + error_code/ErrorOr. That way we get an independent test change showing the improvement.

lib/Bitcode/Reader/BitcodeReader.cpp

while (1) {
     case bitc::FUNC_CODE_INST_EXTRACTVAL: {
                                        // EXTRACTVAL: [opty, opval, n x indices]
       unsigned OpNum = 0;
       Value *Agg;
       if (getValueTypePair(Record, OpNum, NextValueNo, Agg))
         return Error("Invalid record");
 
       SmallVector<unsigned, 4> EXTRACTVALIdx;
+      Type *CurTy = Agg->getType();
       for (unsigned RecSize = Record.size();
            OpNum != RecSize; ++OpNum) {
+        bool IsArray = CurTy->isArrayTy();
+        bool IsStruct = CurTy->isStructTy();
         uint64_t Index = Record[OpNum];
+
+        if (!IsStruct && !IsArray)
+          return Error("EXTRACTVAL: Invalid type");
         if ((unsigned)Index != Index)
           return Error("Invalid value");
+        if (IsStruct && Index >= CurTy->subtypes().size())
+          return Error("EXTRACTVAL: Invalid struct index");
+        if (IsArray && Index >= CurTy->getArrayNumElements())
+          return Error("EXTRACTVAL: Invalid array index");
         EXTRACTVALIdx.push_back((unsigned)Index);
+
+        if (IsStruct)
+          CurTy = CurTy->subtypes()[Index];
+        else
+          CurTy = CurTy->subtypes()[0];
       }
 
       I = ExtractValueInst::Create(Agg, EXTRACTVALIdx);
       InstructionList.push_back(I);
       break;
     }
 
     case bitc::FUNC_CODE_INST_INSERTVAL: {
                            // INSERTVAL: [opty, opval, opty, opval, n x indices]
       unsigned OpNum = 0;
       Value *Agg;
       if (getValueTypePair(Record, OpNum, NextValueNo, Agg))
         return Error("Invalid record");
       Value *Val;
       if (getValueTypePair(Record, OpNum, NextValueNo, Val))
         return Error("Invalid record");
 
       SmallVector<unsigned, 4> INSERTVALIdx;
+      Type *CurTy = Agg->getType();
       for (unsigned RecSize = Record.size();
            OpNum != RecSize; ++OpNum) {
+        bool IsArray = CurTy->isArrayTy();
+        bool IsStruct = CurTy->isStructTy();
         uint64_t Index = Record[OpNum];
+
+        if (!IsStruct && !IsArray)
+          return Error("INSERTVAL: Invalid type");
+        if (!CurTy->isStructTy() && !CurTy->isArrayTy())
+          return Error("Invalid type");
         if ((unsigned)Index != Index)
           return Error("Invalid value");
+        if (IsStruct && Index >= CurTy->subtypes().size())
+          return Error("INSERTVAL: Invalid struct index");
+        if (IsArray && Index >= CurTy->getArrayNumElements())
+          return Error("INSERTVAL: Invalid array index");
+
         INSERTVALIdx.push_back((unsigned)Index);
+        if (IsStruct)
+          CurTy = CurTy->subtypes()[Index];
+        else
+          CurTy = CurTy->subtypes()[0];
       }
 
       I = InsertValueInst::Create(Agg, Val, INSERTVALIdx);
       InstructionList.push_back(I);
       break;
     }
 
     case bitc::FUNC_CODE_INST_SELECT: { // SELECT: [opval, ty, opval, opval]

test/Bitcode/invalid.test

 RUN:   FileCheck --check-prefix=BAD-BITWIDTH %s
 
 INVALID-ENCODING: Invalid encoding
 BAD-ABBREV: Abbreviation starts with an Array or a Blob
 UNEXPECTED-EOF: Unexpected end of file
 BAD-ABBREV-NUMBER: Invalid abbrev number
 BAD-TYPE-TABLE-FORWARD-REF: Invalid TYPE table: Only named structs can be forward referenced
 BAD-BITWIDTH: Bitwidth for integer type out of range
+
+RUN: not llvm-dis -disable-output %p/Inputs/invalid-extractval-array-idx.bc 2>&1 | \
+RUN:   FileCheck --check-prefix=EXTRACT-ARRAY %s
+RUN: not llvm-dis -disable-output %p/Inputs/invalid-extractval-struct-idx.bc 2>&1 | \
+RUN:   FileCheck --check-prefix=EXTRACT-STRUCT %s
+RUN: not llvm-dis -disable-output %p/Inputs/invalid-extractval-too-many-idxs.bc 2>&1 | \
+RUN:   FileCheck --check-prefix=EXTRACT-IDXS %s
+RUN: not llvm-dis -disable-output %p/Inputs/invalid-insertval-array-idx.bc 2>&1 | \
+RUN:   FileCheck --check-prefix=INSERT-ARRAY %s
+RUN: not llvm-dis -disable-output %p/Inputs/invalid-insertval-struct-idx.bc 2>&1 | \
+RUN:   FileCheck --check-prefix=INSERT-STRUCT %s
+RUN: not llvm-dis -disable-output %p/Inputs/invalid-insertval-too-many-idxs.bc 2>&1 | \
+RUN:   FileCheck --check-prefix=INSERT-IDXS %s
+
+
+EXTRACT-ARRAY: EXTRACTVAL: Invalid array index
+EXTRACT-STRUCT: EXTRACTVAL: Invalid struct index
+EXTRACT-IDXS: EXTRACTVAL: Invalid type
+INSERT-ARRAY: INSERTVAL: Invalid array index
+INSERT-STRUCT: INSERTVAL: Invalid struct index
+INSERT-IDXS: INSERTVAL: Invalid type