842305 - IonMonkey: Assertion failure: v->toGCThing(), at gc/Marking.cpp:470 or Crash [@ zone]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 0acbd06d48a9 (no options required):


function shapelessUnknownCalleeLoop(n, f, g, h, a) {
  for (var i = 0; i < 32000, g = new Function("ABCDEFGHIJK", "1234"); i++)
    g = h;
}
function shapelessCalleeTest() {
  var a = [];
  helper = function (i, a) a[30 + i] = i;
  shapelessUnknownCalleeLoop(null, helper, helper, function (i, a) a[30 + i] = -i, a);
}
shapelessCalleeTest()

Comment 1

[Christian Holler (:decoder)]

Debug trace:

Program received signal SIGSEGV, Segmentation fault.
0x085e578d in MarkValueInternal (trc=0x8cc5f1c, v=0xffff8af4) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:470
470             JS_ASSERT(v->toGCThing());
(gdb) bt
#0  0x085e578d in MarkValueInternal (trc=0x8cc5f1c, v=0xffff8af4) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:470
#1  0x085e63a8 in js::gc::MarkValueRoot (trc=0x8cc5f1c, v=0xffff8af4, name=0x8a9e2ea "ion-argv") at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:514
#2  0x087e960d in MarkIonJSFrame (trc=0x8cc5f1c, frame=...) at /srv/repos/mozilla-central/js/src/ion/IonFrames.cpp:479
#3  0x087ea051 in MarkIonActivation (trc=0x8cc5f1c, activations=...) at /srv/repos/mozilla-central/js/src/ion/IonFrames.cpp:657
#4  0x087ea17a in js::ion::MarkIonActivations (rt=0x8cc5d38, trc=0x8cc5f1c) at /srv/repos/mozilla-central/js/src/ion/IonFrames.cpp:681
#5  0x085df3c3 in js::gc::MarkRuntime (trc=0x8cc5f1c, useSavedRoots=false) at /srv/repos/mozilla-central/js/src/gc/RootMarking.cpp:791
#6  0x081cac42 in BeginMarkPhase (rt=0x8cc5d38) at /srv/repos/mozilla-central/js/src/jsgc.cpp:2782
#7  0x081d2071 in IncrementalCollectSlice (rt=0x8cc5d38, budget=0, reason=JS::gcreason::TOO_MUCH_MALLOC, gckind=js::GC_NORMAL) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4184
#8  0x081d287f in GCCycle (rt=0x8cc5d38, incremental=true, budget=0, gckind=js::GC_NORMAL, reason=JS::gcreason::TOO_MUCH_MALLOC) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4369
#9  0x081d2d68 in Collect (rt=0x8cc5d38, incremental=true, budget=0, gckind=js::GC_NORMAL, reason=JS::gcreason::TOO_MUCH_MALLOC) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4494
#10 0x081d3059 in js::GCSlice (rt=0x8cc5d38, gckind=js::GC_NORMAL, reason=JS::gcreason::TOO_MUCH_MALLOC, millis=0) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4532
#11 0x0812ee34 in js_InvokeOperationCallback (cx=0x8cec7e0) at /srv/repos/mozilla-central/js/src/jscntxt.cpp:1135
#12 0x0812ee9d in js_HandleExecutionInterrupt (cx=0x8cec7e0) at /srv/repos/mozilla-central/js/src/jscntxt.cpp:1159
#13 0x088b5865 in js::ion::InterruptCheck (cx=0x8cec7e0) at /srv/repos/mozilla-central/js/src/ion/VMFunctions.cpp:424
#14 0xf770c920 in ?? ()
Cannot access memory at address 0xffffff8b
(gdb)

S-s due to GC-relatedness.

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   115546:8275b86c0b62
user:        Brian Hackett
date:        Mon Dec 10 12:02:31 2012 -0700
summary:     Remove bytecode uses analysis, keep track of SSA values that were folded away when building MIR, bug 818869. r=jandem

This iteration took 94.700 seconds to run.

Comment 3

[Christian Holler (:decoder)]

Needinfo from Brian based on comment 2.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch

Really just a continuation of bug 830042 (and should have realized this and fixed it at that point).  We tolerate JM leaving object/string values with null payloads on the stack, but if these flow back to Ion code then marking will assume they are valid GC things.  Just tolerating the null payloads during Ion marking is difficult as values can be split apart on the stack, but there is only a single place where values flow from the interpreter into Ion so it is easier to fix there.  This can all be ripped out when JM is removed.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 716086 [details] [diff] [review]
patch

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

::: js/src/vm/Stack.cpp
@@ +252,5 @@
> +void
> +StackFrame::cleanupTornValues()
> +{
> +    for (size_t i = 0; i < numFormalArgs(); i++)
> +        CleanupTornValue(this, &formals()[i]);

If numActuals >= numFormals, the extra arguments don't need cleanup? And stack values >= nfixed?

Comment 6

[Brian Hackett [Laid off!]]

Only values in slots which the method JIT computes SSA information for need to be tracked.  Arguments above nformals will only be written by mjit code via the arguments object, and the only things that can be on the stack at OSR points are iterator objects, which will not be dead (other values in scripts not Ion compiled could also be there, but are also not tracked by the mjit's SSA).

Comment 7

[Brian Hackett [Laid off!]]

Comment on attachment 716086 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

not at all. this isn't exploitable except as a null deref

Which older supported branches are affected by this flaw?

only aurora/beta are affected

How likely is this patch to cause regressions; how much testing does it need?

tbpl is fine

Comment 8

[Brian Hackett [Laid off!]]

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

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 716086 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 818869
User impact if declined: potential null deref
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): none

Comment 10

[Brian Hackett [Laid off!]]

Followup fix to move the cleanupTornValues() call to the correct place.

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f815f2ba316a
https://hg.mozilla.org/mozilla-central/rev/dc30947a2d26

Comment 12

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 13

[bhavana bajaj [:bajaj]]

Comment on attachment 716086 [details] [diff] [review]
patch

low risk patch for a sec-high regression,approving in aurora & beta

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ca928196a14
https://hg.mozilla.org/releases/mozilla-beta/rev/d548d3afbaf5

FWIW, I qfolded the follow-up fix in with the original.