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.
<rdar://problem/36363236>
Created attachment 330752 [details] patch
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.
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?
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
Created attachment 330798 [details] patch Forgot oops.
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.
Comment on attachment 330798 [details] patch Clearing flags on attachment: 330798 Committed r226615: <https://trac.webkit.org/changeset/226615>
All reviewed patches have been landed. Closing bug.
(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?
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?
> > > > 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.