1791768 - Assertion failure: isObject() in TeeReaderReadHandler

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Samuel Groß]

The following sample triggers a debug assertion failure in debug builds from latest HEAD:

function main() {
const v1 = eval();
const v3 = 0;
for (let v5 = 1; v5 < 100; v5++) {
    const v6 = 7;
    const v8 = this.oomAtAllocation(v5);
    try {
        const v9 = {};
        const v10 = [];
        const v13 = this.ReadableStream;
        const v14 = new v13();
        const v15 = v14.cancel();
        const v16 = v14.tee();
        const v18 = [v9,v1,v5,...v10];
    } catch(v19) {
        print(v19);
    }
}
gc();
}
main();
// CRASH INFO
// ==========
// TERMSIG: 11
// STDERR:
// Assertion failure: isObject(), at /home/builder/firefox/obj-fuzzbuild/dist/include/js/Value.h:818
// #01: JS::Value::toObject() const[./spidermonkey/js +0x16dad2e]
// #02: ???[./spidermonkey/js +0x1fa2a75]
// #03: ???[./spidermonkey/js +0x1f90bbc]
// #04: ???[./spidermonkey/js +0x17bfb4a]
// #05: ???[./spidermonkey/js +0x17bef81]
// #06: ???[./spidermonkey/js +0x17c0e72]
// #07: ???[./spidermonkey/js +0x185d3be]
// #08: ???[./spidermonkey/js +0x1b8e72c]
// #09: ???[./spidermonkey/js +0x17bfb4a]
// #10: ???[./spidermonkey/js +0x17bef81]
// #11: ???[./spidermonkey/js +0x17c0e72]
// #12: JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>)[./spidermonkey/js +0x197f96b]
// #13: ???[./spidermonkey/js +0x1aa1979]
// #14: js::RunJobs(JSContext*)[./spidermonkey/js +0x1aa1193]
// #15: ???[./spidermonkey/js +0x16aacf8]
// #16: ???[./spidermonkey/js +0x16c6b72]
// #17: ??? (???:???)

Backtrace from gdb:

#0  0x0000555557310334 in JS::Value::toObject (this=0x3da8c8285700) at obj-debug/dist/include/js/Value.h:818
#1  0x0000555557c88000 in js::TeeState::branch2 (this=0x3da8c82856b8) at js/src/builtin/streams/TeeState.h:139
#2  0x0000555557c7b885 in TeeReaderReadHandler (cx=0x7ffff772a100, argc=1, vp=0x7fffffffcb68) at js/src/builtin/streams/ReadableStreamOperations.cpp:236
#3  0x00005555574da27c in CallJSNative (cx=0x7ffff772a100, native=0x555557c7b4f0 <TeeReaderReadHandler(JSContext*, unsigned int, JS::Value*)>, reason=js::CallReason::Call, args=...) at js/src/vm/Interpreter.cpp:458
#4  0x00005555574c7f33 in js::InternalCallOrConstruct (cx=0x7ffff772a100, args=..., construct=js::NO_CONSTRUCT, reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:546
#5  0x00005555574c86b9 in InternalCall (cx=0x7ffff772a100, args=..., reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:613
#6  0x00005555574c8874 in js::Call (cx=0x7ffff772a100, fval=..., thisv=..., args=..., rval=..., reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:645
#7  0x00005555575a8714 in js::Call (cx=0x7ffff772a100, fval=..., thisv=..., arg0=..., rval=...) at js/src/vm/Interpreter.h:116
#8  0x000055555787c339 in PromiseReactionJob (cx=0x7ffff772a100, argc=0, vp=0x7fffffffd1f0) at js/src/builtin/Promise.cpp:2240
#9  0x00005555574da27c in CallJSNative (cx=0x7ffff772a100, native=0x55555787b940 <PromiseReactionJob(JSContext*, unsigned int, JS::Value*)>, reason=js::CallReason::Call, args=...) at js/src/vm/Interpreter.cpp:458
#10 0x00005555574c7f33 in js::InternalCallOrConstruct (cx=0x7ffff772a100, args=..., construct=js::NO_CONSTRUCT, reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:546
#11 0x00005555574c86b9 in InternalCall (cx=0x7ffff772a100, args=..., reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:613
#12 0x00005555574c8874 in js::Call (cx=0x7ffff772a100, fval=..., thisv=..., args=..., rval=..., reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:645
#13 0x00005555576763fa in JS::Call (cx=0x7ffff772a100, thisv=..., fval=..., args=..., rval=...) at js/src/vm/CallAndConstruct.cpp:117
#14 0x000055555778c623 in JS::Call (cx=0x7ffff772a100, thisv=..., funObj=..., args=..., rval=...) at obj-debug/dist/include/js/CallAndConstruct.h:110
#15 0x000055555778c2c6 in js::InternalJobQueue::runJobs (this=0x7ffff7738f80, cx=0x7ffff772a100) at js/src/vm/JSContext.cpp:880
#16 0x000055555778bee7 in js::RunJobs (cx=0x7ffff772a100) at js/src/vm/JSContext.cpp:817
#17 0x00005555572e38ab in RunShellJobs (cx=0x7ffff772a100) at js/src/shell/js.cpp:1147
#18 0x00005555572d59c6 in Shell (cx=0x7ffff772a100, op=0x7fffffffd748) at js/src/shell/js.cpp:11185
#19 0x00005555572d0578 in main (argc=2, argv=0x7fffffffd9e8) at js/src/shell/js.cpp:12275

To me it looks like what happens is that due to an OOM, one of the ReadableStream objects is somehow left in an inconsistent state, but is then still processed (TeeReaderReadHandler) by a pending job, leading to this crash. It seems that this crash would always lead to undefined being used as object, which I'd assume always leads to a crash and isn't exploitable. The issue may also be limited to these special js-shell builtins. I'm still filing this issue as a security bug as a precaution though.

Comment 1

[Andrew McCreight [:mccr8]]

Matthew, could you take a look? Thanks. (I'm not sure if this bug should be in Core: JS engine or DOM: Streams.)

Comment 2

[Kagami Rosylight [:saschanaz] (they/them)]

To me it looks like what happens is that due to an OOM, one of the ReadableStream objects is somehow left in an inconsistent state

Sounds like we really need bug 1762233 🥲. It's currently super likely that OOM can cause weird state for any DOM Stream objects but then we have no good way to differentiate OOM failures from others.

Comment 3

[Jan de Mooij [:jandem]]

IIUC this is about the streams implementation in SpiderMonkey that we're not using in the browser anymore. Maybe we can remove this implementation at some point.

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

Oops! Then DOM: Streams won't fit.

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

This failure is in the JS Streams implementation, which we're not shipping in Firefox; it's only available in shell builds.

At this point we probably should make it an explicit opt-in in configure, as we try to figure out the decommissioning plan.

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

(Fixed by deleting code)