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') });

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.

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.

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

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)?

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

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?

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.

Gosh, nice work.

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

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 :)

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.

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.

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.

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

(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. :)

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

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.

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 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.

(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+

JSBugMon: This bug has been automatically verified fixed.

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.

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

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.

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

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