1571911 - AddressSanitizer: SEGV Crash [@ js::SN_IS_TERMINATOR] involving wasm

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The following testcase crashes on mozilla-central revision fb699b3c084c (build with --enable-address-sanitizer, run with --fuzzing-safe --no-threads --no-baseline --no-ion w146-out.wrapper w146-out.wasm):

See attachment.

Backtrace:

AddressSanitizer:DEADLYSIGNAL
=================================================================
==1937==ERROR: AddressSanitizer: SEGV on unknown address 0x7f581f8b97c7 (pc 0x5620932d7780 bp 0x7fff572c19f0 sp 0x7fff572c19c0 T0)
==1937==The signal is caused by a READ memory access.
    #0 0x5620932d777f in js::SN_IS_TERMINATOR(unsigned char*) js/src/frontend/SourceNotes.h
    #1 0x5620932d777f in js::PCToLineNumber(unsigned int, unsigned char*, unsigned char*, unsigned char*, unsigned int*) js/src/vm/JSScript.cpp:4368
    #2 0x5620932521f4 in PopulateReportBlame(JSContext*, JSErrorReport*) js/src/vm/JSContext.cpp:270:25
    #3 0x562093251bcf in js::ReportErrorVA(JSContext*, unsigned int, char const*, js::ErrorArgumentsType, __va_list_tag*) js/src/vm/JSContext.cpp:408:3
    #4 0x562093a55d84 in JS_ReportErrorUTF8(JSContext*, char const*, ...) js/src/jsapi.cpp:4685:3
/snip

For detailed crash information, see attachment.

This involves WebAssembly as it was found by funbind, but the regressing changeset seems to involve GC, and this is an ASan issue, so setting s-s as a start.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

Attachment: Testcase

Comment 3

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

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/1a4509a3e2ce
user: Jon Coppeard
date: Thu Aug 11 17:14:56 2016 +0100
summary: Bug 1291292 - Use dynamic chunk allocation for the nursery r=terrence

Jon, is bug 1291292 a likely regressor?

Comment 4

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

I confirm that this does not happen with regular debug/opt builds (without --enable-address-sanitizer).

Comment 5

[Jon Coppeard (:jonco)]

That doesn't seem likely at first glance. The crash involves iterating through source notes, which are not nursery allocated.

Comment 6

[Daniel Veditz [:dveditz]]

Gary: this looks like a crash in jsshell error reporting so I'm calling it sec-other for now, but if the real problem is in Firefox JS code please clear or update that.

Comment 7

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

Yeah, shell stuff seem to be on the stack, but I'll punt to Jon to make the call.

Comment 8

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 9

[Lars T Hansen [:lth]]

Judging from the stack this is JS, for now.

Comment 10

[Jon Coppeard (:jonco)]

Clearing needinfo because I don't think this is GC related.

Comment 11

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

Folks say this isn't wasm, GC, so falling back on Jan as a start.

Comment 12

[Jan de Mooij [:jandem]]

Hi Chris, this is the bug I just mentioned :)

Comment 13

[Chris Fallin [:cfallin]]

I can't reproduce the exact asan error message above (I had to build with --enable-address-sanitizer --disable-jemalloc to avoid link errors... first time building js shell with asan, is this normal?) but I did hit another asan failure with this input which points to a very simple issue... a 64-to-32-bit conversion overflow!

js/src/shell/OSObject.cpp (which implements a typed array wrapper around files, used by this test case to load the wasm binary) allocates a buffer here. Note the size of the file, and thus allocation, is a size_t. However, the definition of JS_NewUint8Array takes a 32-bit uint32_t here. This test case has a ~5GB input, hence overflow in the implicit conversion.

Will send a patch to check max size before alloc. Because Uint8Array's max size is 4GB, this input is now (correctly) recognized as invalid.

Comment 14

[Chris Fallin [:cfallin]]

Attachment: Bug 1571911: avoid uint32_t overflow in js shell by checking size of Uint8Array allocation for file buffer. r=jandem

Comment 15

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

(In reply to Chris Fallin [:cfallin] from comment #13)

(I had to build with --enable-address-sanitizer --disable-jemalloc to avoid link errors... first time building js shell with asan, is this normal?)

Yes, one has to disable jemalloc with Valgrind/ASan.

Comment 16

[Lars T Hansen [:lth]]

FWIW, in SpiderMonkey Uint8Array should max out at 2GB currently, we don't allow objects larger than that.

Comment 17

[Jan de Mooij [:jandem]]

Gary, can you double check the patch fixes your ASan issue too? Just to make sure both of you hit the same issue before we open this up.

Comment 18

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

I confirm that the patch fixes the issue, it now shows an error that the .wasm file is too large for a Uint8Array.

Comment 19

[Jan de Mooij [:jandem]]

Great, thanks.

Comment 20

[Chris Fallin [:cfallin]]

(In reply to Lars T Hansen [:lth] from comment #16)

FWIW, in SpiderMonkey Uint8Array should max out at 2GB currently, we don't allow objects larger than that.

Good to know, thanks -- as a sanity check, I verified that if I truncate the testcase to 3GB (> object limit, but < overflow) I get a RangeError: invalid array length, so we're safe in that size range as well. I'm inclined to leave the check in the patch with UINT32_MAX since that's the API type on the actual alloc function? If that's fine, I'll go ahead and set checkin-needed.

Comment 21

[Lars T Hansen [:lth]]

Probably best to use MaxBufferByteLength? https://searchfox.org/mozilla-central/source/js/src/vm/ArrayBufferObject.h#169

Comment 22

[Chris Fallin [:cfallin]]

Thanks! Patch updated.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6910c6ac8d4f
avoid uint32_t overflow in js shell by checking size of Uint8Array allocation for file buffer. r=jandem

Comment 24

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/6910c6ac8d4f