Bug 216501 – Segfault in WebCore::IDBKey::createBinary

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Michael Saboff]

The script below crashes due to code IDBKey::createBinary(JSC::JSArrayBufferView&) not handling the case where we can't get a buffer view due to running out of memory.

-- test.html --
<script>
const a = [];
a.length = 2**30
a.__proto__ = {};
Object.defineProperty(a, 0, { get: foo });
function foo() {
  new Uint8Array(a);
}

try {
  foo();
} catch {}

try {
  while(1) {
    new ArrayBuffer(2**20);
  }
} catch {}

let u8 = new Uint8Array();
IDBKeyRange.only(u8);
</script>

Steps To Reproduce:

DYLD_FRAMEWORK_PATH=<WEBKIT_PATH>/WebKitBuild/Debug <WEBKIT_PATH>/WebKitBuild/Debug/WebKitTestRunner test.html

Results:

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000020
Exception Note:        EXC_CORPSE_NOTIFY


Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore                   0x00000004a14631fa WebCore::IDBKey::createBinary(JSC::JSArrayBufferView&) + 42
1   com.apple.WebCore                   0x00000004a19abe62 WebCore::createIDBKeyFromValue(JSC::JSGlobalObject&, JSC::JSValue, WTF::Vector<JSC::JSArray*, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&) + 2178
2   com.apple.WebCore                   0x00000004a197ee62 WebCore::scriptValueToIDBKey(JSC::JSGlobalObject&, JSC::JSValue) + 34
3   com.apple.WebCore                   0x00000004a147098a WebCore::IDBKeyRange::only(JSC::JSGlobalObject&, JSC::JSValue) + 122
4   com.apple.WebCore                   0x00000004a097090f WebCore::jsIDBKeyRangeConstructorFunctionOnly(JSC::JSGlobalObject*, JSC::CallFrame*) + 287
5   ???                                 0x0000251383201178 0 + 40765734523256
6   com.apple.JavaScriptCore            0x00000004a9a451b1 llint_entry + 120103

[Michael Saboff]

<rdar://problem/68498785>

[Michael Saboff]

Created attachment 408869 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 408869 [details]
Patch

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

> LayoutTests/storage/indexeddb/IDBKey-create-array-buffer-view-oom.html:44
> +shouldBeTrue("exceptionString == undefined || exceptionString === 'DataError: Provided data is inadequate.'");

If this happened, shouldn't we throw out-of-memory error instead of DataError?

[Michael Saboff]

(In reply to Yusuke Suzuki from comment #3)
> Comment on attachment 408869 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408869&action=review
> 
> > LayoutTests/storage/indexeddb/IDBKey-create-array-buffer-view-oom.html:44
> > +shouldBeTrue("exceptionString == undefined || exceptionString === 'DataError: Provided data is inadequate.'");
> 
> If this happened, shouldn't we throw out-of-memory error instead of
> DataError?

That error is existing when the key range source is invalid.  This fix uses that existing error path.  The test code catches and throws away two out-of-memory errors before getting to this point.

We could specifically throw out-of-memory, but I suspect that would be a riskier fix.

[youenn fablet]

Comment on attachment 408869 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBKey.cpp:56
> +        return adoptRef(*new IDBKey());

Can we try returning nullptr here as a RefPtr<IDBKey>?
If we are low on memory, it seems best to not allocate a new object.
Might be a bigger change though.

[Saam Barati]

Comment on attachment 408869 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBKey.cpp:56
>> +        return adoptRef(*new IDBKey());
> 
> Can we try returning nullptr here as a RefPtr<IDBKey>?
> If we are low on memory, it seems best to not allocate a new object.
> Might be a bigger change though.

I agree this seems cleaner, given this now can fail. It's nicer to document things using nullability.

[Michael Saboff]

(In reply to youenn fablet from comment #5)
> Comment on attachment 408869 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408869&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBKey.cpp:56
> > +        return adoptRef(*new IDBKey());
> 
> Can we try returning nullptr here as a RefPtr<IDBKey>?
> If we are low on memory, it seems best to not allocate a new object.
> Might be a bigger change though.

Are you suggesting we change
    Ref<IDBKey> createBinary(JSC::JSArrayBufferView&)
to
    RefPtr<IDBKey> createBinary(JSC::JSArrayBufferView&)

If so, it seems weird to have two different return types for the overloaded createBinary(...) methods.

If not, there isn't a way to convert a RefPtr<IDBKey>(nullptr) into a Ref<IDBKey>.

This code is only being called by createIDBKeyFromValue() (via createIDBKeyFromValue()) which return IDBKey::createInvalid() aka adoptRef(*new IDBKey()) for a key with the value of RefPtr<IDBKey>(nullptr).  So we aren't avoiding the allocation, only delaying it.

[youenn fablet]

Comment on attachment 408869 [details]
Patch

> Are you suggesting we change
>     Ref<IDBKey> createBinary(JSC::JSArrayBufferView&)
> to
>     RefPtr<IDBKey> createBinary(JSC::JSArrayBufferView&)
> 
> If so, it seems weird to have two different return types for the overloaded createBinary(...) methods.

We could introduce tryCreateBinary.
Or at least return IDBKey::createInvalid() which seems a bit clearer.

[Michael Saboff]

(In reply to youenn fablet from comment #8)
> Comment on attachment 408869 [details]
> Patch
> 
> > Are you suggesting we change
> >     Ref<IDBKey> createBinary(JSC::JSArrayBufferView&)
> > to
> >     RefPtr<IDBKey> createBinary(JSC::JSArrayBufferView&)
> > 
> > If so, it seems weird to have two different return types for the overloaded createBinary(...) methods.
> 
> We could introduce tryCreateBinary.
> Or at least return IDBKey::createInvalid() which seems a bit clearer.

Let's go with IDBKey::createInvalid().

[Michael Saboff]

Created attachment 409043 [details]
Updated Patch

[Michael Saboff]

Committed r267205: <https://trac.webkit.org/changeset/267205>

[Truitt Savell]

It looks like the new test
storage/indexeddb/IDBKey-create-array-buffer-view-oom.html

added in https://trac.webkit.org/changeset/267205/webkit

is a flaky timeout 
History:
https://results.webkit.org/?suite=layout-tests&test=storage%2Findexeddb%2FIDBKey-create-array-buffer-view-oom.html

[Aakash Jain]

(In reply to Truitt Savell from comment #12)
> It looks like the new test storage/indexeddb/IDBKey-create-array-buffer-view-oom.html added in https://trac.webkit.org/changeset/267205/webkit 
> is a flaky timeout 
This test has been very flaky and slowing down EWS.
e.g.:
https://ews-build.webkit.org/#/builders/24/builds/27172
https://ews-build.webkit.org/#/builders/10/builds/37806
https://ews-build.webkit.org/#/builders/10/builds/37803
https://ews-build.webkit.org/#/builders/24/builds/27145
https://ews-build.webkit.org/#/builders/24/builds/27122
https://ews-build.webkit.org/#/builders/10/builds/37769
https://ews-build.webkit.org/#/builders/24/builds/27100
https://ews-build.webkit.org/#/builders/24/builds/27086
https://ews-build.webkit.org/#/builders/10/builds/37699
https://ews-build.webkit.org/#/builders/24/builds/27069
https://ews-build.webkit.org/#/builders/24/builds/27053
https://ews-build.webkit.org/#/builders/10/builds/37671
https://ews-build.webkit.org/#/builders/24/builds/27018
https://ews-build.webkit.org/#/builders/10/builds/37644