Closed Bug 1396557 Opened 7 years ago Closed 7 years ago

JS::StringIsUTF8, used for Wasm name validation, must be more stringent

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(1 file)

As pointed out in bug 1396220, the checker is too lenient with illegal pairs and some out-of-range values.  I failed to notice those restrictions when I implemented the code earlier.  The correct restrictions are in https://github.com/WebAssembly/spec/blob/master/interpreter/binary/utf8.ml.

We'll not wait for bug 1396220 to be resolved (since that involves politics), but will fix this in our codebase and let that thing play out independently.
This adds the value computation and the range checks from the reference implementation, see link in comment 0.
Attachment #8904224 - Flags: review?(luke)
(In reply to Lars T Hansen [:lth] from comment #1)
> This adds the value computation

FWIW, Computing the scalar value isn't logically necessary. The validity can be decided from the byte values alone by limiting the range of the first continuation depending on what the value of the lead was.
(In reply to Henri Sivonen (:hsivonen) from comment #2)
> (In reply to Lars T Hansen [:lth] from comment #1)
> > This adds the value computation
> 
> FWIW, Computing the scalar value isn't logically necessary. The validity can
> be decided from the byte values alone by limiting the range of the first
> continuation depending on what the value of the lead was.

OK, that's an interesting fact.  Even so I'll leave that for future work, I think.
Attachment #8904224 - Flags: review?(luke) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8822dc7f2d3c
Make JS::StringIsUTF8 stricter. r=luke
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8822dc7f2d3c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.