Open Bug 2037997 Opened 1 month ago Updated 1 month ago

Crash [@ WasmGlobalExtractLane / WasmGlobalToString] — missing null check after WasmGlobalObject::create / JS_NewStringCopyZ under OOM

Categories

(Core :: JavaScript Engine: JIT, defect, P2)

defect

Tracking

()

UNCONFIRMED

People

(Reporter: david.lie, Unassigned)

Details

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/147.0.0.0 Safari/537.36

Steps to reproduce:

Two shell testing helpers in js/src/builtin/TestingFunctions.cpp pass the return
value of a fallible allocation directly to args.rval().setObject() /
args.rval().setString() without null-checking, triggering a debug assertion abort
under OOM pressure.

Variant A — WasmGlobalExtractLane (TestingFunctions.cpp:1366):

Assertion failure: js::gc::IsCellPointerValid(&obj),
  at js/include/js/Value.h:604

Variant B — WasmGlobalToString (TestingFunctions.cpp:1612):

Assertion failure: js::gc::IsCellPointerValid(str),
  at js/include/js/Value.h:586

Both reproduce with and without --fuzzing-safe.


Steps to reproduce

Variant A (attached as poc_extract_lane.js):

gcparam("maxBytes", gcparam("gcBytes") + 4 * 1024);
var buf = new ArrayBuffer(16);
new Uint8Array(buf).fill(17);
var g = wasmGlobalFromArrayBuffer('v128', buf);
for (var i = 0; i < 5000; i++) {
    wasmGlobalExtractLane(g, 'i32x4', i & 3);
}

Run:

./js --fuzzing-safe poc_extract_lane.js

Variant B (attached as poc_to_string.js):

gcparam("maxBytes", gcparam("gcBytes") + 4096);
var buf = new ArrayBuffer(16);
new Uint8Array(buf).fill(42);
var g = wasmGlobalFromArrayBuffer('v128', buf);
for (var i = 0; i < 10000; i++) {
    wasmGlobalToString(g);
}

Run:

./js --fuzzing-safe poc_to_string.js

The gcparam("maxBytes", …) call caps the GC heap 4 KB above the current live
data. After a few hundred loop iterations the cap is reached, the allocation
inside WasmGlobalExtractLane or WasmGlobalToString returns null, and the
unchecked setObject / setString asserts.

No additional flags are required.


Root cause

Variant AWasmGlobalExtractLane (TestingFunctions.cpp:1364–1366):

Rooted<WasmGlobalObject*> result(
    cx, WasmGlobalObject::create(cx, val, false, proto));
args.rval().setObject(*result.get());   // ← no null check

WasmGlobalObject::create returns nullptr on OOM; *result.get() dereferences
null and passes a null reference to setObject(JSObject&).

The sibling helper WasmGlobalFromArrayBuffer (lines 1220–1232) already has the
correct null checks — this is a copy-paste omission in WasmGlobalExtractLane.

Variant BWasmGlobalToString (TestingFunctions.cpp:1612):

args.rval().setString(JS_NewStringCopyZ(cx, result.get()));  // ← no null check

JS_NewStringCopyZ returns nullptr on OOM; the result is passed directly to
setString(JSString*).


Fix

Add null checks consistent with the pattern in WasmGlobalFromArrayBuffer:

Variant A (after line 1365):

if (!result) {
    return false;
}

Also add a proto null-check (after line 1363):

if (!proto) {
    return false;
}

Variant B (split the one-liner at line 1612):

JSString* str = JS_NewStringCopyZ(cx, result.get());
if (!str) {
    return false;
}
args.rval().setString(str);

The rest of TestingFunctions.cpp is worth a quick audit for the same pattern.


Build / repro environment

  • Engine: SpiderMonkey JavaScript-C142.0a1
  • Tree HEAD: 5836a062726f ("Bug 1960567 — remove last C++ scriptable APIs for legacy telemetry")
  • Platform: Linux x86_64

Actual results:

Actual results

Variant A:

[PID] Assertion failure: js::gc::IsCellPointerValid(&obj),
  at /…/js/include/js/Value.h:604

Stack (UBSan symbolised):
  #0 MOZ_CrashSequence(void*, long)           mozilla/Assertions.h:248
  #1 JS::Value::setObject(JSObject&)           js/Value.h:604
  #2 JS::ObjectValue(JSObject&)                js/Value.h:1150
  #3 MutableWrappedPtrOperations<…>::setObject js/Value.h:1362
  #4 WasmGlobalExtractLane(…)                  builtin/TestingFunctions.cpp:1366
  #5 [anon:js-executable-memory]               (call trampoline)

Variant B:

[PID] Assertion failure: js::gc::IsCellPointerValid(str),
  at /…/js/include/js/Value.h:586

Stack (UBSan symbolised):
  #0 MOZ_CrashSequence(void*, long)           mozilla/Assertions.h:248
  #1 JS::Value::setString(JSString*)           js/Value.h:586
  #2 WasmGlobalToString(…)                     builtin/TestingFunctions.cpp:1612
  #3 [anon:js-executable-memory]               (call trampoline)

The follow-on UBSan SEGV is the deliberate MOZ_CrashSequence null write, not
an independent fault.

Expected results:

Expected results

Both functions should return false (with an OOM exception already set on the
context) when the internal allocation fails, rather than asserting.

Summary: WasmGlobalObject → Crash [@ WasmGlobalExtractLane / WasmGlobalToString] — missing null check after WasmGlobalObject::create / JS_NewStringCopyZ under OOM
Severity: -- → S4
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.