Closed Bug 1394771 Opened 7 years ago Closed 7 years ago

Wasm: DecodeName does not validate UTF8

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

(1 file, 2 obsolete files)

Attached file decoder-test.wasm (obsolete) —
According to the spec interpreter (binary/decode.ml, in the function 'name') the decoding of a name (as for an export) must validate that the name represents valid UTF-8.

Our corresponding function, DecodeName in WasmValidate.cpp, does not do this.  As a result, we may fail later, when the name is used.  I have a test case from decoder which has invalid UTF8 in an export name, and it passes module creation but fails during instantiation because our atomization procedure checks for valid UTF-8.

(Attaching the test case, though there may be other problems with it as well, so beware.)
See Also: → 1394774
So, the attached test case may not exhibit the problem, exactly - I'm not sure.  First we should fix bug 1394774, since it's the misinterpretation of the export kind that leads us astray in this test case.  After that we may need a different test case to show that we're not validating utf8 properly.
Specifically, we may fail in AtomizeUTF8Chars when trying to create an export object with one of these non-utf8 strings.  Since we don't trip over the illegal export kind (see comment 1) we end up creating an invalid UTF8 name and then later we fail here.
Attached file decoder-test.js (obsolete) —
Script to load test case
Assignee: nobody → lhansen
Simple patch, with test case.
Attachment #8902214 - Attachment is obsolete: true
Attachment #8902228 - Attachment is obsolete: true
Attachment #8902241 - Flags: review?(luke)
Comment on attachment 8902241 [details] [diff] [review]
bug1394771-check-utf8-ness-of-name.patch

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

Thanks!

::: js/src/vm/CharacterEncoding.cpp
@@ +511,5 @@
> +            len = 3;
> +        else if ((*s & 0xF8) == 0xF0)
> +            len = 4;
> +        if (len == 0)
> +            return false;

Instead of 'if (len == 0)', how about 'else'?

@@ +516,5 @@
> +        if (s + len > limit)
> +            return false;
> +        for (uint32_t i = 1; i < len; i++)
> +            if ((s[i] & 0xC0) != 0x80)
> +                return false;

nit: { } around body of for loop
Attachment #8902241 - Flags: review?(luke) → review+
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d9babb54886
Check that a wasm name is UTF8 when we first read it. r=luke
https://hg.mozilla.org/mozilla-central/rev/9d9babb54886
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1396220
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: