Closed Bug 1511538 Opened 2 years ago Closed 2 years ago

Assertion failure: s_.payload_.why_ == why, at dist/include/js/Value.h:689 with arguments

Categories

(Core :: JavaScript Engine, defect, P1)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 + verified
firefox66 + verified

People

(Reporter: decoder, Assigned: nbp, NeedInfo)

References

Details

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

Attachments

(1 file)

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

function testSingleObject() {
    do {} while (!inIon());
    var f2 = function() {};
    do {
        arguments.length;
    } while (!inIon());
}
testSingleObject();


Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  JS::Value::isMagic (why=JS_OPTIMIZED_ARGUMENTS, this=<optimized out>) at dist/include/js/Value.h:689
#1  js::WrappedPtrOperations<JS::Value, JS::Handle<JS::Value> >::isMagic (why=JS_OPTIMIZED_ARGUMENTS, this=0x7fffffffc530) at dist/include/js/Value.h:1303
#2  js::jit::GetPropIRGenerator::tryAttachMagicArgumentsName (this=this@entry=0x7fffffffc380, valId=valId@entry=..., id=id@entry=...) at js/src/jit/CacheIR.cpp:2071
#3  0x000055555616997b in js::jit::GetPropIRGenerator::tryAttachStub (this=this@entry=0x7fffffffc380) at js/src/jit/CacheIR.cpp:315
#4  0x0000555556043002 in js::jit::DoGetPropFallback (cx=<optimized out>, frame=0x7fffffffc608, stub=0x7ffff49241f8, val=..., res=...) at js/src/jit/BaselineIC.cpp:2867
#5  0x000017a167c1952f in ?? ()
[...]
#19 0x0000000000000000 in ?? ()
rax	0x555557b6e480	93825032184960
rbx	0x7fffffffc380	140737488339840
rcx	0x555556a75350	93825014387536
rdx	0x0	0
rsi	0x7ffff6eeb770	140737336227696
rdi	0x7ffff6eea540	140737336223040
rbp	0x7fffffffc1c0	140737488339392
rsp	0x7fffffffc1a0	140737488339360
r8	0x7ffff6eeb770	140737336227696
r9	0x7ffff7fe6cc0	140737354034368
r10	0x58	88
r11	0x7ffff6b927a0	140737332717472
r12	0x7fffffffc220	140737488339488
r13	0x0	0
r14	0x7fffffffc380	140737488339840
r15	0x1	1
rip	0x55555615a839 <js::jit::GetPropIRGenerator::tryAttachMagicArgumentsName(js::jit::ValOperandId, JS::Handle<JS::PropertyKey>)+473>
=> 0x55555615a839 <js::jit::GetPropIRGenerator::tryAttachMagicArgumentsName(js::jit::ValOperandId, JS::Handle<JS::PropertyKey>)+473>:	movl   $0x0,0x0
   0x55555615a844 <js::jit::GetPropIRGenerator::tryAttachMagicArgumentsName(js::jit::ValOperandId, JS::Handle<JS::PropertyKey>)+484>:	ud2


Not sure what is going on here, but JIT + arguments is rarely a good sign and this assertion was previously involved in s-s issues.
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/c0bef417dc8e
user:        Nicolas B. Pierron
date:        Fri Oct 05 18:35:19 2018 +0200
summary:     Bug 1489142 - Rewrite FlagPhiInputsAsHavingRemovedUses to iterate at most once per Phi. r=jandem

This iteration took 383.767 seconds to run.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Flags: needinfo?(nicolas.b.pierron)
Priority: -- → P1
sec-high because it kinda looks like type confusion bugs we've had in the past -- but please correct (or clear) if we're guessing wrong!
Blocks: 1489142
Keywords: sec-high
Any news on this, nbp?
The problem seems to be that we generated a MArgumentsLength instruction when compiling with IonMonkey, but MArgumentsLength does not depend on MArguments. However, when we bailout we do depend on it and we bailout with the arguments fields optimized like any other variables instead of being optimized like the arguments field optimization case.

I suspect the same issue will happen with catch block that we are not even considering in IonBuilder.

Therefore we should just prevent the replacement of optimized-arguments by the generic optimized-out.
Ok, one of the weirdness seems to be that the lazy-args is not stored in the reserved Argument object slot, but in one of the local slots. No idea why this issue would only appear now, unless we did not properly optimized this case before.
Ok MArgumentsLength does not use MArguments input, but all cases in IonBuilder
do properly set the ImplicitlyUse flag on the object which are representing the
MArguments input.

The error was that FlagPhiInputsAsHavingRemovedUses was not checking the value
of ImplicitlyUsed flags. Which is fixed in this patch.
Attachment #9034186 - Flags: review?(jdemooij)
Assignee: nobody → nicolas.b.pierron

Comment on attachment 9034186 [details] [diff] [review]
Check implicitly used flags when flagging phis for removed uses.

Review of attachment 9034186 [details] [diff] [review]:

Sorry for the delay :/

Let's try to fix this in 65 so we don't ship the regression.

::: js/src/jit/IonAnalysis.cpp
@@ +221,3 @@

 // between the |block| and its successor |succ|.
 MDefinition* def = phi->getOperand(predIndex);
 if (def->isUseRemoved()) {

Do we also need to check isImplicitlyUsed here?

Attachment #9034186 - Flags: review?(jdemooij) → review+

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

Comment on attachment 9034186 [details] [diff] [review]
Check implicitly used flags when flagging phis for removed uses.

Review of attachment 9034186 [details] [diff] [review]:

Sorry for the delay :/

Let's try to fix this in 65 so we don't ship the regression.

::: js/src/jit/IonAnalysis.cpp
@@ +221,3 @@

 // between the |block| and its successor |succ|.
 MDefinition* def = phi->getOperand(predIndex);
 if (def->isUseRemoved()) {

Do we also need to check isImplicitlyUsed here?

Good catch!
I do not think it is mandatory, it would just reduce the amount of work by skipping the rest of the loop.

Flags: needinfo?(nicolas.b.pierron)

Comment on attachment 9034186 [details] [diff] [review]
Check implicitly used flags when flagging phis for removed uses.

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: I do not know if it can be exploited, as I would expect blew up on some assertions later on.
However, I think it would likely cause crashes in the wild, even without deliberate intent to exploit this crashes.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown

Which older supported branches are affected by this flaw?: fx65

If not all supported branches, which bug introduced the flaw?: Bug 1489142

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: This is the same patch.

How likely is this patch to cause regressions; how much testing does it need?: It needs to pass the test suite, plus the test from this bug.

Attachment #9034186 - Flags: sec-approval?

Comment on attachment 9034186 [details] [diff] [review]
Check implicitly used flags when flagging phis for removed uses.

sec-approval+. Please nominate a beta patch to go in as well as soon as you can.

Attachment #9034186 - Flags: sec-approval? → sec-approval+

https://hg.mozilla.org/integration/mozilla-inbound/rev/a861fff5d00c71af2719e40b6cc512f5af5bc598

As already noted, this grafts cleanly to Beta as-is. Please nominate it for Beta approval when you get a chance.

Flags: needinfo?(nicolas.b.pierron)

(In reply to Nicolas B. Pierron [:nbp] from comment #8)

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

::: js/src/jit/IonAnalysis.cpp
@@ +221,3 @@

 // between the |block| and its successor |succ|.
 MDefinition* def = phi->getOperand(predIndex);
 if (def->isUseRemoved()) {

Do we also need to check isImplicitlyUsed here?

Good catch!
I do not think it is mandatory, it would just reduce the amount of work by skipping the rest of the loop.

I will open a follow-up bug as the patch landed without this modification. :/
No big deal.

Comment on attachment 9034186 [details] [diff] [review]
Check implicitly used flags when flagging phis for removed uses.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1489142

User impact if declined: Potentially some crashes in the wild. (not sure if they could be made exploitable, my uncertain guess would be no)

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce:

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The change is not risky as it prevents code from being removed incorrectly, and thus replaced by optimized-out magic value.

String changes made/needed: N/A

Flags: needinfo?(nicolas.b.pierron)
Attachment #9034186 - Flags: approval-mozilla-beta?
Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Comment on attachment 9034186 [details] [diff] [review]
Check implicitly used flags when flagging phis for removed uses.

[Triage Comment]
Fixes some maybe-exploitable crashes. Approved for 65.0b10.

Attachment #9034186 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.

Ni? my-self for landing the test case later.

Flags: needinfo?(nicolas.b.pierron)
JSBugMon: This bug has been automatically verified fixed on Fx65
Duplicate of this bug: 1514835
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.