Closed Bug 1571911 Opened 5 months ago Closed 5 months ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- fixed

People

(Reporter: gkw, Assigned: cfallin)

References

(Blocks 2 open bugs, Regression)

Details

(5 keywords, Whiteboard: [jsbugmon:][adv-main70+][adv-main70+r])

Crash Data

Attachments

(3 files)

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.

Attached file Testcase

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?

Flags: needinfo?(jcoppeard)
Regressed by: 1291292

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

Summary: Crash [@ js::SN_IS_TERMINATOR] → AddressSanitizer: SEGV [@ js::SN_IS_TERMINATOR]
Summary: AddressSanitizer: SEGV [@ js::SN_IS_TERMINATOR] → AddressSanitizer: SEGV Crash [@ js::SN_IS_TERMINATOR] involving wasm

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

Type: -- → defect

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.

Flags: needinfo?(nth10sd)
Keywords: sec-other

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

Flags: needinfo?(nth10sd)
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Judging from the stack this is JS, for now.

Component: Javascript: WebAssembly → JavaScript Engine

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

Flags: needinfo?(jcoppeard)

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

Flags: needinfo?(jdemooij)

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

Flags: needinfo?(jdemooij) → needinfo?(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.

Flags: needinfo?(cfallin)

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

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

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.

Flags: needinfo?(nth10sd)

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

Flags: needinfo?(nth10sd) → needinfo?(cfallin)

Great, thanks.

Assignee: nobody → cfallin
Group: javascript-core-security
Status: NEW → ASSIGNED

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

Flags: needinfo?(cfallin)

Thanks! Patch updated.

Keywords: checkin-needed

Pushed by dluca@mozilla.com:
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

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main70+][adv-main70-rollup]
Whiteboard: [jsbugmon:][adv-main70+][adv-main70-rollup] → [jsbugmon:][adv-main70+][adv-main70+r]
You need to log in before you can comment on or make changes to this bug.