Closed
Bug 1394771
Opened 7 years ago
Closed 7 years ago
Wasm: DecodeName does not validate UTF8
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: lth, Assigned: lth)
References
Details
Attachments
(1 file, 2 obsolete files)
2.97 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Comment 1•7 years ago
|
||
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.
Assignee | ||
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
Script to load test case
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → lhansen
Assignee | ||
Comment 4•7 years ago
|
||
Simple patch, with test case.
Attachment #8902214 -
Attachment is obsolete: true
Attachment #8902228 -
Attachment is obsolete: true
Attachment #8902241 -
Flags: review?(luke)
Comment 5•7 years ago
|
||
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
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9d9babb54886
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•