Skip to content
This repository has been archived by the owner on Nov 3, 2021. It is now read-only.

table.fill needs to bounds check before executing table.set? #105

Closed
fitzgen opened this issue Jul 2, 2020 · 3 comments · Fixed by #106
Closed

table.fill needs to bounds check before executing table.set? #105

fitzgen opened this issue Jul 2, 2020 · 3 comments · Fixed by #106

Comments

@fitzgen
Copy link
Contributor

fitzgen commented Jul 2, 2020

I want to

  1. confirm my understanding of table.fill's intended semantics, and
  2. double check my reading of table.fill's spec text.

It is my understanding that the intention is that if any part of the table region to be filled is out of bounds, then a trap must be raised and the in-bounds subset of the table's elements left unmodified. This understanding is based on this assertion that checks that we see the old table element value after a partially out-of-bounds table.fill, and the spec interpreter also does an eager bounds check. If my understanding of table.fill's intended semantics is not correct, then that assertion and bounds check in the spec interpreter need to change.

If my understanding of table.fill's intended semantics is correct, then I want to double check my reading of table.fill's spec text. The bounds checking that raises the trap only happens after n reaches 0 (step 12), which means that for a partially-in-bounds table.fill, the in-bounds subset of elements will be mutated via the nested table.sets before we get to the n = 0 base case and the trap is raised (or more likely: we do an nested table.set that is out of bounds and raises a trap). This contradicts my understanding of the intended semantics, the assertion linked above, and the spec interpreter.

memory.fill, on the other hand, does the bounds check before checking for its n = 0 base case (steps 12 and 13), and does not suffer these issues with partially-in-bounds fills. I believe that table.fill should be changed to do this bounds check before checking for its n = 0 base case as well.

@lars-t-hansen
Copy link
Contributor

I believe your understanding of table.fill's semantics is correct: WebAssembly/bulk-memory-operations#123

@binji
Copy link
Member

binji commented Jul 13, 2020

Yes, it looks like the memory.fill instruction was updated in November 2019, but table.fill never was (that part of the spec seems to be from April 2019).

@fitzgen
Copy link
Contributor Author

fitzgen commented Jul 14, 2020

Fix at #106

fitzgen added a commit to fitzgen/reference-types that referenced this issue Jul 21, 2020
Fixes WebAssembly#105

Co-authored-by: Andreas Rossberg <rossberg@mpi-sws.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants