<div dir="ltr">Yeah, if we're going to get into the business of making bitcode robust to arbitrary errors, we really should talk about a testing strategy for it. I imagine that'll probably look like a fuzzer (as mentioned in review) and we'll just pull out cases from the fuzzer to check in as test cases & keep as part of the fuzzer's seed library (whatever the right word is for that).</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 17, 2016 at 1:12 PM, Justin Bogner via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: bogner<br>
Date: Thu Mar 17 15:12:06 2016<br>
New Revision: 263742<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=263742&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=263742&view=rev</a><br>
Log:<br>
Bitcode: Error out instead of crashing on corrupt metadata<br>
<br>
I hit a crash in the bitcode reader on some corrupt input where an<br>
MDString had somehow been attached to an instruction instead of an<br>
MDNode. This input is pretty bogus, but we shouldn't be crashing on bad<br>
input here.<br>
<br>
This change adds error handling in all of the places where we<br>
currently have unchecked casts from Metadata to MDNode, which means<br>
we'll error out instead of crashing for that sort of input.<br>
<br>
Unfortunately, I don't have tests. Hitting this requires flipping bits<br>
in the input bitcode, and committing corrupt binary files to catch<br>
these cases is a bit too opaque and unmaintainable.<br>
<br>
Modified:<br>
llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp<br>
<br>
Modified: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=263742&r1=263741&r2=263742&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=263742&r1=263741&r2=263742&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp (original)<br>
+++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp Thu Mar 17 15:12:06 2016<br>
@@ -126,7 +126,8 @@ public:<br>
MetadataPtrs.resize(N);<br>
}<br>
<br>
- Metadata *getValueFwdRef(unsigned Idx);<br>
+ Metadata *getMetadataFwdRef(unsigned Idx);<br>
+ MDNode *getMDNodeFwdRefOrNull(unsigned Idx);<br>
void assignValue(Metadata *MD, unsigned Idx);<br>
void tryToResolveCycles();<br>
};<br>
@@ -295,7 +296,7 @@ private:<br>
return ValueList.getValueFwdRef(ID, Ty);<br>
}<br>
Metadata *getFnMetadataByID(unsigned ID) {<br>
- return MetadataList.getValueFwdRef(ID);<br>
+ return MetadataList.getMetadataFwdRef(ID);<br>
}<br>
BasicBlock *getBasicBlock(unsigned ID) const {<br>
if (ID >= FunctionBBs.size()) return nullptr; // Invalid ID<br>
@@ -1068,7 +1069,7 @@ void BitcodeReaderMetadataList::assignVa<br>
--NumFwdRefs;<br>
}<br>
<br>
-Metadata *BitcodeReaderMetadataList::getValueFwdRef(unsigned Idx) {<br>
+Metadata *BitcodeReaderMetadataList::getMetadataFwdRef(unsigned Idx) {<br>
if (Idx >= size())<br>
resize(Idx + 1);<br>
<br>
@@ -1091,6 +1092,10 @@ Metadata *BitcodeReaderMetadataList::get<br>
return MD;<br>
}<br>
<br>
+MDNode *BitcodeReaderMetadataList::getMDNodeFwdRefOrNull(unsigned Idx) {<br>
+ return dyn_cast_or_null<MDNode>(getMetadataFwdRef(Idx));<br>
+}<br>
+<br>
void BitcodeReaderMetadataList::tryToResolveCycles() {<br>
if (!AnyFwdRefs)<br>
// Nothing to do.<br>
@@ -1907,7 +1912,7 @@ std::error_code BitcodeReader::parseMeta<br>
SmallVector<uint64_t, 64> Record;<br>
<br>
auto getMD = [&](unsigned ID) -> Metadata * {<br>
- return MetadataList.getValueFwdRef(ID);<br>
+ return MetadataList.getMetadataFwdRef(ID);<br>
};<br>
auto getMDOrNull = [&](unsigned ID) -> Metadata *{<br>
if (ID)<br>
@@ -1963,8 +1968,7 @@ std::error_code BitcodeReader::parseMeta<br>
unsigned Size = Record.size();<br>
NamedMDNode *NMD = TheModule->getOrInsertNamedMetadata(Name);<br>
for (unsigned i = 0; i != Size; ++i) {<br>
- MDNode *MD =<br>
- dyn_cast_or_null<MDNode>(MetadataList.getValueFwdRef(Record[i]));<br>
+ MDNode *MD = MetadataList.getMDNodeFwdRefOrNull(Record[i]);<br>
if (!MD)<br>
return error("Invalid record");<br>
NMD->addOperand(MD);<br>
@@ -2011,7 +2015,7 @@ std::error_code BitcodeReader::parseMeta<br>
if (!Ty)<br>
return error("Invalid record");<br>
if (Ty->isMetadataTy())<br>
- Elts.push_back(MetadataList.getValueFwdRef(Record[i + 1]));<br>
+ Elts.push_back(MetadataList.getMetadataFwdRef(Record[i + 1]));<br>
else if (!Ty->isVoidTy()) {<br>
auto *MD =<br>
ValueAsMetadata::get(ValueList.getValueFwdRef(Record[i + 1], Ty));<br>
@@ -2044,7 +2048,7 @@ std::error_code BitcodeReader::parseMeta<br>
SmallVector<Metadata *, 8> Elts;<br>
Elts.reserve(Record.size());<br>
for (unsigned ID : Record)<br>
- Elts.push_back(ID ? MetadataList.getValueFwdRef(ID - 1) : nullptr);<br>
+ Elts.push_back(ID ? MetadataList.getMetadataFwdRef(ID - 1) : nullptr);<br>
MetadataList.assignValue(IsDistinct ? MDNode::getDistinct(Context, Elts)<br>
: MDNode::get(Context, Elts),<br>
NextMetadataNo++);<br>
@@ -2056,9 +2060,11 @@ std::error_code BitcodeReader::parseMeta<br>
<br>
unsigned Line = Record[1];<br>
unsigned Column = Record[2];<br>
- MDNode *Scope = cast<MDNode>(MetadataList.getValueFwdRef(Record[3]));<br>
+ MDNode *Scope = MetadataList.getMDNodeFwdRefOrNull(Record[3]);<br>
+ if (!Scope)<br>
+ return error("Invalid record");<br>
Metadata *InlinedAt =<br>
- Record[4] ? MetadataList.getValueFwdRef(Record[4] - 1) : nullptr;<br>
+ Record[4] ? MetadataList.getMetadataFwdRef(Record[4] - 1) : nullptr;<br>
MetadataList.assignValue(<br>
GET_OR_DISTINCT(DILocation, Record[0],<br>
(Context, Line, Column, Scope, InlinedAt)),<br>
@@ -2078,8 +2084,9 @@ std::error_code BitcodeReader::parseMeta<br>
auto *Header = getMDString(Record[3]);<br>
SmallVector<Metadata *, 8> DwarfOps;<br>
for (unsigned I = 4, E = Record.size(); I != E; ++I)<br>
- DwarfOps.push_back(<br>
- Record[I] ? MetadataList.getValueFwdRef(Record[I] - 1) : nullptr);<br>
+ DwarfOps.push_back(Record[I]<br>
+ ? MetadataList.getMetadataFwdRef(Record[I] - 1)<br>
+ : nullptr);<br>
MetadataList.assignValue(<br>
GET_OR_DISTINCT(GenericDINode, Record[0],<br>
(Context, Tag, Header, DwarfOps)),<br>
@@ -3925,8 +3932,10 @@ std::error_code BitcodeReader::parseMeta<br>
auto K = MDKindMap.find(Record[I]);<br>
if (K == MDKindMap.end())<br>
return error("Invalid ID");<br>
- Metadata *MD = MetadataList.getValueFwdRef(Record[I + 1]);<br>
- F.setMetadata(K->second, cast<MDNode>(MD));<br>
+ MDNode *MD = MetadataList.getMDNodeFwdRefOrNull(Record[I + 1]);<br>
+ if (!MD)<br>
+ return error("Invalid metadata attachment");<br>
+ F.setMetadata(K->second, MD);<br>
}<br>
continue;<br>
}<br>
@@ -3939,12 +3948,15 @@ std::error_code BitcodeReader::parseMeta<br>
MDKindMap.find(Kind);<br>
if (I == MDKindMap.end())<br>
return error("Invalid ID");<br>
- Metadata *Node = MetadataList.getValueFwdRef(Record[i + 1]);<br>
+ Metadata *Node = MetadataList.getMetadataFwdRef(Record[i + 1]);<br>
if (isa<LocalAsMetadata>(Node))<br>
// Drop the attachment. This used to be legal, but there's no<br>
// upgrade path.<br>
break;<br>
- Inst->setMetadata(I->second, cast<MDNode>(Node));<br>
+ MDNode *MD = dyn_cast_or_null<MDNode>(Node);<br>
+ if (!MD)<br>
+ return error("Invalid metadata attachment");<br>
+ Inst->setMetadata(I->second, MD);<br>
if (I->second == LLVMContext::MD_tbaa)<br>
InstsWithTBAATag.push_back(Inst);<br>
}<br>
@@ -4105,10 +4117,16 @@ std::error_code BitcodeReader::parseFunc<br>
unsigned ScopeID = Record[2], IAID = Record[3];<br>
<br>
MDNode *Scope = nullptr, *IA = nullptr;<br>
- if (ScopeID)<br>
- Scope = cast<MDNode>(MetadataList.getValueFwdRef(ScopeID - 1));<br>
- if (IAID)<br>
- IA = cast<MDNode>(MetadataList.getValueFwdRef(IAID - 1));<br>
+ if (ScopeID) {<br>
+ Scope = MetadataList.getMDNodeFwdRefOrNull(ScopeID - 1);<br>
+ if (!Scope)<br>
+ return error("Invalid record");<br>
+ }<br>
+ if (IAID) {<br>
+ IA = MetadataList.getMDNodeFwdRefOrNull(IAID - 1);<br>
+ if (!IA)<br>
+ return error("Invalid record");<br>
+ }<br>
LastLoc = DebugLoc::get(Line, Col, Scope, IA);<br>
I->setDebugLoc(LastLoc);<br>
I = nullptr;<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>