1796744 - Various assertion failures due to unexpectedly pending exception from OOM during printf

Status: RESOLVED DUPLICATE of bug 1839396

(Core :: JavaScript Engine, defect, P3)

Description

[Samuel Groß]

During fuzzing with Fuzzilli, I get a lot of crashes from assertion failures such as
- !JS_IsExceptionPending(cx_), at js/src/vm/JSContext.cpp:1325
- !JS_IsExceptionPending(cx_), at js/src/vm/JSContext.h:1074
- !cx->isExceptionPending(), at js/src/vm/JSContext-inl.h:252

Below are two samples that reproduce these issues on a debug build from latest HEAD (happy to provide other samples as well if that'd be helpful):

function main() {
let v1 = 0;
do {
    const v2 = v1++;
    try {
        const v4 = this.oomAtAllocation(v2);
        const v6 = this.dis();
    } catch(v7) {
        const v8 = /l\WV\W.*/myi;
        const v10 = new Float64Array(v8);
    }
} while (v1 < 1000.0);
gc();
}
main();
// CRASH INFO
// ==========
// TERMSIG: 11
// STDERR:
// Assertion failure: !cx->isExceptionPending(), at /home/builder/firefox/js/src/vm/JSContext-inl.h:252
// ARGS:

And

function main() {
let v0 = "-9223372036854775807";
for (const v1 in v0) {
    for (const v3 in "undefined") {
        const v4 = /.\S\Sr\Sc.JoXV\w/;
        let v5 = v4.__proto__;
        const v6 = [v4,v5,v4,v5,v4,v4,v5,v4,v5,v5];
        v5 %= v6;
    }
    try {
        const v8 = null;
        v0.length = 1024;
        const v12 = this.evalInWorker("268435456");
        for (let v14 = v1; v14 < 100; v14++) {
            const v16 = this.oomAtAllocation(v14);
            const v18 = this.disassemble();
            const v20 = [151544.9880271703,151544.9880271703];
        }
    } catch(v21) {
    }
}
const v22 = v0[v0];
v0 &= v0;
const v24 = [151544.9880271703];
gc();
}
main();
// CRASH INFO
// ==========
// TERMSIG: 11
// STDERR:
// Assertion failure: !JS_IsExceptionPending(cx_), at /home/builder/firefox/js/src/vm/JSContext.cpp:1325
// ARGS: --baseline-warmup-threshold=10 --ion-warmup-threshold=100

From what I understand, I don't think these assertions have any security implications but wanted to let you double check (and then downgrade this issue to a regular bug).

Comment 1

[Jan de Mooij [:jandem]]

I looked at this briefly. Both of these are similar. Under JSScript::dump we call JSONPrinter::propertyName where we have:

  out_.printf("\"%s\":", name);

However GenericPrinter::printf is fallible, so we end up ignoring an OOM and leave a pending exception on the context. If I add [[nodiscard] to some of these methods, I get a lot of warnings.

I'm not sure what the best fix is. Maybe the printer API should be more like the masm interface with an OOM check at the end instead of fallible methods for everything?

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #1)

I'm not sure what the best fix is. Maybe the printer API should be more like the masm interface with an OOM check at the end instead of fallible methods for everything?

This sounds like this would be a sensible behavior.
I should probably look at it next week.

Comment 3

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

AFAIK this is fixed by Bug 1839396, correct?

Comment 4

[Nicolas B. Pierron [:nbp]]

Testing on top of these changes, both test case are passing now.

This is most likely a duplicate of Bug 1839396, which was meant to solve this issue by making the GenericPrinter interface infallible until the point where the string outputs are needed, as suggested by Jan in comment 1.