Closed Bug 1369774 Opened 7 years ago Closed 7 years ago

Assertion failure: types, at js/src/jit/SharedIC.cpp:2560 with TypedObject

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 + verified
firefox56 --- verified

People

(Reporter: decoder, Unassigned)

References

Details

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

Attachments

(1 file)

The following testcase crashes on mozilla-central revision aeb3d0ca558f (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --baseline-eager --ion-offthread-compile=off):

var T = TypedObject;
var ObjectStruct = new T.StructType({
    f: T.Object
});
var o = new ObjectStruct();
function testGC(o, p) {
    for (var i = 0; i < 5; i++) {
        o.f = p;
    }
}
testGC(o, null);


Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x000000000080a2f9 in js::jit::ICUpdatedStub::addUpdateStubForValue (this=this@entry=0x7ffff434c3c0, cx=cx@entry=0x7ffff6924000, outerScript=..., outerScript@entry=..., obj=..., obj@entry=..., id=id@entry=..., val=val@entry=...) at js/src/jit/SharedIC.cpp:2560
#0  0x000000000080a2f9 in js::jit::ICUpdatedStub::addUpdateStubForValue (this=this@entry=0x7ffff434c3c0, cx=cx@entry=0x7ffff6924000, outerScript=..., outerScript@entry=..., obj=..., obj@entry=..., id=id@entry=..., val=val@entry=...) at js/src/jit/SharedIC.cpp:2560
#1  0x00000000005edc3e in js::jit::DoTypeUpdateFallback (cx=0x7ffff6924000, frame=<optimized out>, stub=0x7ffff434c3c0, objval=..., value=...) at js/src/jit/BaselineIC.cpp:331
#2  0x000012bf8da155e5 in ?? ()
[...]
#43 0x00007fffffffc640 in ?? ()
#44 0x00000000005e5fd2 in EnterBaseline (cx=0x7fffffffc268, data=...) at js/src/jit/BaselineJIT.cpp:162
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
rax	0x0	0
rbx	0x0	0
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffc0c0	140737488339136
rsp	0x7fffffffbfc0	140737488338880
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7ffff6924000	140737330167808
r13	0x7ffff434c3c0	140737290486720
r14	0x7fffffffc220	140737488339488
r15	0x7fffffffc160	140737488339296
rip	0x80a2f9 <js::jit::ICUpdatedStub::addUpdateStubForValue(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>)+1961>
=> 0x80a2f9 <js::jit::ICUpdatedStub::addUpdateStubForValue(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>)+1961>:	movl   $0x0,0x0
   0x80a304 <js::jit::ICUpdatedStub::addUpdateStubForValue(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>)+1972>:	ud2    


Marking s-s because it's an IC assert and involves TypedObject.
Flags: needinfo?(jdemooij)
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/ba5cee986655
user:        Jan de Mooij
date:        Wed May 31 09:43:16 2017 +0200
summary:     Bug 1368509 part 2 - Generalize Baseline unknown/unknownObject type update stubs. r=tcampbell

This iteration took 269.181 seconds to run.
tracking as regression in 55.
Sounds like a type confusion, so I will mark it sec-high.
Keywords: sec-high
Flags: needinfo?(jdemooij)
Attached patch PatchSplinter Review
In addUpdateStubForValue we assumed we always had a HeapTypeSet if the group doesn't have unknownProperties(), but unfortunately that's not true for TypedObjects - see DoTypeUpdateFallback. So this patch just restructures the code a bit to deal with this.

TypedObjects are still Nightly only so this doesn't affect 55 (but we should probably backport it anyway to help fuzzing).
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8878543 - Flags: review?(tcampbell)
Comment on attachment 8878543 [details] [diff] [review]
Patch

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

Your solution makes sense to me.

::: js/src/jit/SharedIC.cpp
@@ +2565,5 @@
> +            // We don't record null/undefined types for certain TypedObject
> +            // properties. See DoTypeUpdateFallback.
> +            MOZ_ASSERT(obj->is<TypedObject>());
> +            MOZ_ASSERT(val.isNullOrUndefined());
> +        }

This took me a few reads to understand that the Null/Undef case is handled as normal primitive types.

Maybe add a sentence in the comment that in these |types| is allowed to be nullptr in this case without implying unknown types.

(Your comment in the bug explained well, but the patch was harder to follow)
Attachment #8878543 - Flags: review?(tcampbell) → review+
Comment on attachment 8878543 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1368509.
[User impact if declined]: This only affects TypedObjects which are Nightly-only. However I'd like to have this fixed on beta, as I have more patches for this code and it's better to use the same code everywhere.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Patch is pretty straight-forward, very local change.
[String changes made/needed]: None.
Attachment #8878543 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/140285b81cd3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Comment on attachment 8878543 [details] [diff] [review]
Patch

sec-high fix, with test, beta55+
Attachment #8878543 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: core-security-release
JSBugMon: This bug has been automatically verified fixed on Fx55
Assignee: jdemooij → nobody
Keywords: bugmon
You need to log in before you can comment on or make changes to this bug.