<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Dec 2, 2013 at 4:48 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, Dec 2, 2013 at 4:34 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Mon, Dec 2, 2013 at 4:26 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br>
>><br>
>> On Mon, Dec 2, 2013 at 4:21 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
>> ><br>
>> ><br>
>> ><br>
>> > On Mon, Dec 2, 2013 at 2:07 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> > Add a helper function getDebugInfoVersionFromModule to return the<br>
>> >> > debug<br>
>> >> > info<br>
>> >> > version number for a module.<br>
>> >> ><br>
>> >><br>
>> >> I think this should be getDebugMetadataVersionFromModule since this<br>
>> >> could be easily confused with the dwarf version that someone has asked<br>
>> >> for.<br>
>> ><br>
>> ><br>
>> > Done in r196172.<br>
>> >><br>
>> >><br>
>> >> > "Verifier/module-flags-1.ll" checks for verification errors.<br>
>> >> > It will seg fault when calling getDebugInfoVersionFromModule because<br>
>> >> > of<br>
>> >> > the<br>
>> >> > incorrect format for module flags in the testing case. We make<br>
>> >> > getModuleFlagsMetadata more robust by checking for error conditions.<br>
>> >> ><br>
>> >><br>
>> >> I'm not sure what's going on here, can you elaborate?<br>
>> ><br>
>> ><br>
>> > We have the code path and the verification path. Code path should not<br>
>> > seg<br>
>> > fault when the format of a module flag is incorrect (as in the testing<br>
>> > case<br>
>> > Verifier/module-flags-1.ll).<br>
>> > module-flags-1.ll tries to make sure that the verifier dumps the right<br>
>> > error<br>
>> > message when the format of a module flag is incorrect.<br>
>> ><br>
>> > Before this patch, the code path will seg fault because we don't perform<br>
>> > error checking in the code path as shown below.<br>
>> ><br>
>><br>
>> You seem to be hard coding certain operations leading me to believe<br>
>> there's some specific format here that you're looking for, but it's<br>
>> not obvious what it is<br>
><br>
><br>
> Can you be specific on where I am hard coding operations?<br>
<br>
</div></div>When you check for which operand is where. In other words:<br>
<div class="im"><br>
+ if (Flag->getNumOperands() >= 3 && isa<ConstantInt>(Flag->getOperand(0)) &&<br>
+ isa<MDString>(Flag->getOperand(1))) {<br>
+ // Check the operands of the MDNode before accessing the operands.<br>
+ // The verifier will actually catch these failures.<br>
+ ConstantInt *Behavior = cast<ConstantInt>(Flag->getOperand(0));<br>
+ MDString *Key = cast<MDString>(Flag->getOperand(1));<br>
+ Value *Val = Flag->getOperand(2);<br>
+ Flags.push_back(ModuleFlagEntry(ModFlagBehavior(Behavior->getZExtValue()),<br>
+ Key, Val));<br>
<br>
</div>here.<br></blockquote><div><br></div><div>As the comments stated, "Check the operands of the MDNode before accessing the operands".</div><div>We are checking that the MDNode has at least 3 operands and the first and second operands have the correct format before</div>
<div>accessing the 3rd operand in Flag->getOperand(2), cast<ConstantInt>(Flag->getOperands(0)) and cast<MDString>(Flag->getOperand(1)).</div><div><br></div><div>My understanding is that we should not seg fault when module flag has incorrect format, that is why I added the if condition to guard the accesses.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
><br>
>><br>
>> - are you saying that if you turn off the<br>
>> verifier we'll crash on certain types of bitcode files that normally<br>
>> we wouldn't be able to read anyhow since we'd fail verification?<br>
><br>
><br>
> No, it does not matter whether the verifier is on or off. The code path<br>
> should not seg fault when the input bit code has incorrect module flag.<br>
><br>
> The reason my patch exposes the seg fault is that we parse the module flag<br>
> before<br>
> the verifier and the testing case will seg fault before getting the<br>
> verification error.<br>
><br>
<br>
</div>This seems to me to be an indication that we're doing this in the<br>
wrong location. I think it might be more clean if we were to run the<br>
IR verifier and then as a separate pass to verify debug info metadata<br>
that will also strip if the version number isn't what's expected.<br>
Seems to have the right mental sequencing at least.<br></blockquote><div><br></div><div>No matter whether the verifier is on or not, we should not crash. If you agree on that, then we should guard the above accesses.</div>
<div>I don't think we should run the IR verifier before we upgrade (drop) the debug info. Similar to other IR upgrading, we upgrade first, then verify the upgraded IR passes the verifier.</div><div>The old format before upgrading should not pass the verifier.</div>
<div><br></div><div>I thought we agreed that stripping should not be part of debug info verifier. User can turn on/off verifier, but stripping should happen always.</div><div><br></div><div>Thanks,</div><div>Manman</div><div>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
-eric<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
<br>
> Manman<br>
><br>
>><br>
>><br>
>> -eric<br>
>><br>
>> > Cheers,<br>
>> > Manman<br>
>> ><br>
>> >><br>
>> >><br>
>> >> > Modified: llvm/trunk/lib/IR/Module.cpp<br>
>> >> > URL:<br>
>> >> ><br>
>> >> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Module.cpp?rev=196158&r1=196157&r2=196158&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Module.cpp?rev=196158&r1=196157&r2=196158&view=diff</a><br>
>> >> ><br>
>> >> ><br>
>> >> > ==============================================================================<br>
>> >> > --- llvm/trunk/lib/IR/Module.cpp (original)<br>
>> >> > +++ llvm/trunk/lib/IR/Module.cpp Mon Dec 2 15:29:56 2013<br>
>> >> > @@ -318,11 +318,16 @@ getModuleFlagsMetadata(SmallVectorImpl<M<br>
>> >> ><br>
>> >> > for (unsigned i = 0, e = ModFlags->getNumOperands(); i != e; ++i)<br>
>> >> > {<br>
>> >> > MDNode *Flag = ModFlags->getOperand(i);<br>
>> >> > - ConstantInt *Behavior = cast<ConstantInt>(Flag->getOperand(0));<br>
>> >> > - MDString *Key = cast<MDString>(Flag->getOperand(1));<br>
>> >> > - Value *Val = Flag->getOperand(2);<br>
>> >> > -<br>
>> >> ><br>
>> >> > Flags.push_back(ModuleFlagEntry(ModFlagBehavior(Behavior->getZExtValue()),<br>
>> >> > - Key, Val));<br>
>> >> > + if (Flag->getNumOperands() >= 3 &&<br>
>> >> > isa<ConstantInt>(Flag->getOperand(0)) &&<br>
>> >> > + isa<MDString>(Flag->getOperand(1))) {<br>
>> >> > + // Check the operands of the MDNode before accessing the<br>
>> >> > operands.<br>
>> >> > + // The verifier will actually catch these failures.<br>
>> >> > + ConstantInt *Behavior =<br>
>> >> > cast<ConstantInt>(Flag->getOperand(0));<br>
>> >> > + MDString *Key = cast<MDString>(Flag->getOperand(1));<br>
>> >> > + Value *Val = Flag->getOperand(2);<br>
>> >> > +<br>
>> >> ><br>
>> >> > Flags.push_back(ModuleFlagEntry(ModFlagBehavior(Behavior->getZExtValue()),<br>
>> >> > + Key, Val));<br>
>> >> > + }<br>
>> >> > }<br>
>> >> > }<br>
>> >> ><br>
>> >> ><br>
>> >> > _______________________________________________<br>
>> >> > llvm-commits mailing list<br>
>> >> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> >> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>