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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Detailed Crash Information

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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?

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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)

Comment 4

[Michelangelo De Simone [Offline] [:mds]]

(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?

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

(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.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 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.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 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.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8887870 [details] [diff] [review]
fix.patch

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

Thanks for looking at this Benjamin!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b8a76e20661
Fix benign assertion issue when decoding wasm names; r=luke

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the quick review!

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6b8a76e20661

Comment 12

[Ryan VanderMeulen [:RyanVM]]

I assume we don't care about uplifting this since it's just changing an assertion.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

(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!