<div dir="ltr">Patch looks good to me. Thanks!<br><div class="gmail_extra"><br><br><div class="gmail_quote">On 12 March 2014 00:57, Dinesh Dwivedi <span dir="ltr"><<a href="mailto:dinesh.d@samsung.com" target="_blank">dinesh.d@samsung.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Gentle ping ..<br>
<br>
------- Original Message -------<br>
Sender : Dinesh Dwivedi<<a href="mailto:dinesh.d@samsung.com">dinesh.d@samsung.com</a>> Chief Engineer/SRI-Bangalore-Native Framework/Samsung Electronics<br>
Date : Mar 04, 2014 20:09 (GMT+05:30)<br>
Title : Re: Re: [PATCH] Fix for bitcode reader crash on invalid input [PR18704]<br>
<br>
I have gone through the code to understand in what cases, values are reserved in advance. Whenever there is some forward reference for value/ type/ metadata, BitcodeReader resize corresponding list to index of referenced value + 1. All new elements except the last one initialized to NULL. Ideally, once parsing is complete, all elements should be non-NULL.<br>
<br>
This works fine if bc file has all proper references. Current bc file has one instruction referring to an invalid value.<br>
<br>
<INST_LOAD op0=4294966291 op1=2 op2=0 op3=0/><br>
<br>
Here after adjusting for relative ID, index for op0 comes out to be 1040. For creating forward reference, we are resizing ValueList to 1041. As there aren't this many values, many of them will remains resolved, leaving null pointers in the list.<br>
<br>
To safe-guard BitcodeReader, we need to add null check for all access to these list [lists which resize them-selves based on record values, as invalid value will cause list to grow to invalid range]. I have tried to add Null checks to all such places.<br>
I am assuming, new and other create type function always return non-null value. Attached fix will add null checks at required places.<br>
<br>
I have one doubt about using SmallVector here. LLVM programmer manual suggest use of SmallVector for small size vector (usually less than 8). As value references in program may be much more than this, should be use Vector instead.<br>
<br>
My understanding might be wrong or I have missed some cases where null check is required. Please comment.<br>
<br>
Regards<br>
Dinesh Dwivedi<br>
<br>
------- Original Message -------<br>
Sender : Nick Lewycky<<a href="mailto:nlewycky@google.com">nlewycky@google.com</a>><br>
Date : Mar 03, 2014 11:40 (GMT+05:30)<br>
Title : Re: [PATCH] Fix for bitcode reader crash on invalid input [PR18704]<br>
<br>
On 2 March 2014 21:13, Reid Kleckner <<a href="mailto:rnk@google.com">rnk@google.com</a>> wrote:<br>
<br>
Nick and I looked at this and we think there are other issues of a similar nature in the bitcode reader. There are many places where ValueList is indexed and accessed without any null check. I think Nick said the list shouldn't contain any null elements unless we're returning an error, which is what your test case does.<br>
<br>
<br>
I did say that, but I take it back. There is a place where it does reserve values in advance, and that's what happens in this testcase. The testcase then demonstrates an access without a null check in error handling code, but I don't see why the other non-error handling code shouldn't have it either (I kept looking, which is why I never replied).<br>
<br>
On Sun, Mar 2, 2014 at 9:08 PM, Dinesh Dwivedi <<a href="mailto:dinesh.d@samsung.com">dinesh.d@samsung.com</a>> wrote:<br>
<br>
ping..<br>
<br>
<br>
<br>
Regards<br>
Dinesh Dwivedi<br>
<br>
------- Original Message -------<br>
<br>
Sender : Dinesh Dwivedi<<a href="mailto:dinesh.d@samsung.com">dinesh.d@samsung.com</a>> Chief Engineer/SRI-Bangalore-Native Framework/Samsung Electronics<br>
Date : Feb 27, 2014 15:51 (GMT+05:30)<br>
Title : [PATCH] Fix for bitcode reader crash on invalid input [PR18704]<br>
<br>
<br>
<br>
Hi Reid,<br>
<br>
I have attached a separate binary patch for bc file. This is same file attached in bug report. I have added llvm-bcanalyzer as comment in test case. But I do not understand much of it.<br>
<br>
I have updated my fix a bit. I have update dyn_cast to dyn_cast_or_null for all location casting values from ValueList to something. If code was already returning some error code on null value, I left them as it is. For other cases, I have added null check and on null it will return "InvalidValue".<br>
<br>
Regards<br>
Dinesh Dwivedi<br>
<br>
<br>
------- Original Message -------<br>
Sender : Reid Kleckner<<a href="mailto:rnk@google.com">rnk@google.com</a>><br>
Date : Feb 27, 2014 00:16 (GMT+05:30)<br>
Title : Re: [PATCH] Fix for bitcode reader crash on invalid input [PR18704]<br>
<br>
The binary content of the .bc file isn't present in the diff. Can you attach it, and maybe paste the output of llvm-bcanalyzer into the test case as a comment? Also, the fix doesn't look correct. It looks like subsequent Arguments might leak, although that isn't really critical.<br>
<br>
<br>
<br>
On Wed, Feb 26, 2014 at 7:37 AM, Dinesh Dwivedi <<a href="mailto:dinesh.d@samsung.com">dinesh.d@samsung.com</a>> wrote:<br>
<br>
Attaching patch for PR18704. I have just gone through back-trace for the assert [dyn_cast was getting applied to NULL value], locate code causing this and added check to print error for NULL values.<br>
<br>
Regards<br>
Dinesh Dwivedi<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><p> </p><p> </p><p> </p><p> </p></blockquote>
</div><br></div></div>