Bug 154346 – Callers of JSString::value() should check for exceptions thereafter.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Applying the fix in https://bugs.webkit.org/show_bug.cgi?id=154340 project wide for straightforward cases.

JSString::value() can throw an exception if the JS string is a rope and value() needs to resolve the rope but encounters an OutOfMemory error.  If value() is not able to resolve the rope, it will return a null string (in addition to throwing the exception).  If a caller does not check for exceptions after calling JSString::value(), they may eventually use the returned null string and crash the VM.

The fix is to add all the necessary exception checks, and do the appropriate handling if needed.

[Mark Lam]

Created attachment 271569 [details]
proposed patch.

[Mark Lam]

Created attachment 271571 [details]
proposed patch.

[Geoffrey Garen]

Comment on attachment 271571 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=271571&action=review

> Source/WebCore/bindings/js/JSStorageCustom.cpp:120
> -        return true;
> +        return false;

Why did you change this? If there's a reason, you need a test for it. If there isn't a reason, you shouldn't change this.

[Mark Lam]

Comment on attachment 271571 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=271571&action=review

>> Source/WebCore/bindings/js/JSStorageCustom.cpp:120
>> +        return false;
> 
> Why did you change this? If there's a reason, you need a test for it. If there isn't a reason, you shouldn't change this.

I explained this in the WebCore ChangeLog: "Changed the return value to false if we return early due to an exception.  This is because the put operation hasn't actually taken place yet."

It's not easy to write a test for this change just like it is not easy to write a test for all the other changes in this patch: "The crash that results from this issue is dependent on a race condition where an OutOfMemory error occurs precisely at the point where the JSString::value() function is called on a rope JSString."

[Mark Lam]

Comment on attachment 271571 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=271571&action=review

>>> Source/WebCore/bindings/js/JSStorageCustom.cpp:120
>>> +        return false;
>> 
>> Why did you change this? If there's a reason, you need a test for it. If there isn't a reason, you shouldn't change this.
> 
> I explained this in the WebCore ChangeLog: "Changed the return value to false if we return early due to an exception.  This is because the put operation hasn't actually taken place yet."
> 
> It's not easy to write a test for this change just like it is not easy to write a test for all the other changes in this patch: "The crash that results from this issue is dependent on a race condition where an OutOfMemory error occurs precisely at the point where the JSString::value() function is called on a rope JSString."

OK, as requested, I've looked into making a test for this.  It turns out, I was wrong and the code was correct to begin with (and should return true, not false).  That's because the boolean is not meant to communicate whether putDelegate() is successful at putting the value or not.  Instead, it is meant to communicate to its caller JSStorage::put() that the put operation should be handled by putDelegate() (even though it failed in this case due to an OutOfMemory exception), and that JSStorage::put() should not execute its superclass put to do the operation instead.  False would, therefore, be the wrong value to return here since it has been determined by this point that the put operation should be handled by the delegate.

Some interesting tidbits to note from my attempt at writing a test for triggering this exception: on OSX, my test was creating a rope nearly 2G in length.  During rope creation, the jsString() function (called by jsAdd() in Operations.h) ensures that the length of the rope does not exceeds INT_MAX i.e. around 2G.  Hence, it is not possible to create a rope of length greater than INT_MAX.  Meanwhile, StringImpl allocation is able to allocate a buffer up to near (UINT_MAX i.e. about 4G) for the resolved rope ... that is if tryFastMalloc() succeeds in making the allocation.  On OSX, tryFastMalloc() is indeed able to do this allocation.  Hence, I can't actually get the test to throw an OutOfMemory exception here, except by instrumenting StringImpl::tryCreateUninitialized() to cap its allocation at 1G.  On iOS, tryFastMalloc() will fail, and we will get the OutOfMemory exception.  Barring the addition of permanent debugging instrumentation into bmalloc or StringImpl::tryCreateUninitialized() to force this allocation failure, my test is not actually very useful. 

Anyway, I'll revert this change to return true, and add a comment on its meaning so that no one else comes along later and repeat the same misunderstanding.  I'll land the patch with that.

[Mark Lam]

Landed in r196745: <http://trac.webkit.org/r196745>.