JSArrayBufferViews of length 0 allocate 0 CopiedSpace bytes, which is invalid

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

We have 3 options here:

(1) Allow clients to allocate 0 bytes and return NULL when they do. This is less than ideal because it adds an extra null check to the fast path for CopiedSpace allocation.

(2) Allow clients to allocate 0 bytes and do no special checks (i.e. return a valid pointer to some CopiedBlock). This is the worst of the three options because clients are already not allowed to copy allocations of size 0, so they would have a valid pointer that they could do nothing with and which would eventually point to invalid memory when the CopiedBlock was thrown away without updating the pointer. All in all, not a good idea.

(3) Disallow clients from allocating 0 bytes. Enforce with a RELEASE_ASSERT in C++ code and breakpoints in JIT code. This is probably the way to go. Clients who care about 0-byte allocations must handle that case themselves, but we don't punish anybody else for the rare case that somebody decides to allocate a 0-length typed array. It also makes the allocation and copying cases consistent for CopiedSpace: no 0-byte allocations, no 0-byte copying.

[Mark Hahnenberg]

<rdar://problem/15378335>

[Mark Hahnenberg]

Created attachment 215936 [details]
Patch

[Mark Hahnenberg]

(In reply to comment #2)
> Created an attachment (id=215936) [details]
> Patch

Forgot to svn add the new test, uploading new version...

[Geoffrey Garen]

Comment on attachment 215936 [details]
Patch

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

r=me

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4712
> +    slowCases.append(m_jit.branchTest32(MacroAssembler::Zero, sizeGPR));

I think it's probably reasonably common to make a typed array and then append to it. Instead of a slow case, I think this should ultimately be a branch around the allocation code, followed by a store of 0 to JSArrayBufferView::offsetOfVector(). Can you file a follow-up bug?

[Mark Hahnenberg]

(In reply to comment #4)
> (From update of attachment 215936 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215936&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4712
> > +    slowCases.append(m_jit.branchTest32(MacroAssembler::Zero, sizeGPR));
> 
> I think it's probably reasonably common to make a typed array and then append to it. Instead of a slow case, I think this should ultimately be a branch around the allocation code, followed by a store of 0 to JSArrayBufferView::offsetOfVector(). Can you file a follow-up bug?

Is it possible to have a dynamically sized TypedArray? I thought they had a fixed size.

[Mark Hahnenberg]

Committed r158583: <http://trac.webkit.org/changeset/158583>

[Mark Hahnenberg]

Reopening because Phil has beef.

[Alexey Proskuryakov]

Marking as blocking bug 122679, because this prevents running a WebCrypto test suite.

[Alexey Proskuryakov]

Can we track the remaining issues in a separate bug? It doesn't seem right that this bug blocks WebCrypto at this point.

[Mark Hahnenberg]

(In reply to comment #9)
> Can we track the remaining issues in a separate bug? It doesn't seem right that this bug blocks WebCrypto at this point.

Filed bug 124799.