Bug 83712 – Do not dereference a newly allocated JSArray if it is null

• Component: JavaScriptCore
• Status: RESOLVED INVALID

[Benjamin Poulain]

In JSArray::tryCreateUninitialized(), we dereference array: "array->tryFinishCreationUninitialized", that looks bad.

[Benjamin Poulain]

Created attachment 136735 [details]
Patch

[Oliver Hunt]

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.

[Benjamin Poulain]

(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.

[Oliver Hunt]

(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.

[Benjamin Poulain]

> 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.