801195 - Opt-only crash with invalid read [@ js::gc::MarkInternal<JSScript>]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 90857937b601 (run with --ion-eager):


gczeal(2, 2)
evaluate("\
function f() {\
  r = arguments;\
  test();\
  yield 170;\
}\
function test() {\
  for (var i in f());\
}\
test();\
",{ newContext: true, compileAndGo: false, global: newGlobal('new-compartment') });

Comment 1

[Christian Holler (:decoder)]

This bug requires an opt-build with gczeal enabled. Also, the test is non-deterministic and requires a few runs to crash. It does not crash in GDB, and in Valgrind I see:


==16372== Invalid write of size 8
==16372==    at 0x5BEEB0: js::gc::MarkValueInternal(JSTracer*, JS::Value*) (jsval.h:719)
==16372==    by 0x54975D: js::ArgumentsObject::trace(JSTracer*, JSObject*) (ArgumentsObject.cpp:502)
==16372==    by 0x5BFA22: js::GCMarker::processMarkStackTop(js::SliceBudget&) (Marking.cpp:1255)
==16372==    by 0x5BFCAC: js::GCMarker::drainMarkStack(js::SliceBudget&) (Marking.cpp:1299)
==16372==    by 0x46E4D6: IncrementalCollectSlice(JSRuntime*, long, js::gcreason::Reason, js::JSGCInvocationKind) (jsgc.cpp:4266)
==16372==    by 0x470564: GCCycle(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:4536)
==16372==    by 0x4715EE: Collect(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:4650)
==16372==    by 0x4F3D17: _ZN2js2gc10NewGCThingI8JSStringEEPT_P9JSContextNS0_9AllocKindEm.clone.340 (jsgcinlines.h:449)
==16372==    by 0x4F5501: js_NewString(JSContext*, unsigned short*, unsigned long) (jsgcinlines.h:521)
==16372==    by 0x41A2F7: JS_NewStringCopyZ (jsapi.cpp:6004)
==16372==    by 0x45CED5: js_ErrorToException(JSContext*, char const*, JSErrorReport*, JSErrorFormatString const* (*)(void*, char const*, unsigned int), void*) (jsexn.cpp:973)
==16372==    by 0x4396E3: ReportError(JSContext*, char const*, JSErrorReport*, JSErrorFormatString const* (*)(void*, char const*, unsigned int), void*) (jscntxt.cpp:491)
==16372==  Address 0x6392a78 is 8 bytes inside a block of size 32 free'd
==16372==    at 0x4C282ED: free (vg_replace_malloc.c:366)
==16372==    by 0x4683BC: bool js::gc::Arena::finalize<JSObject>(js::FreeOp*, js::gc::AllocKind, unsigned long) (jsobjinlines.h:233)
==16372==    by 0x46CD22: bool js::gc::FinalizeTypedArenas<JSObject>(js::FreeOp*, js::gc::ArenaHeader**, js::gc::ArenaList&, js::gc::AllocKind, js::SliceBudget&) (jsgc.cpp:424)
==16372==    by 0x46DBE6: js::gc::ArenaLists::queueObjectsForSweep(js::FreeOp*) (jsgc.cpp:1665)
==16372==    by 0x46F9A2: IncrementalCollectSlice(JSRuntime*, long, js::gcreason::Reason, js::JSGCInvocationKind) (jsgc.cpp:3870)
==16372==    by 0x470564: GCCycle(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:4536)
==16372==    by 0x4715EE: Collect(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:4650)
==16372==    by 0x4F3D17: _ZN2js2gc10NewGCThingI8JSStringEEPT_P9JSContextNS0_9AllocKindEm.clone.340 (jsgcinlines.h:449)
==16372==    by 0x4F5501: js_NewString(JSContext*, unsigned short*, unsigned long) (jsgcinlines.h:521)
==16372==    by 0x41A2F7: JS_NewStringCopyZ (jsapi.cpp:6004)
==16372==    by 0x45CED5: js_ErrorToException(JSContext*, char const*, JSErrorReport*, JSErrorFormatString const* (*)(void*, char const*, unsigned int), void*) (jsexn.cpp:973)
==16372==    by 0x4396E3: ReportError(JSContext*, char const*, JSErrorReport*, JSErrorFormatString const* (*)(void*, char const*, unsigned int), void*) (jscntxt.cpp:491)


Marking sec-critical due to use-after-free.

Comment 2

[Sean Stangl [:sstangl]]

The test case in Comment 0 reproduces with --no-ion --no-jm, and, at least locally, reproduces in GDB. This is not a JIT bug.

Comment 3

[Sean Stangl [:sstangl]]

Attachment: GDB Backtrace

Backtrace from GDB, in case it's not reproducible locally for someone else.

Comment 4

[Daniel Veditz [:dveditz]]

Terrence: any progress here?

Christian: this was filed when m-c was FF19. Any idea whether this is a regression that doesn't affect FF18 or earlier (that is, would you expect your fuzzer to have found it if it were)?

Comment 5

[Christian Holler (:decoder)]

Silly me thought that bugmon wasn't handling opt-only crashes but I actually implemented that. Trying it now...

Comment 6

[Terrence Cole [:terrence]]

Sorry, I forgot to update the bug status after I looked at it.

After spending 4 hours on it, I realized it was going to need at least a full day to figure out what was going wrong.  Given that reproducing this bug requires cross-compartment references, generators, arguments, compileAndGo, *and* a GC has to happen in just the right spot (unlikely), I deprioritized this bug in favor of work more likely to have an impact.

In the meantime, a bunch of work has happened, in particular on cross-compartment wrappers.  I am not currently able to reproduce the crash.  Christian, can you confirm that the crash is gone on inbound?

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think I figured this out. I'll work on a fix next week. The relevant code looks like this:

SendToGenerator(JSContext *cx, JSGeneratorOp op, HandleObject obj,
                JSGenerator *gen, const Value &arg)
{
....
    switch (op) {
      default:
        JS_ASSERT(op == JSGENOP_CLOSE);
        cx->setPendingException(MagicValue(JS_GENERATOR_CLOSING));
        gen->state = JSGEN_CLOSING;
        break;
    }

    JSBool ok;
    {
        GeneratorFrameGuard gfg;
        if (!cx->stack.pushGeneratorFrame(cx, gen, &gfg)) {
            SetGeneratorClosed(cx, gen);
            return JS_FALSE;
        }
     ....
}

We enter this function and set the generator state to JSGEN_CLOSING. Then, when we run pushGeneratorFrame, we do a GC. This happens before the frame is pushed. The GC does not mark the generator frame because it expects that JSGEN_CLOSING frames will be on the stack. But we haven't had a chance to push this one yet. Later on, we push the frame, but we're in trouble then, because the frame's slots have been collected.

We can't just make JSGEN_CLOSING frames be markable because once the generator frame *is* on the stack it's not valid to mark it (the slots pointers are all messed up at that point). So I think the solution is to flip the state to JSGEN_CLOSING later on, after the frame has been pushed.

Comment 8

[Luke Wagner [:luke]]

Gosh, nice work.

Comment 9

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 9a6d708faf3f).

Comment 10

[Christian Holler (:decoder)]

Finally got JSBugMon to reproduce this, gczeal was missing in opt-builds. Gary, it would probably be a good idea to configure opt-builds with gczeal support, if you're not already doing :)

Comment 11

[Christian Holler (:decoder)]

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

The first good revision is:
changeset:   112849:83fe0193339b
user:        Bill McCloskey
date:        Mon Nov 05 21:09:20 2012 -0800
summary:     Bug 809295 - Do a better job handling failure in JSCompartment::wrap (r=luke)

This iteration took 128.126 seconds to run.

Comment 12

[Christian Holler (:decoder)]

Fix bisection looks ok, billm, is this a dup of the bug in comment 11?

Meanwhile I'll do a regular bisection to answer comment 4.

Comment 13

[Christian Holler (:decoder)]

Fix bisection looks ok, billm, is this a dup of the bug in comment 11?

Meanwhile I'll do a regular bisection to answer comment 4.

Comment 14

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 5242359612d2).

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Christian Holler (:decoder) from comment #10)
> Finally got JSBugMon to reproduce this, gczeal was missing in opt-builds.
> Gary, it would probably be a good idea to configure opt-builds with gczeal
> support, if you're not already doing :)

Yes, opt builds are configured w/ gczeal for me. :)

Comment 16

[Christian Holler (:decoder)]

Another bug in JSBugMon, it wouldn't bisect bugs that no longer reproduce... trying again now with the fix :)

Comment 17

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 4639da479a93).
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   107801:c500f57ba296
user:        Paul Adenot
date:        Tue Sep 18 22:23:35 2012 -0700
summary:     Bug 792274 - Don't setup mediastreams in RecreateDecodedStream() if we did not have a mediastream before. r=roc

This iteration took 122.635 seconds to run.

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

This fixes it. I don't know when this regressed. The code that sets the state has been around since generators were introduced I think (it predates the switch to Mercurial). However, it may not always have been possible to GC while pushing the generator frame. I think that's been possible since compartments were introduced.

The problem is not fixed by bug 809295, just covered up.

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

I'm looking for advice about how to land this. If I include the comment, then someone might be able to figure out that this patch is related to collecting objects that shouldn't be collected. However, it would be much harder to actually trigger the problem, since it only happens on a pretty obscure path (GC while wrapping an exception object).

If I leave out the comment, then it seems pretty unlikely to me that anyone would figure out what's going on here. However, it would be nice to have the comment.

[Security approval request comment]
How easily can the security issue be deduced from the patch?
See above.

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

Which older supported branches are affected by this flaw?
Everything back to FF4, I think.

If not all supported branches, which bug introduced the flaw?
All branches affected.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Backporting this should be easy. The code hasn't changed much at all.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely to cause regressions. We should land it on m-c first though.

Comment 20

[Daniel Veditz [:dveditz]]

(In reply to Bill McCloskey (:billm) from comment #19)
> I'm looking for advice about how to land this. If I include the comment,
> then someone might be able to figure out that this patch is related to
> collecting objects that shouldn't be collected. However, it would be much
> harder to actually trigger the problem, since it only happens on a pretty
> obscure path (GC while wrapping an exception object).
> 
> If I leave out the comment, then it seems pretty unlikely to me that anyone
> would figure out what's going on here. However, it would be nice to have the
> comment.

The comment looks fine, sec-approval+

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ef27355903e8

Comment 23

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

This is something that's unlikely to happen during normal browsing, but it is a security issue that has affected us for a long time.

I flagged for esr10 since I'm not sure if we still land things there.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): compartments (FF4)
User impact if declined: Security exploits.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None.

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

will track this for the ESR that ships with 18, since it is sec-critical, and approve accordingly.

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Please go ahead with uplift and ESR landings - see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process if you need tips on ESR landings.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fb3ea5cfb1c4
https://hg.mozilla.org/releases/mozilla-beta/rev/2c2afc063586
https://hg.mozilla.org/releases/mozilla-esr17/rev/fd165dfe63e5
https://hg.mozilla.org/releases/mozilla-esr10/rev/bed4b9ab05a2

Comment 28

[u279076]

Requesting that we get the testcase for this bug in-testsuite, if possible.