Closed Bug 925308 Opened 6 years ago Closed 6 years ago

Assertion failure: *(int*)size_ >= 0, at vm/SPSProfiler.cpp:196

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox25 --- wontfix
firefox26 --- wontfix
firefox27 --- fixed
firefox28 --- fixed
firefox-esr24 --- wontfix
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- wontfix
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed

People

(Reporter: decoder, Assigned: djvj)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, sec-moderate, testcase, Whiteboard: [fuzzblocker] [jsbugmon:update,ignore][adv-main27+])

Attachments

(2 files, 1 obsolete file)

The following testcase asserts on mozilla-central revision aa986b6ce882 (run with --fuzzing-safe --ion-eager):


var lfcode = new Array();
lfcode.push("3");
lfcode.push("enableSPSProfilingAssertions(false);foo();");
while (true) {
  var file = lfcode.shift(); if (file == undefined) { break; }
  loadFile(file)
}
function loadFile(lfVarx) {
    if (lfVarx.substr(-3) != ".js" && lfVarx.length != 1) {
        switch (lfRunTypeId) {
            default: function newFunc(x) { new Function(x)(); }; newFunc(lfVarx); break;
        }
    } else if (!isNaN(lfVarx)) {
        lfRunTypeId = parseInt(lfVarx);
    }
}
Whiteboard: [jsbugmon:update,bisect]
This is causing quite a few signatures in the fuzzer, including a critical heap crash. Marking as fuzzblocker.
Flags: needinfo?(kvijayan)
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][fuzzblocker]
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/88a218a4b5bf
user:        Jan de Mooij
date:        Tue Dec 25 16:12:59 2012 +0100
summary:     Bug 764310 part 2 - Implement JSOP_DEFFUN in IonMonkey. r=bhackett

This iteration took 144.623 seconds to run.
Seeing this on beta as well.
Marking sec-moderate because it looks like it requires SPS, but it still sounds pretty bad...
Keywords: sec-moderate
Attachment #815316 - Attachment is obsolete: true
Whiteboard: [fuzzblocker] [jsbugmon:update] → [fuzzblocker] [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 9f8233fcce1d).
I filed this before and it stopped reproducing. Please investigate this on the original revision and fix it if required ,thanks.
Gak!  This is a hairy issue.  Here's what happens:

1. We compile Ion code before SPS is enabled, enter the compiled Ion code, and do a VM-call to InvokeFunction a JS function.
2. Inside the InvokeFunction, code executes which turns profiling on, invalidating the original compiled IonCode.
3. Before returning, though.. an exception is raised.  This exception is handled by the VM-call's trampoline, which occurs BEFORE the patched OSI-point for the invalidated IonCode is hit.  It sees the current profiling state as "on" and tries to pop an SPS frame (which doesn't exist, since it was pushed by non-SPS enabled Ion code that has since been invalidated).

Jan, I need to talk with you about potential solutions to this.  I have the beginnings of one sketched out, but I'd like to run it by you.
After discussing this with :jandem, he suggested maybe changing probes::ExitScript to take a ScriptFrameIter instead of an AbstractFramePtr, and thread the APIs for ScriptFrameIter such that the SPS information was available from it.

Thinking more about it, I realized that the only thing probes::ExitScript uses from the frame is the flag on whether it needs to pop an SPS frame or not.  It seemed a lot easier and lightweight to have the callsite pass in just that flag, so that's what I did.
Flags: needinfo?(kvijayan)
See comment 10 for description of fix.

Confirmed that it worked on the bug on the revision where bug was first discovered.  Bug no longer reproduces on tip, so I'm not sure if I should include the test case or not.
Attachment #828891 - Flags: review?(jdemooij)
> Confirmed that it worked on the bug on the revision where bug was first
> discovered.  Bug no longer reproduces on tip, so I'm not sure if I should
> include the test case or not.

Yes, we should, in case it reappears again.
Noted.  Added test case to jit-tests.
Comment on attachment 828891 [details] [diff] [review]
sps-bug-925308.patch

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

Nice.

::: js/src/jit/IonFrames.cpp
@@ +552,5 @@
>              for (;;) {
>                  HandleExceptionIon(cx, frames, rfe, &overrecursed);
>  
> +                IonScript *ionScript = nullptr;
> +                bool invalidated = iter.checkInvalidation(&ionScript);

We can move these two lines out of this loop, since |invalidated| will have the same value for all inlined frames.

Then we can change these lines after the loop:

    IonScript *ionScript = nullptr;
    if (iter.checkInvalidation(&ionScript))
        ionScript->decref(cx->runtime()->defaultFreeOp());

To:

    if (invalidated)
        ionScript->decref(cx->runtime()->defaultFreeOp());

r=me with that.
Attachment #828891 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/34001410d373
Assignee: general → kvijayan
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Please land the test.
Flags: in-testsuite?
Sorry, forgot to 'hg add' the file.

https://hg.mozilla.org/integration/mozilla-inbound/rev/85e15ff221ca
Flags: in-testsuite? → in-testsuite+
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
For now this sec-moderate is a wontfix, unless others would like to nominate for ESR24 approval. Unclear that this is desirable on the ESR branch.
Should this be uplifted to Fx27 still? What about b2g18* or b2g26?
Flags: needinfo?(kvijayan)
27 should get it. ESR has a different bar.
Comment on attachment 828891 [details] [diff] [review]
sps-bug-925308.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Hard to identify.  Probably there since profiler instrumentation.

User impact if declined: 
Potential for bad memory writes at fixed address, leading to crash.

Testing completed (on m-c, etc.): 
3-weeks green on m-c.

Risk to taking this patch (and alternatives if risky):
Low.
 
String or IDL/UUID changes made by this patch:
N/A
Attachment #828891 - Flags: approval-mozilla-aurora?
Flags: needinfo?(kvijayan)
Attachment #828891 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/37df29dd2f8f

I'm guessing b2g18 is wontfix for this, but what about b2g26?
Flags: needinfo?(praghunath)
Keywords: checkin-needed
Kannan, is this worth trying to uplift to the B2G branches?
Flags: needinfo?(praghunath) → needinfo?(kvijayan)
I'd guess no.  This is a rare corner case that only hits when profiling is switched on or off in very particular ways, deep within a call stack.
Flags: needinfo?(kvijayan)
Whiteboard: [fuzzblocker] [jsbugmon:update,ignore] → [fuzzblocker] [jsbugmon:update,ignore][adv-main27+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.