Bug 181412 – WebAssembly: mask indexed accesses to Table

• Component: WebAssembly
• Status: RESOLVED FIXED

[JF Bastien]

WebAssembly Table indexed accesses are user-controlled and bounds-checked. Force allocations of Table data to be a power-of-two, and explicitly mask accesses after bounds-check branches.

[Radar WebKit Bug Importer]

<rdar://problem/36363236>

[JF Bastien]

Created attachment 330752 [details]
patch

[Saam Barati]

Comment on attachment 330752 [details]
patch

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

r=me

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1231
> +    if (!Options::disableSpectreMitigations())

I feel like we should've named this option with affirmative instead of the negative. So this could just be:
if (Options::enableSpectreMitigations())
    ...

> Source/JavaScriptCore/wasm/WasmTable.cpp:37
> +uint32_t Table::allocatedSize(uint32_t size)

let's call this allocationSize instead of allocatedSize. That's what we usually name functions that do this.

> Source/JavaScriptCore/wasm/WasmTable.cpp:101
> +        if (reallocSize > allocatedSize(m_size))

This might actually be a lot of memory we're going to dirty even if we never grow. Do you think it's worth having a different policy:
- always allocate virtually up to power of two, but decommit the extra tail?

> Source/JavaScriptCore/wasm/WasmTable.cpp:104
>              new (&container.get()[i]) std::remove_reference_t<decltype(*container.get())>();

Not this patch, but just something I'm realizing when reading this code: using decltype here makes it difficult to read this code and know what's going on without looking at another file.

[JF Bastien]

Created attachment 330797 [details]
patch

> > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1231
> > +    if (!Options::disableSpectreMitigations())
> 
> I feel like we should've named this option with affirmative instead of the
> negative. So this could just be:
> if (Options::enableSpectreMitigations())
>     ...

Flipped.


> > Source/JavaScriptCore/wasm/WasmTable.cpp:37
> > +uint32_t Table::allocatedSize(uint32_t size)
> 
> let's call this allocationSize instead of allocatedSize. That's what we
> usually name functions that do this.

Actually this is really badly named and misleading! What we usually call allocationSize is what's in a create() function, that we pass to the allocator, and to me says "that's the size of the whole object in bytes". This really isn't the case here! It's the *length* yet Table code uses size (which to me means bytes) everywhere it means length (which would be element count).

I've renamed appropriately to make this way clearer.


> > Source/JavaScriptCore/wasm/WasmTable.cpp:101
> > +        if (reallocSize > allocatedSize(m_size))
> 
> This might actually be a lot of memory we're going to dirty even if we never
> grow. Do you think it's worth having a different policy:
> - always allocate virtually up to power of two, but decommit the extra tail?

Yeah, but as a follow up because I think it's bigger than this patch and needs proper allocator support.

See: https://bugs.webkit.org/show_bug.cgi?id=181425

In this case the limit is 10 million table entries, so it'll be at worst a ~3x blowup. That's a lot :-)


> > Source/JavaScriptCore/wasm/WasmTable.cpp:104
> >              new (&container.get()[i]) std::remove_reference_t<decltype(*container.get())>();
> 
> Not this patch, but just something I'm realizing when reading this code:
> using decltype here makes it difficult to read this code and know what's
> going on without looking at another file.

We had that discussion when I originally committed this code. I proposed having a helper for this, motivated by this specific line of code, but my idea wasn't popular so I dropped it and kept the code that I thought to be unreadable. So I agree with you... Should I revive that patch?

[WebKit Commit Bot]

Comment on attachment 330797 [details]
patch

Rejecting attachment 330797 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 330797, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in JSTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/5990500

[JF Bastien]

Created attachment 330798 [details]
patch

Forgot oops.

[EWS Watchlist]

Attachment 330798 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:105:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 330798 [details]
patch

Clearing flags on attachment: 330798

Committed r226615: <https://trac.webkit.org/changeset/226615>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

(In reply to JF Bastien from comment #4)
> Created attachment 330797 [details]
> patch
> 
> > > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1231
> > > +    if (!Options::disableSpectreMitigations())
> > 
> > I feel like we should've named this option with affirmative instead of the
> > negative. So this could just be:
> > if (Options::enableSpectreMitigations())
> >     ...
> 
> Flipped.
Nice this is much cleaner.

> 
> 
> > > Source/JavaScriptCore/wasm/WasmTable.cpp:37
> > > +uint32_t Table::allocatedSize(uint32_t size)
> > 
> > let's call this allocationSize instead of allocatedSize. That's what we
> > usually name functions that do this.
> 
> Actually this is really badly named and misleading! What we usually call
> allocationSize is what's in a create() function, that we pass to the
> allocator, and to me says "that's the size of the whole object in bytes".
> This really isn't the case here! It's the *length* yet Table code uses size
> (which to me means bytes) everywhere it means length (which would be element
> count).
> 
> I've renamed appropriately to make this way clearer.
👍🏽
> 
> 
> > > Source/JavaScriptCore/wasm/WasmTable.cpp:101
> > > +        if (reallocSize > allocatedSize(m_size))
> > 
> > This might actually be a lot of memory we're going to dirty even if we never
> > grow. Do you think it's worth having a different policy:
> > - always allocate virtually up to power of two, but decommit the extra tail?
> 
> Yeah, but as a follow up because I think it's bigger than this patch and
> needs proper allocator support.
> 
> See: https://bugs.webkit.org/show_bug.cgi?id=181425
> 
> In this case the limit is 10 million table entries, so it'll be at worst a
> ~3x blowup. That's a lot :-)
> 
> 
> > > Source/JavaScriptCore/wasm/WasmTable.cpp:104
> > >              new (&container.get()[i]) std::remove_reference_t<decltype(*container.get())>();
> > 
> > Not this patch, but just something I'm realizing when reading this code:
> > using decltype here makes it difficult to read this code and know what's
> > going on without looking at another file.
> 
> We had that discussion when I originally committed this code. I proposed
> having a helper for this, motivated by this specific line of code, but my
> idea wasn't popular so I dropped it and kept the code that I thought to be
> unreadable. So I agree with you... Should I revive that patch?
Nah, not worth it. What’s the actual type resolve to here? Why can’t we write the type directly?

[Saam Barati]

Comment on attachment 330798 [details]
patch

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

> Source/JavaScriptCore/wasm/WasmTable.cpp:39
> +    return WTF::roundUpToPowerOfTwo(length);

Do we need to worry about overflow here?

[JF Bastien]

> > > > Source/JavaScriptCore/wasm/WasmTable.cpp:104
> > > >              new (&container.get()[i]) std::remove_reference_t<decltype(*container.get())>();
> > > 
> > > Not this patch, but just something I'm realizing when reading this code:
> > > using decltype here makes it difficult to read this code and know what's
> > > going on without looking at another file.
> > 
> > We had that discussion when I originally committed this code. I proposed
> > having a helper for this, motivated by this specific line of code, but my
> > idea wasn't popular so I dropped it and kept the code that I thought to be
> > unreadable. So I agree with you... Should I revive that patch?
> Nah, not worth it. What’s the actual type resolve to here? Why can’t we
> write the type directly?

The lambda is used to grow two containers of different type. One resolves to CallableFunction, the other to Instance*. auto lambda parameters are more elegant templates... except when decltype comes into play ;-)


> > Source/JavaScriptCore/wasm/WasmTable.cpp:39
> > +    return WTF::roundUpToPowerOfTwo(length);
> 
> Do we need to worry about overflow here?

Shouldn't be possible. The maximum Table length is 10 million. It's also encoded as an leb128 limited to 32-bit in the binary format.