Bulk table / memory bounds checking semantics update
Categories
(Core :: JavaScript: WebAssembly, enhancement, P3)
Tracking
()
People
(Reporter: lth, Assigned: lth)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
jseward
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jseward
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•6 years ago
|
||
Some discussion:
https://github.com/WebAssembly/bulk-memory-operations/issues/10
https://github.com/WebAssembly/bulk-memory-operations/issues/11
https://github.com/WebAssembly/design/issues/897#issuecomment-266432753 et seq
Vague-ish spec language here:
https://github.com/WebAssembly/bulk-memory-operations/blob/master/proposals/bulk-memory-operations/Overview.md#memoryinit-instruction
(That is "vague" because it requires the memory to be "accessed" but if the number of bytes written is zero then the memory is not "accessed" per se, yet the sense of the committee in the discussion quoted above was that there should be a bounds check that should fail in this case.)
Assignee | ||
Comment 2•6 years ago
|
||
There's a clarification on zero-length accesses, traps are required if the addresses are out of bounds, whether any bytes are read or written: https://github.com/WebAssembly/bulk-memory-operations/pull/48/files
I have checked the code and we are handling this aspect properly already.
(Still need solid details on per-access checks, which would allow partial writes.)
Assignee | ||
Comment 3•6 years ago
|
||
More data is accumulating here:
https://github.com/WebAssembly/bulk-memory-operations/issues/10
For memory.init and table.init we want something like this:
Initialization takes place bytewise from lower addresses toward higher addresses. A trap resulting from an access outside the source data segment or target memory only occurs once the first byte that is outside the source or target is reached. Bytes written before the trap stay written.
For memory.fill, we want something like this:
Filling takes place bytewise from lower addresses toward higher addresses. A trap resulting from an access outside the target memory only occurs once the first byte that is outside the target is reached. Bytes written before the trap stay written.
For memory.copy it's much more open in the context of shared memory and the mythical mprotect. I've filed this to pin down the memory.copy semantics, with pseudocode we can use now if we want for getting the bounds checking right until we have an mprotect instruction:
https://github.com/WebAssembly/bulk-memory-operations/issues/49
Assignee | ||
Comment 4•6 years ago
|
||
WIP. This implements partial writes for memFill and memInit. Needs test cases, and of course the spec is not quite baked, but I filed a PR against it to clarify this behavior, so we'll see.
Found a couple existing bugs here (not serious): we can't use straight memcpy and memset if the memory is shared, as this is UB, we have to use our safe-for-races shims. Flagged with comments, may land separately.
Assignee | ||
Comment 5•6 years ago
|
||
Code complete for memory.{copy,init,fill} but more test cases needed for copy.
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Comment 7•6 years ago
|
||
Implements element-by-element bounds checking behavior for memory.init, memory.fill, and memory.copy, and copy-up/copy-down semantics for memory.copy.
Assignee | ||
Comment 8•6 years ago
|
||
Ditto code + tests for tables.
Comment 9•6 years ago
|
||
Comment 10•6 years ago
|
||
Assignee | ||
Comment 11•6 years ago
|
||
(In reply to Julian Seward [:jseward] from comment #9)
Comment on attachment 9040325 [details] [diff] [review]
bug1502033-bulk-memory-partial-write-v3.patchReview of attachment 9040325 [details] [diff] [review]:
::: js/src/wasm/WasmInstance.cpp
@@ +446,5 @@
len = 0;
} else {
// Compute what we have space for in target and what's available in the
// source and pick the lowest value as the new len.
uint64_t srcAvail = memLen < srcByteOffset ? 0 : memLen - srcByteOffset;
Although it doesn't change the computed value (zero), I'd prefer the use of
<= here, so as to be consistent with the usual C idiom that "OOB === index >= numElements".
Cleaned up the test cases as suggested, but this I'll leave as it is. It's not a bounds check, it's Max(0,memLen-srcByteOffset) -- but since those values are unsigned it gets expressed as a conditional.
Comment 12•6 years ago
|
||
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a1c6c6703dfb
https://hg.mozilla.org/mozilla-central/rev/e3e2baccf180
https://hg.mozilla.org/mozilla-central/rev/9ddffb46398b
Updated•6 years ago
|
Updated•6 years ago
|
Description
•