Bug 83712 - Do not dereference a newly allocated JSArray if it is null
Summary: Do not dereference a newly allocated JSArray if it is null
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-11 13:11 PDT by Benjamin Poulain
Modified: 2012-04-11 14:05 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.14 KB, patch)
2012-04-11 13:18 PDT, Benjamin Poulain
oliver: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2012-04-11 13:11:55 PDT
In JSArray::tryCreateUninitialized(), we dereference array: "array->tryFinishCreationUninitialized", that looks bad.
Comment 1 Benjamin Poulain 2012-04-11 13:18:44 PDT
Created attachment 136735 [details]
Patch
Comment 2 Oliver Hunt 2012-04-11 13:30:40 PDT
Comment on attachment 136735 [details]
Patch

The initial allocation of the JSArray must succeed -- if it didn't we would crash in the constructor.  But also the GC guarantees allocation will work.  tryFinishCreationUninitialized on the jsarray is the bit that may fail, and if it fails that results in a bogus JSArray.  That's where the try... comes from.
Comment 3 Benjamin Poulain 2012-04-11 13:44:15 PDT
(In reply to comment #2)
> (From update of attachment 136735 [details])
> The initial allocation of the JSArray must succeed -- if it didn't we would crash in the constructor.  But also the GC guarantees allocation will work.  tryFinishCreationUninitialized on the jsarray is the bit that may fail, and if it fails that results in a bogus JSArray.  That's where the try... comes from.

That is a very good thing to know.

So would you agree, all the tests like the following are wrong after JSArray::tryCreateUninitialized()?:
if (!array)
   doSomething()

If so, I'll remove that in an other patch.
Comment 4 Oliver Hunt 2012-04-11 13:48:47 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 136735 [details] [details])
> > The initial allocation of the JSArray must succeed -- if it didn't we would crash in the constructor.  But also the GC guarantees allocation will work.  tryFinishCreationUninitialized on the jsarray is the bit that may fail, and if it fails that results in a bogus JSArray.  That's where the try... comes from.
> 
> That is a very good thing to know.
> 
> So would you agree, all the tests like the following are wrong after JSArray::tryCreateUninitialized()?:
> if (!array)
>    doSomething()
> 
> If so, I'll remove that in an other patch.

I don't understand what you're asking.

If you mean:

JSArray* array = JSArray::tryCreateUninitialised(..)
if (!array)
   doSomething();

then no, the null check is necessary.

The bit of initialisation that makes it a "try" allocation is:

tryFinishCreationUninitialized(..)

That allocates the array's backing store.  And that allocation may fail.  If that allocation fails then the total allocation from JSArray::tryCreateUninitialised is invalid.
Comment 5 Benjamin Poulain 2012-04-11 14:05:17 PDT
> I don't understand what you're asking.
> 
> If you mean:
> 
> JSArray* array = JSArray::tryCreateUninitialised(..)
> if (!array)
>    doSomething();
> 
> then no, the null check is necessary.

Thanks for looking into this. That is exactly what I was wondering about.