Baldr: Update binary to (temporary) text rendering

RESOLVED FIXED in Firefox 48

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
Created attachment 8736283 [details] [diff] [review]
fix.binary.text.patch

This updates BinaryToText to fully validate the entire test suite, to ensure that the binary to text conversion is a bijection (modulo names, spaces, etc.). To ensure this, I've slightly changed wasmEvalText by adding this line:

str = wasmBinaryToText(wasmTextToBinary(str));
// then Wasm.instantiate(wasmTextToBinary(str), etc.)

Also, it makes wasmBinaryToText raise the same errors that wasmTextToBinary would, so the above transformation can work. Thinking about it, maybe some of the error checking should go to Wasm.cpp rather than WasmTextToBinary.cpp in the first place...

I know the text format is supposed to be short-lived, but it has lived long enough, and binary to text has proven useful for understanding fuzzbugs. Plus I think some additions in this patch will show useful for the future (e.g. rendering select, branches with values, i64, etc.). Plus Dan hasn't touched the binary to text transformation yet in the postorder patch. So it seems worth to have in tree.
Attachment #8736283 - Flags: review?(luke)
(Assignee)

Comment 1

2 years ago
Created attachment 8736284 [details] [diff] [review]
projection.patch

For what it's worth, the patch used for testing.

Comment 2

2 years ago
Comment on attachment 8736283 [details] [diff] [review]
fix.binary.text.patch

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

Nice, thanks!
Attachment #8736283 - Flags: review?(luke) → review+

Comment 3

2 years ago
Christian, do you suppose you could have your (most excellent) wasm fuzzer scripts use wasmBinaryToText() to print the binaries as text in the bug reports?  It won't always work (in cases where the binary is invalid), but for (the majority, I think) of the cases it does, it'd both save the time of posting the binary testcase *and* save us the time of doing it locally.
(In reply to Luke Wagner [:luke] from comment #3)
> Christian, do you suppose you could have your (most excellent) wasm fuzzer
> scripts use wasmBinaryToText() to print the binaries as text in the bug
> reports?  It won't always work (in cases where the binary is invalid), but
> for (the majority, I think) of the cases it does, it'd both save the time of
> posting the binary testcase *and* save us the time of doing it locally.

Posting the testcase is done fully automatically and doesn't involve any manual action on our side. The only thing I could do is have the reproduction script *also* output the text representation and include that output in the bug report in addition to the binary file. Not sure how well that works in our process but I could try it out.
(Assignee)

Comment 5

2 years ago
That would be great, but I assume this wouldn't work, most of the time: the test cases often have some invalid sections after the functions section, so showing the output would just throw an error when decoding the module.

If you could make it that all sections be valid (esp. ensure section sizes are correct), that would be awesome!
(In reply to Benjamin Bouvier [:bbouvier] from comment #5)
> That would be great, but I assume this wouldn't work, most of the time: the
> test cases often have some invalid sections after the functions section, so
> showing the output would just throw an error when decoding the module.
> 
> If you could make it that all sections be valid (esp. ensure section sizes
> are correct), that would be awesome!

We have no way to do that. The fuzzer we use doesn't generate valid wasm but uses simple binary mutations on existing wasm. The only way I could imagine this to work is by *you* providing a function that syntactically "fixes" given wasm binary data. We could call this function and see if the resulting test still reproduces the problem.

Comment 7

2 years ago
Ah, good point bbouvier.  I was thinking about the runtime errors (which means validation succeeded), but these compile-time errors are indeed likely to have trash at the end.  A related idea we already discussed is having the BinaryToText() do a best-effort printing when the input is invalid by printing up to the last valid bits.  We could use this for validation error messages as well.

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf8dc00cc76f

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf8dc00cc76f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.