[PATCH] Fix for bitcode reader crash on invalid input [PR18704]

[Nick Lewycky]

Patch looks good to me. Thanks!


On 12 March 2014 00:57, Dinesh Dwivedi <dinesh.d at samsung.com> wrote:

> Gentle ping ..
>
> ------- Original Message -------
> Sender : Dinesh Dwivedi<dinesh.d at samsung.com>  Chief
> Engineer/SRI-Bangalore-Native Framework/Samsung Electronics
> Date   : Mar 04, 2014 20:09 (GMT+05:30)
> Title  : Re: Re: [PATCH] Fix for bitcode reader crash on invalid input
> [PR18704]
>
> 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.
>
> This works fine if bc file has all proper references. Current bc file has
> one instruction referring to an invalid value.
>
> <INST_LOAD op0=4294966291 op1=2 op2=0 op3=0/>
>
> 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.
>
> 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.
> I am assuming, new and other create type function always return non-null
> value. Attached fix will add null checks at required places.
>
> 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.
>
> My understanding might be wrong or I have missed some cases where null
> check is required. Please comment.
>
> Regards
> Dinesh Dwivedi
>
> ------- Original Message -------
> Sender : Nick Lewycky<nlewycky at google.com>
> Date : Mar 03, 2014 11:40 (GMT+05:30)
> Title : Re: [PATCH] Fix for bitcode reader crash on invalid input [PR18704]
>
> On 2 March 2014 21:13, Reid Kleckner <rnk at google.com> wrote:
>
> 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.
>
>
> 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).
>
> On Sun, Mar 2, 2014 at 9:08 PM, Dinesh Dwivedi <dinesh.d at samsung.com>
> wrote:
>
> ping..
>
>
>
> Regards
> Dinesh Dwivedi
>
> ------- Original Message -------
>
> Sender : Dinesh Dwivedi<dinesh.d at samsung.com>  Chief
> Engineer/SRI-Bangalore-Native Framework/Samsung Electronics
> Date   : Feb 27, 2014 15:51 (GMT+05:30)
> Title  : [PATCH] Fix for bitcode reader crash on invalid input [PR18704]
>
>
>
> Hi Reid,
>
> 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.
>
> 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".
>
> Regards
> Dinesh Dwivedi
>
>
> ------- Original Message -------
> Sender : Reid Kleckner<rnk at google.com>
> Date : Feb 27, 2014 00:16 (GMT+05:30)
> Title : Re: [PATCH] Fix for bitcode reader crash on invalid input [PR18704]
>
> 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.
>
>
>
> On Wed, Feb 26, 2014 at 7:37 AM, Dinesh Dwivedi <dinesh.d at samsung.com>
> wrote:
>
> 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.
>
> Regards
> Dinesh Dwivedi
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> <p> </p><p> </p><p> </p><p> </p>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140319/513497cb/attachment.html>