Closed Bug 955850 Opened 6 years ago Closed 6 years ago

Assertion failure: safepoint->hasNunboxPayload(alloc), at jit/RegisterAllocator.cpp

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla29
Tracking Status
firefox26 --- unaffected
firefox27 + fixed
firefox28 + fixed
firefox29 + fixed
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed

People

(Reporter: gkw, Assigned: jandem)

References

(Blocks 2 open bugs)

Details

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

Attachments

(5 files)

Attached file stack
function testMathyFunction(f, inputs) {
    for (var j = 0; j < 99; ++j) {
        for (var k = 0; k < 99; ++k) {
            try {
                f(inputs[j], inputs[j]);
            } catch (e) {}
        }
    }
}
Function("\
    mathy0 = (function() {\
        var atan2 = Math.atan2;\
        Uint8ArrayView = Uint8Array();\
        function f(d, d) {\
            d = Uint8ArrayView[6]((0 != (0 > d))(atan2(Infinity, 1)));\
            arguments\
        }\
        return f\
    })();\
    testMathyFunction(mathy0, [1, 2, 0, -Number, 1, 0, 1, 0, -Number, 0, 0])\
")()

asserts js debug shell on m-c changeset 29882306e8ca without any CLI arguments at Assertion failure: safepoint->hasNunboxPayload(alloc), at jit/RegisterAllocator.cpp

My configure flags are:

CC="gcc -m32" AR=ar PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig CXX="g++ -m32" sh ./configure --target=i686-pc-linux --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/9f2a7e76e2e2
parent:      ee7541e01eb1
user:        Kannan Vijayan
date:        Fri Dec 20 18:11:21 2013 -0500
summary:     Bug 951528. r=jandem

Kannan, is bug 951528 a likely regressor? (s-s because that bug is s-s)
Flags: needinfo?(kvijayan)
It's likely the regressor, but it should be incidental.  The patch doesn't modify any core regalloc behaviour, aside from turning off spill-at-use for certain definitions.  This is likely an existing bug that has been exposed, but I'll look into it and confirm.
Flags: needinfo?(kvijayan)
also found via bughunter on http://mariontrent.com/radio-spots/ and reported for all branches and OS. I was able to reproduce the problem on a win7 debug trunk build
Blocks: 532972
Duplicate of this bug: 951970
Assignee: nobody → kvijayan
Please transfer tracking flags if you mark bugs duplicate!
Sorry Boris!  Thanks for the heads up on that.
I need to learn more about regalloc to fix this bug.  I think I sort of know why this is happening, but not how.  Taking notes in bug as we go.

For reference, the compile that fails is for the following function:

    function f(d, d) {
        d = Uint8ArrayView[6]((0 != (0 > d))(atan2(Infinity, 1)));
        arguments
    }

Note that the first |d| argument is shadowed by the second |d| argument.  The issue seems to be that the first |d| is not given a proper allocation in the stack slot.  More to come.
The assert itself triggers when checking consistency for type-barrier Unbox instruction 22 in the LIR dump (https://bugzilla.mozilla.org/attachment.cgi?id=8356144).

We check the integrity of the 12th input of the instruction.  The instruction itself has 2 inputs, and the remaining come from its snapshot, so this is the 10th entry in the snapshot.

The 'oldInput' for this entry has vreg 4.  The current allocation for this input is as a stack slot.  Note that vreg 4 comes from the first parameter (the shadowed |d| argument).

The next prior instruction which has a safepoint is the CheckOverRecrused instruction 6.  The safepoint for CheckOverRecursed doesn't have an allocation for vreg 4.

I suspect that somehow, vreg 4 is not getting an allocation because it's not used anywhere else, and it's not getting marked spillAtUse because we refrain from marking things spillAtUse because this function contains a JSOP_SETARG and also an arguments reference.

Brian, does this synposis sound right to you?
Flags: needinfo?(bhackett1024)
Brian is out so I can take a look. The spill to stack:8 happens before CheckOverRecursed, and the use of this slot by the Unbox happens after CheckOverRecursed, so the verifier is rightly complaining about this slot not being in the safepoint.

Not sure why we're not adding it, looking..
Attached patch PatchSplinter Review
The problem is that populateSafepoints() does nothing if payloadAlloc->isArgument(). This used to be correct, but now it's possible to have a stack slot as canonical spill slot and if it's spill-at-definition, we have to add this stack slot to the safepoints (see isSpilledAt).
Assignee: kvijayan → jdemooij
Status: NEW → ASSIGNED
Attachment #8356562 - Flags: review?(kvijayan)
Comment on attachment 8356562 [details] [diff] [review]
Patch

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

Verified to fix testcase.
Attachment #8356562 - Flags: review?(kvijayan) → review+
Comment on attachment 8356562 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not easily; maybe for somebody familiar with the engine.

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

Which older supported branches are affected by this flaw?
27+.

If not all supported branches, which bug introduced the flaw?
Bug 951528.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be easy.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely.
Attachment #8356562 - Flags: sec-approval?
Flags: needinfo?(bhackett1024)
Comment on attachment 8356562 [details] [diff] [review]
Patch

sec-approval+ for trunk. Please prepare branch patches and nominate them after it goes into trunk.
Attachment #8356562 - Flags: sec-approval? → sec-approval+
Comment on attachment 8356562 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 951528.
User impact if declined: Crashes, security issues.
Testing completed (on m-c, etc.): On m-i.
Risk to taking this patch (and alternatives if risky): Low risk.
String or IDL/UUID changes made by this patch: None.
Attachment #8356562 - Flags: approval-mozilla-beta?
Attachment #8356562 - Flags: approval-mozilla-aurora?
Attachment #8356562 - Flags: approval-mozilla-beta?
Attachment #8356562 - Flags: approval-mozilla-beta+
Attachment #8356562 - Flags: approval-mozilla-aurora?
Attachment #8356562 - Flags: approval-mozilla-aurora+
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #17)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/ab13ac48d6dc
> https://hg.mozilla.org/releases/mozilla-beta/rev/46cd787239b7

fwiw, this patch hasn't yet seemed to land on m-c...
Well aware, but I also looked at the tree and saw that it passed tests and will on the next merge...
https://hg.mozilla.org/mozilla-central/rev/9aba403595d5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Depends on: 958432
Group: core-security
You need to log in before you can comment on or make changes to this bug.