Closed Bug 1390082 Opened 3 years ago Closed 3 years ago

Crash [@ JS::Value::toInt32] or Assertion failure: isObject(), at dist/include/js/Value.h:642 involving async and yield

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- disabled
firefox56 --- disabled
firefox57 --- verified

People

(Reporter: gkw, Assigned: arai)

References

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(4 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision 3bfcbdf5c6c3 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

// Adapted from randomly chosen test: js/src/tests/test262/built-ins/Array/prototype/reduceRight/15.4.4.22-8-b-iii-1-22.js
Object.defineProperty(Array.prototype, "2", {
    set: function () {}
});
// Adapted from randomly chosen test: js/src/tests/test262/language/statements/async-generator/yield-identifier-spread-non-strict.js
async function* f() {
    yield {}
}
var x = f();
x.next();
x.next();
x.next();


Backtrace:

#0  0x000000000046e130 in JS::Value::toObject (this=<optimized out>) at /home/ubuntu/shell-cache/js-dbg-64-dm-linux-3bfcbdf5c6c3/objdir-js/dist/include/js/Value.h:642
#1  0x0000000000af59fa in js::WrappedPtrOperations<JS::Value, JS::Rooted<JS::Value> >::toObject (this=0x7ffd67c2fd30) at /home/ubuntu/shell-cache/js-dbg-64-dm-linux-3bfcbdf5c6c3/objdir-js/dist/include/js/Value.h:1335
#2  js::AsyncGeneratorObject::peekRequest (cx=cx@entry=0x7f0283055000, asyncGenObj=..., asyncGenObj@entry=...) at js/src/vm/AsyncIteration.cpp:380
#3  0x00000000005c8fb7 in AsyncGeneratorResumeNext (cx=cx@entry=0x7f0283055000, asyncGenObj=asyncGenObj@entry=...) at js/src/builtin/Promise.cpp:2597
#4  0x00000000005c998f in js::AsyncGeneratorResolve (cx=cx@entry=0x7f0283055000, asyncGenObj=..., asyncGenObj@entry=..., value=..., value@entry=..., done=<optimized out>) at js/src/builtin/Promise.cpp:2540
#5  0x0000000000af87e7 in AsyncGeneratorReturned (value=..., asyncGenObj=..., cx=<optimized out>) at js/src/vm/AsyncIteration.cpp:416
/snip

For detailed crash information, see attachment.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/a91dd87054d3
user:        Tooru Fujisawa
date:        Fri Aug 04 13:04:31 2017 +0900
summary:     Bug 1379525 - Part 1: Await on the value before yielding or returning inside async generator. r=shu,till

Arai-san, is bug 1379525 a likely regressor?
Blocks: 1379525
Flags: needinfo?(arai.unmht)
// Adapted from randomly chosen testcase: js/src/tests/test262/built-ins/Object/defineProperty/15.2.3.6-3-256-1.js
Object.prototype.set = function () {};
// Adapted from randomly chosen testcase: js/src/tests/test262/language/expressions/async-generator/named-yield-spread-arr-multiple.js
let f = async function* () {
    yield
};
let x = f();
x.next();
x.next().then(function () {
    x.next();
});
// Adapted from randomly chosen testcase: js/src/tests/test262/built-ins/Array/prototype/map/15.4.4.19-8-c-i-6.js
Object.defineProperty(Array.prototype, "0", {
    get: function () {
        return 1
    }
});

crashes opt shell with --fuzzing-safe --no-threads --no-baseline --no-ion at JS::Value::toInt32 with the following configuration command:

AR=ar sh ./configure --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

This gives the same assertion failure with debug builds.
Crash Signature: [@ JS::Value::toInt32]
Keywords: crash
Summary: Assertion failure: isObject(), at dist/include/js/Value.h:642 → Crash [@ JS::Value::toInt32] or Assertion failure: isObject(), at dist/include/js/Value.h:642
Attached file opt crash stack
Opt crash stack for testcase in comment 3.
Summary: Crash [@ JS::Value::toInt32] or Assertion failure: isObject(), at dist/include/js/Value.h:642 → Crash [@ JS::Value::toInt32] or Assertion failure: isObject(), at dist/include/js/Value.h:642 involving async and yield
Setting s-s. I have a testcase that seems to access a weird memory address 0x6e6f6c6f6369:

(gdb) x/i $pc
=> 0x553e05 <FulfillMaybeWrappedPromise(JSContext*, JS::HandleObject, JS::HandleValue)+101>:	mov    (%rdi),%rax
(gdb) x/b $rdi
0x6e6f6c6f6369:	Cannot access memory at address 0x6e6f6c6f6369
(gdb)

that reduces to the testcases here.
Group: javascript-core-security
Attached file unreduced testcase
Also tested on an opt shell from m-c rev 3bfcbdf5c6c3 with --fuzzing-safe --no-threads --no-baseline --no-ion :

(gdb) bt
#0  FulfillMaybeWrappedPromise (cx=cx@entry=0x7ffff6952000, promiseObj=promiseObj@entry=..., value_=value_@entry=...)
    at /home/ubuntu/trees/mozilla-central/js/src/builtin/Promise.cpp:655
#1  0x00000000005557df in ResolvePromiseInternal (cx=cx@entry=0x7ffff6952000, promise=promise@entry=..., resolutionVal=resolutionVal@entry=...)
    at /home/ubuntu/trees/mozilla-central/js/src/builtin/Promise.cpp:430
#2  0x0000000000557f57 in js::AsyncGeneratorResolve (cx=cx@entry=0x7ffff6952000, asyncGenObj=..., asyncGenObj@entry=..., value=value@entry=..., done=done@entry=true)
    at /home/ubuntu/trees/mozilla-central/js/src/builtin/Promise.cpp:2536
#3  0x000000000092736c in AsyncGeneratorReturned (value=..., asyncGenObj=..., cx=0x7ffff6952000) at /home/ubuntu/trees/mozilla-central/js/src/vm/AsyncIteration.cpp:416
#4  js::AsyncGeneratorResume (cx=cx@entry=0x7ffff6952000, asyncGenObj=asyncGenObj@entry=..., completionKind=completionKind@entry=(js::CompletionKind::Return | unknown: 76), 
    argument=..., argument@entry=...) at /home/ubuntu/trees/mozilla-central/js/src/vm/AsyncIteration.cpp:504
#5  0x0000000000557907 in AsyncGeneratorResumeNext (cx=cx@entry=0x7ffff6952000, asyncGenObj=asyncGenObj@entry=...)
    at /home/ubuntu/trees/mozilla-central/js/src/builtin/Promise.cpp:2666
#6  0x00000000005582a5 in js::AsyncGeneratorEnqueue (cx=0x7ffff6952000, asyncGenVal=..., completionKind=completionKind@entry=js::CompletionKind::Normal, 
    completionValue=..., result=...) at /home/ubuntu/trees/mozilla-central/js/src/builtin/Promise.cpp:2714
/snip
thanks.

the issue is that we use array_push/array_shift for internal queue object, but it's affected by monkey-patched Array prototype...
I'll re-implement the queue with better way.
So far, here's simplified implementation that does push/shift with [[Define]] instead of [[Set]], and also ignoring holes and some exceptions.
Maybe we could add some fast path like array_push/array_shift tho,
not sure if it worth adding them (not sure how much this path can become hot...)
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Flags: needinfo?(arai.unmht)
Attachment #8897048 - Flags: review?(till)
Comment on attachment 8897048 [details] [diff] [review]
Implement AsyncGeneratorQueue with simpler array operations.

Review of attachment 8897048 [details] [diff] [review]:
-----------------------------------------------------------------

Ugh, yes.

I'm fine with landing this as-is, but another option would be to take the queue handling stuff from Stream.cpp and generalize them - see {AppendTo,Peek,ShiftFrom}List.

Those should be more efficient because they use the optimized machinery jandem introduced in bug 1348772. Not sure how important that is here, but it doesn't seem entirely irrelevant.

Anyway, feel free to land this as-is, but perhaps file a follow-up bug for a more efficient implementation?
Attachment #8897048 - Flags: review?(till) → review+
Thank you for reviewing and letting me know about List code in Stream.cpp :)
I'll fix the patch to use them, since in that way the patch looks more like just an optimization.
Moved List functions to vm/List-inl.h, and used them in AsyncIteration.cpp
Attachment #8897048 - Attachment is obsolete: true
Attachment #8897311 - Flags: review?(till)
Comment on attachment 8897311 [details] [diff] [review]
Use optimized List operation for AsyncGeneratorQueue.

Review of attachment 8897311 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you, looks great!

We could land this as an optimization as a new bug, yes. I'm not sure it's worth the hassle here: this is only on Nightly, so let's just land it, with a test case.

::: js/src/vm/List-inl.h
@@ +17,5 @@
> +#include "vm/NativeObject-inl.h"
> +
> +namespace js {
> +
> +inline static MOZ_MUST_USE NativeObject*

Nit: remove "static" here and below.
Attachment #8897311 - Flags: review?(till) → review+
https://hg.mozilla.org/mozilla-central/rev/11b446dfdf82
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Group: javascript-core-security → core-security-release
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.