Closed Bug 1394774 Opened 3 years ago Closed 3 years ago

Wasm: DecodeExport does not read exportKind in the same way as the spec interpreter

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(3 files)

Attached file decoder-test.wasm
The spec interpreter reads a u8 (binary/decode.ml, function export_desc).  We read a varUint32 (WasmValidate.cpp, function DecodeExport).  On the attached test case, wasm2wast reads 0x83 0x80 0x80 0x80 0x00 as 0x83 and consumes only the first byte, whereas we read 3 and consume all five bytes.
According to BinaryEncoding.md this is a single-byte field, so this is our bug.
Attached file decoder-test.js
Script to load test case
Read export kind with readFixedU8 instead of readVarU32, as the field is supposed to be a single byte.
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Attachment #8902230 - Flags: review?(luke)
Comment on attachment 8902230 [details] [diff] [review]
bug1394774-wasm-export-kind.patch

Review of attachment 8902230 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, good point.  This also matches the spec, which ends up specifying these codes as single-byte terminal symbols in the grammar of the binary format, e.g.:
  https://webassembly.github.io/spec/binary/modules.html#export-section
Therefore a LEB128 encoding of the same number won't match.

There is a similar spec mismatch in basically every place we readVarU32() into a fixed set of values:
 - both readVarU32(&idValue) in startSection
 - readVarU32(&nameTypeValue) in startNameSubsection
 - readVarU32(&form) in DecodeTypeSection()
 - readVarU32(&flags) in DecodeLimits
 - readVarU32(&elementType) in DecodeTableLimits
 - readVarU32(&rawImportKind) in DecodeImport
 - readVarU32(&flags) in OpIter::readCurrentMemory, readGrowMemory, readCallIndirect
(I confirmed each with webassembly.github.io/spec.)  Could you fix these too?

(I'd double-check that, e.g., webassembly.org/demo and ZenGarden still validate.)
Attachment #8902230 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #4)
> Comment on attachment 8902230 [details] [diff] [review]
> bug1394774-wasm-export-kind.patch
> 
> Review of attachment 8902230 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm, good point.  This also matches the spec, which ends up specifying these
> codes as single-byte terminal symbols in the grammar of the binary format,
> e.g.:
>   https://webassembly.github.io/spec/binary/modules.html#export-section
> Therefore a LEB128 encoding of the same number won't match.
> 
> There is a similar spec mismatch in basically every place we readVarU32()
> into a fixed set of values:...
>
> Could you fix these too?

Sure.
Oops, also readVarU32(&flags) in DecodeGlobalType
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/053e0c2c51ec
wasm, read u8 fields as u8, not as varu32. r=luke
https://hg.mozilla.org/mozilla-central/rev/053e0c2c51ec
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.