[wasm] Assertion failure: n.offset + n.length < maybeBytecode->length(), at js/src/wasm/WasmCode.cpp:635

RESOLVED FIXED in Firefox 56

Status

()

--
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gkw, Assigned: bbouvier)

Tracking

(Blocks: 2 bugs, {assertion, jsbugmon, testcase})

Trunk
mozilla56
x86_64
Linux
assertion, jsbugmon, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 wontfix, firefox56 fixed)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
The following testcase crashes on mozilla-central revision dece50457378 (build with --enable-debug, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager):

// Adapted from randomly chosen test: js/src/jit-test/tests/profiler/enterjit-osr-enabling-earlyret.js
enableGeckoProfilingWithSlowAssertions();
// Adapted from randomly chosen test: js/src/jit-test/tests/wasm/binary.js
new WebAssembly.Instance(new WebAssembly.Module((function () {
    return Uint8Array.from([
        0, 97, 115, 109, 1, 0, 0, 0, 1, 4, 1, 96, , 0, 3, 2, 1, 0, 10, 4, 1,
        2, , 11, , 10, 4, 110, 97, 109, 101, 1, 3, 1, 0, 0
    ])
})()));


Backtrace:

#0  0x0000000000d73d80 in js::wasm::Metadata::getFuncName (this=<optimized out>, maybeBytecode=<optimized out>, funcIndex=<optimized out>, name=0x7ffc69b42b00) at js/src/wasm/WasmCode.cpp:635
#1  0x0000000000d7c142 in js::wasm::Code::ensureProfilingLabels (this=0x7f2a22814580, maybeBytecode=<optimized out>, profilingEnabled=<optimized out>) at js/src/wasm/WasmCode.cpp:885
#2  0x0000000000da63f3 in js::wasm::Instance::ensureProfilingLabels (this=this@entry=0x7f2a228f8500, profilingEnabled=<optimized out>) at js/src/wasm/WasmInstance.cpp:790
#3  0x0000000000d846e5 in js::wasm::Compartment::registerInstance (this=0x7f2a228583e0, cx=cx@entry=0x7f2a22873000, instanceObj=..., instanceObj@entry=...) at js/src/wasm/WasmCompartment.cpp:68
#4  0x0000000000e173e4 in js::wasm::Module::instantiate (this=this@entry=0x7f2a1fe0ee10, cx=cx@entry=0x7f2a22873000, funcImports=funcImports@entry=..., tableImport=..., tableImport@entry=..., memoryImport=..., memoryImport@entry=..., globalImports=..., instanceProto=..., instance=...) at js/src/wasm/WasmModule.cpp:1039
/snip

For detailed crash information, see attachment.
(Reporter)

Comment 1

a year ago
Created attachment 8887708 [details]
Detailed Crash Information
(Reporter)

Comment 2

a year ago
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/c67aff0a734d
user:        Michelangelo De Simone
date:        Thu May 18 09:51:02 2017 -0500
summary:     Bug 1357911 - Baldr: update names section format (r=luke)

Michelangelo, is bug 1357911 a likely regressor?
Blocks: 1357911
Flags: needinfo?(mdesimone)
(Reporter)

Comment 3

a year ago
Benjamin, do you think the awsm fuzzer can generate this? This was quite a painful testcase to reduce, fwiw. (Lots of inlining to create the required list)
Flags: needinfo?(bbouvier)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)

> Michelangelo, is bug 1357911 a likely regressor?

Aww, the new name section, so many memories...:)
Not sure about that TBH, :luke, any idea what's going on here?
Flags: needinfo?(mdesimone) → needinfo?(luke)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #3)
> Benjamin, do you think the awsm fuzzer can generate this? This was quite a
> painful testcase to reduce, fwiw. (Lots of inlining to create the required
> list)

awsm doesn't generate name sections yet, so it couldn't have found this one. That's a great catch, btw!

(In reply to Michelangelo De Simone [:mds] from comment #4)
> (In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)
> 
> > Michelangelo, is bug 1357911 a likely regressor?
> 
> Aww, the new name section, so many memories...:)
> Not sure about that TBH, :luke, any idea what's going on here?

It seems there's at least an off-by-one in this assertion (it should probably be offset+func_name_length <= total_length), but in general there's no validation at all on NameInBytecode values.
Flags: needinfo?(bbouvier)
Created attachment 8887869 [details] [diff] [review]
fix.patch

This is just the assertion being incorrect:

- if there was a hole in the name section (e.g. a module with two functions but only the second one has a name), then the first name will be uninitialized, with an offset of UINT32_MAX. So we have to move the assert into the `if name.length != 0` branch.
- there's an off-by-one error in this assertion, if the name section is located at the end of the bytecode.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Flags: needinfo?(luke)
Attachment #8887869 - Flags: review?(luke)
Created attachment 8887870 [details] [diff] [review]
fix.patch

Figured we could do slightly better: to not record 0-length names at all. This doesn't prevent the holes issue described in the previous comment, so we need to keep the `if name.length != 0` in Metadata::getFuncName.
Attachment #8887869 - Attachment is obsolete: true
Attachment #8887869 - Flags: review?(luke)
Attachment #8887870 - Flags: review?(luke)
Comment on attachment 8887870 [details] [diff] [review]
fix.patch

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

Thanks for looking at this Benjamin!
Attachment #8887870 - Flags: review?(luke) → review+

Comment 9

a year ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b8a76e20661
Fix benign assertion issue when decoding wasm names; r=luke
Thanks for the quick review!

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6b8a76e20661
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
I assume we don't care about uplifting this since it's just changing an assertion.
status-firefox54: --- → unaffected
status-firefox55: --- → wontfix
status-firefox-esr52: --- → unaffected
(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> I assume we don't care about uplifting this since it's just changing an
> assertion.

I do confirm, thanks for setting the flags!
You need to log in before you can comment on or make changes to this bug.