This is an archive of the discontinued LLVM Phabricator instance.

Differential D64507

[BitcodeReader] Validate OpNum, before accessing Record array.
ClosedPublic

Authored by fhahn on Jul 10 2019, 9:19 AM.

Details

Reviewers
t.p.northover
thegameg
jfb
Commits
rG8b222ecf2769: [BitcodeReader] Validate OpNum, before accessing Record array.
rL365750: [BitcodeReader] Validate OpNum, before accessing Record array.
Summary

Currently invalid bitcode files can cause a crash, when OpNum exceeds
the number of elements in Record, like in the attached bitcode file.

The test case was generated by clusterfuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=15698

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Jul 10 2019, 9:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2019, 9:19 AM
Herald added subscribers: dexonsmith, hiraditya. · View Herald Transcript
Harbormaster completed remote builds in B34703: Diff 208986.Jul 10 2019, 9:21 AM
jfb added inline comments.Jul 10 2019, 9:42 AM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
4169 ↗(On Diff #208986)

Could you provide more info than "invalid record"?

fhahn marked an inline comment as done.Jul 10 2019, 10:01 AM
fhahn added inline comments.
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
4169 ↗(On Diff #208986)

Sure, although I am not sure what level of detail would be appropriate, as the error itself does not provide any context to the user besides the message.

How about Invalid record: operand number exceeded available operands?

jfb added inline comments.Jul 10 2019, 10:06 AM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
4169 ↗(On Diff #208986)

SGTM

fhahn updated this revision to Diff 209010.Jul 10 2019, 10:19 AM

Improve error message.

Harbormaster completed remote builds in B34707: Diff 209010.Jul 10 2019, 10:21 AM
jfb accepted this revision.Jul 10 2019, 10:24 AM
This revision is now accepted and ready to land.Jul 10 2019, 10:24 AM
Closed by commit rL365750: [BitcodeReader] Validate OpNum, before accessing Record array. (authored by fhahn). · Explain WhyJul 11 2019, 2:56 AM
This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Jul 11 2019, 4:05 AM

I've reverted the patch, because the bitcode file caused an out-of-memory error on a builder.

I think there is another problem with the bitcode file, which causes large allocations. I've seen something similar in other cases generated by cluster fuzz, which set the number of variables or something in the header to a very large number, causing large up-front allocations for an invalid file. I'll take a look.

fhahn mentioned this in D64577: [BitcodeReader] Use tighter upper bound to validate forward references..Jul 11 2019, 11:22 AM
fhahn added a parent revision: D64577: [BitcodeReader] Use tighter upper bound to validate forward references..Jul 11 2019, 11:24 AM
fhahn mentioned this in rL366017: [BitcodeReader] Use tighter upper bound to validate forward references..Jul 14 2019, 5:35 AM
fhahn mentioned this in rG864474c9c72a: [BitcodeReader] Use tighter upper bound to validate forward references..

Revision Contents

PathSize
llvm/
trunk/
lib/
Bitcode/
Reader/
4 lines
test/
Bitcode/
Inputs/
5 lines

Diff 209153

llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp

Show First 20 Lines • Show All 4,159 Lines • ▼ Show 20 Lines case bitc::FUNC_CODE_INST_CMP2: { // CMP2: [opty, opval, opval, pred]
// FCmp/ICmp returning bool or vector of bool // FCmp/ICmp returning bool or vector of bool
unsigned OpNum = 0; unsigned OpNum = 0;
Value *LHS, *RHS; Value *LHS, *RHS;
if (getValueTypePair(Record, OpNum, NextValueNo, LHS) || if (getValueTypePair(Record, OpNum, NextValueNo, LHS) ||
popValue(Record, OpNum, NextValueNo, LHS->getType(), RHS)) popValue(Record, OpNum, NextValueNo, LHS->getType(), RHS))
return error("Invalid record"); return error("Invalid record");
if (OpNum >= Record.size())
return error(
"Invalid record: operand number exceeded available operands");
unsigned PredVal = Record[OpNum]; unsigned PredVal = Record[OpNum];
bool IsFP = LHS->getType()->isFPOrFPVectorTy(); bool IsFP = LHS->getType()->isFPOrFPVectorTy();
FastMathFlags FMF; FastMathFlags FMF;
if (IsFP && Record.size() > OpNum+1) if (IsFP && Record.size() > OpNum+1)
FMF = getDecodedFastMathFlags(Record[++OpNum]); FMF = getDecodedFastMathFlags(Record[++OpNum]);
if (OpNum+1 != Record.size()) if (OpNum+1 != Record.size())
return error("Invalid record"); return error("Invalid record");
▲ Show 20 Lines • Show All 2,515 Lines • Show Last 20 Lines

llvm/trunk/test/Bitcode/Inputs/invalid-fcmp-opnum.bc

  • This is a binary file.
PropertyOld ValueNew Value
svn:mime-typenullapplication/octet-stream
\ No newline at end of property

llvm/trunk/test/Bitcode/invalid.test

Show First 20 Lines • Show All 229 Lines • ▼ Show 20 Lines
RUN: FileCheck --check-prefix=NONPOINTER-STOREATOMIC %s RUN: FileCheck --check-prefix=NONPOINTER-STOREATOMIC %s
NONPOINTER-STOREATOMIC: Invalid record NONPOINTER-STOREATOMIC: Invalid record
RUN: not llvm-dis -disable-output %p/Inputs/invalid-nonpointer-atomicrmw.bc 2>&1 | \ RUN: not llvm-dis -disable-output %p/Inputs/invalid-nonpointer-atomicrmw.bc 2>&1 | \
RUN: FileCheck --check-prefix=NONPOINTER-ATOMICRMW %s RUN: FileCheck --check-prefix=NONPOINTER-ATOMICRMW %s
NONPOINTER-ATOMICRMW: Invalid record NONPOINTER-ATOMICRMW: Invalid record
RUN: not llvm-dis -disable-output %p/Inputs/invalid-fcmp-opnum.bc 2>&1 | \
RUN: FileCheck --check-prefix=INVALID-FCMP-OPNUM %s
INVALID-FCMP-OPNUM: Invalid record: operand number exceeded available operands