memory64: when enabled, check offset range at validation-time #2253
Conversation
921c767 to
324e800
Compare
| Opcode, | ||
| Var memidx, | ||
| Address align, | ||
| Address offset); |
There was a problem hiding this comment.
I wonder if would make sense to bundle memidx, align, offset into some kind of MemLocation struct? Are there other places in the code that want to reason about these three as a unit?
There was a problem hiding this comment.
I think it would make a lot of sense, but it would be a pretty intensive refactor.
There's a lot of callsites in the BinaryReader (and its delegates, e.g. BinaryReaderIR/Interp/etc.) that pass these three in the form where memidx is an Index and align is in the binary (log2) form.
And there's a lot of callsites in the SharedValidator and in the IR expr constructors that pass these three in the form where memidx is a Var and align is in the expanded form.
So I think the most elegant/comprehensive treatment might have some sort of (name TBD) MemLocationRaw, used in all the binary readers, and a (name also TBD) MemLocationCooked that's used in the SharedValidator and IR, with one codepath to convert from one to the other? Or we can just do one of these and leave the other unchanged.
Happy to do this now if you want but it might make sense for a separate PR.
d6b8159 to
44633e7
Compare
Before memory64, the "offset" in a load/store expression was a u32, and we enforced this in the WastParser and BinaryReader. After memory64, the "offset" becomes a u64 syntactically, and the validator checks that it's <= UINT32_MAX for i32 memories. We hadn't been correctly allowing these very large offsets in the text format (even when memory64 was enabled and the memory was i64). (This change also eliminates the "memories" member in the BinaryReader. The BinaryReader no longer needs to keep track of the memories and their types to check well-formedness.)
44633e7 to
62ed39a
Compare
Before memory64, the "offset" in a load/store expression was a u32, and we enforced this in the WastParser and BinaryReader. After memory64, the "offset" becomes a u64 syntactically, and the validator checks that it's <= UINT32_MAX for i32 memories.
We hadn't been correctly allowing these very large offsets in the text format (even when memory64 was enabled and the memory was i64).
(This change also eliminates the "memories" member in the BinaryReader. The BinaryReader no longer needs to keep track of the memories and their types to check well-formedness.)
This PR is logically dependent on #2252 (I don't think the tests will pass until #2252 is merged).All of this is a long prelude to eventually fixing #2192 with a good regression test!