The attached testcase asserts js debug shell on m-c changeset c64a9f342156 with -m, -n and -a at Assertion failure: src->length() > 0 && chars[0] == '(', It only reproduces from ftp shell builds (e.g. http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-09-01-mozilla-central-debug/jsshell-mac64.zip ) and does not reproduce locally. May be related to bug 779215 or 783895. This testcase is intermittent (sometimes asserts half the time around count 1580) but a stacktrace with symbols is luckily available. s-s because we don't know how bad this intermittent assert is. Regression window is (seems to assert on 20120801 but not 20120731 builds): http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9d2a7a8ca1c7&tochange=582d4c67b3a7

Luke, is it possible for you to take a look? (Bug 779215 is in the regression range)

Still occurs with http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-09-10-mozilla-central-debug/jsshell-mac64.zip Flags were not needed, and on some runs the assert morphed to: Assertion failure: !!(fun->flags & 0x1000) ^ braced, I've attached a stack which is sort-of useless because of the lack of symbols.

Using https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-macosx64-debug/1348637296/jsshell-mac64.zip I now have a reduced testcase: function g() { switch (1) { default: primarySandbox = newGlobal() } return function(code) { try { evalcx(code, primarySandbox) } catch (e) {} } } function h(code) { f(code) } f = g() h("\ (s0 );\ function x() {}\ Object.defineProperty(a0, 11, {\ configurable: false,\ enumerable: true,\ get: (function() {\ var nloxtf = 0;\ return function() {\ ++nloxtf;\ if (nloxtf % 11 == 2) {\ print();\ i0.set(o1.b2, i1);\ }\ };\ })()\ });\ ") h("for(;;){gczeal(4,2);print(x)}") involving gczeal asserting at Assertion failure: src->length() > 0 && chars[0] == '(',

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-09-26-mozilla-central-debug/jsshell-mac64.zip is reproducible. That said, I was only able to reproduce this on Mac, not on Linux 64-bit, and only on tinderbox js shells.

Smaller: eval(" function x() {}" + Array(241).join(" ")); for (;;) { gczeal(4, 2); String(x); } The string being eval'd on the first line is 256 characters long; change 241 to 240 and the assertion failure does not occur.

OK, I'm able to reproduce this locally on tip by using browser/config/mozconfigs/macosx64/debug as my mozconfig and building the browser. If I add --disable-optimize, it goes away.

It would probably be better to change Rooted to be at least as strong as Anchor; but that has perf implications so I will leave it to the GC professionals.

(In reply to Jason Orendorff [:jorendorff] from comment #9) > Created attachment 665533 [details] [diff] [review] > v1 > > It would probably be better to change Rooted to be at least as strong as > Anchor; but that has perf implications so I will leave it to the GC > professionals. Any idea which bug may have been the regressor? I'm not sure the window in comment 0 is reliable, may need a double check.

Comment on attachment 665533 [details] [diff] [review] v1 Terrence says the perf impact will be negligible if I just customize Rooted<JSStableString *>. So I will do that.

The regressor is: changeset: 99940:e080642175e6 user: Benjamin Peterson <benjamin@python.org> date: Fri Jul 20 20:17:38 2012 +0200 summary: Bug 761723 - Save script sources to implement Function.prototype.toString. r=jorendorff,njn,jimb,jst,Ms2ger ...if we take ScriptSource::substring() as the code containing the bug. But my view is that the bug is a little broader than that, and might affect other JSString-using code throughout the engine. In that case there is probably not a single key regressing patch.

OK, this isn't the answer because it causes Apple clang to emit a ud2 instruction near the top of EvalKernel. A ud2 instruction always causes SIGILL. Clang bug.

This doesn't work either. It's getting to be kind of a long day.

I just got the 'volatile' in the wrong place. This works.

Comment on attachment 665706 [details] [diff] [review] v4 - works Review of attachment 665706 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/Root.h @@ +355,5 @@ > + * conservative scanning can fail. JSStableString's chars() method makes it > + * particularly attractive to use that way, so we use some voodoo to convince > + * the compiler to keep the string pointer on the stack for the full lifetime > + * of the Rooted<JSStableString *>. > + */ Nice comment. ::: js/src/jspubtd.h @@ +202,5 @@ > typedef struct JSTracer JSTracer; > > #ifdef __cplusplus > class JSFlatString; > +class JSStableString; // long story Nuh uh. We can avoid exposing JSStableString by using RootedBase. Something like: template <> class RootedBase<JSStableString*> { Anchor<JSStableString*> a; // Or equivalent hack. }; I'm ~100% sure this should work, but let me throw together a test patch to make sure I'm not sending you on a wild goose chase.

This is more what I had in mind. I don't have a Mac, but on Linux it doesn't appear to miscompile with clang 3.1.

Comment on attachment 665990 [details] [diff] [review] v5: using RootedBase. Review of attachment 665990 [details] [diff] [review]: ----------------------------------------------------------------- This doesn't trigger the SIGILL that WIP 2 did on my mac.

Comment on attachment 665990 [details] [diff] [review] v5: using RootedBase. Review of attachment 665990 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/String.h @@ +569,5 @@ > + JS::Anchor<JSString*> a; > + > + protected: > + RootedBase<JSStableString*>() { > + a = static_cast<Rooted<JSStableString*>*>(this)->get(); Wait, I don't think this actually works. get() here is accessing a field that's not initialized yet, right? The ptr field of the Rooted object?

Huh, I think you are right /and/ that it doesn't matter in this case. The Anchor's side-effecting destructor is forcing stack storage for the whole class, which is also causing the value stored in the Rooted to live for the full duration. It's as if this Anchor has another smaller anchor hanging off of it. This actually gives me an idea, the attached is basically what you had before, but using RootedBase so that we don't need to add a second class and expose JSStableString.

(In reply to Terrence Cole [:terrence] from comment #20) > /and/ that it doesn't matter in this case. It does, because you were using the value of a not-yet-constructed component of a subclass, which would have undefined behavior. In v6, should it be |volatile JSStableString*| or |JSStableString* volatile|? Intuitively I would think the latter, so that volatility is a property of the pointer, but then again, it's volatile, intuition will be wrong for it. :-) Or, why not just have a JS::Anchor<JSString*> str(stableStringPtr); inside the method? Then you can reuse the anchor mechanism and be inductively correct.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #21) > (In reply to Terrence Cole [:terrence] from comment #20) > > /and/ that it doesn't matter in this case. > > It does, because you were using the value of a not-yet-constructed component > of a subclass, which would have undefined behavior. Good point! Of course, I thought we were getting this miscompilation in the first place because aliasing the inline memory as jschar* is undefined. Is this accurate? > In v6, should it be |volatile JSStableString*| or |JSStableString* > volatile|? Intuitively I would think the latter, so that volatility is a > property of the pointer, but then again, it's volatile, intuition will be > wrong for it. :-) > > Or, why not just have a JS::Anchor<JSString*> str(stableStringPtr); inside > the method? Then you can reuse the anchor mechanism and be inductively > correct. Yeah, that would be the "best" of both worlds. I'll whip up v7.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #21) > In v6, should it be |volatile JSStableString*| or |JSStableString* > volatile|? Intuitively I would think the latter, so that volatility is a > property of the pointer, but then again, it's volatile, intuition will be > wrong for it. :-) I just literally copied from Anchor. In any case, I like this better.

(In reply to Terrence Cole [:terrence] from comment #22) > Of course, I thought we were getting this miscompilation in the first place > because aliasing the inline memory as jschar* is undefined. Is this accurate? I think C++98 3.8 paragraph 6 is the relevant spec language. Calling get() matches the "the lvalue [the pointer to the Rooted<JSStableString*>] is used to access a non-static data member or call a non-static member function of the object" case, I think. Maybe clang was smart enough to figure that out, I dunno. In any case I am not sufficiently well-versed in all the spec terminology to be 100% certain of exactly which UB is being invoked. (In reply to Terrence Cole [:terrence] from comment #23) We *did* look at assembly to verify the volatile* version that's used by everything other than gcc (going from memory) actually works, right? I would have thought pointers to volatile stuff could be passed around, dead-code eliminated, and so on til the cows come home.

(In reply to Terrence Cole [:terrence] from comment #23) > I just literally copied from Anchor. In any case, I like this better. Anchor is right, but in copying it, v6 introduced a slight error! Anchor says this: volatile T sink; sink = hold; Here 'sink' is definitely a volatile object. The trick is that if T is a pointer type X*, then volatile T sink; means X * volatile sink; // right, sink is volatile not volatile X * sink; // wrong, sink isn't volatile

Comment on attachment 668272 [details] [diff] [review] v7 Review of attachment 668272 [details] [diff] [review]: ----------------------------------------------------------------- Clearing the request flag because I think the compiler would *still* be justified in optimizing away the pointer field... it seems unlikely, but who knows? If it'd be less effort to finish removing conservative stack scanning, that's ok with me. :) ::: js/src/vm/String.h @@ +567,5 @@ > +class RootedBase<JSStableString*> > +{ > + protected: > + ~RootedBase<JSStableString*>() { > + Anchor<JSString*> str(static_cast<Rooted<JSStableString*>*>(this)->get()); Reading from already-destroyed objects triggers undefined behavior too. :-P

Comment on attachment 665706 [details] [diff] [review] v4 - works Okay then, it seems this is our best choice.

What's the security rating for this bug? Why hasn't this been marked with sec-approval?

Looks like this fell between the cracks when Jason went on paternity leave. I'll take it from here. This is a case where the GC can accidentally throw away something still in use, so there is the potential for use-after-free.

Comment on attachment 665706 [details] [diff] [review] v4 - works [Security approval request comment] How easily can the security issue be deduced from the patch? Pretty easily, if you are familiar with SpiderMonkey GC. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes. Which older supported branches are affected by this flaw? 17 and 18. If not all supported branches, which bug introduced the flaw? Source compression. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Easy to backport. How likely is this patch to cause regressions; how much testing does it need? Very unlikely to cause regressions.

What is the bug number for the source compression bug, Bug 761723?

Comment on attachment 665706 [details] [diff] [review] v4 - works Let's get this in on the affected branches.

Jason, Ryan, who is going to land this on aurora/beta? Should I?

Thanks bugzilla.

I can land it on the branches if wanted, but it needs approval first :)

(In reply to Terrence Cole [:terrence] from comment #35) > Jason, Ryan, who is going to land this on aurora/beta? Should I? You need to set the approval-aurora and approval-beta things first, and wait for approval.

(In reply to Ryan VanderMeulen from comment #33) > https://hg.mozilla.org/integration/mozilla-inbound/rev/e1c7f5195704 Should the test have been checked in?

Most sec bugs have their tests checked in once the fix has shipped in all supported releases.

(In reply to Ryan VanderMeulen from comment #40) > Most sec bugs have their tests checked in once the fix has shipped in all > supported releases. (Hit enter too soon) But not always :P

Comment on attachment 665706 [details] [diff] [review] v4 - works [Approval Request Comment] Bug caused by (feature/regressing bug #): Source compression. User impact if declined: Possible use-after-free. Testing completed (on m-c, etc.): On m-c for 4 days. Risk to taking this patch (and alternatives if risky): Low. String or UUID changes made by this patch: None.

Comment on attachment 665706 [details] [diff] [review] v4 - works [Approval Request Comment] Bug caused by (feature/regressing bug #): Source compression. User impact if declined: Possible use-after-free. Testing completed (on m-c, etc.): On m-c for 4 days. Risk to taking this patch (and alternatives if risky): Low. String or UUID changes made by this patch: None.

Comment on attachment 665706 [details] [diff] [review] v4 - works Been on central a few days, approving for uplift.

Needs landing on aurora/beta. Release drivers, please set the tracking-firefox19 flag back to +, it was unfortunately accidentally reset.

Resetting the tracking-firefox19 flag as per comment 45

Testcase was landed -> VERIFIED.

https://hg.mozilla.org/releases/mozilla-aurora/rev/9712dc86183e https://hg.mozilla.org/releases/mozilla-beta/rev/c07969cb133c

Backed out on aurora in https://hg.mozilla.org/releases/mozilla-aurora/rev/97c6c326435e - as bug 805862 and the Profiling branch predicted, it's broken for Windows PGO plus --disable-profiling.

(In reply to Phil Ringnalda (:philor) from comment #49) > Backed out on aurora in > https://hg.mozilla.org/releases/mozilla-aurora/rev/97c6c326435e - as bug > 805862 and the Profiling branch predicted, it's broken for Windows PGO plus > --disable-profiling. Well, in my defense it kind of post-dicted it, or rather portended it...

Current status: this patch is in moz-beta and moz-central but not moz-aurora. I think the simplest explanation would be that Windows PGO is just seriously broken with this code. It's hard for me to see how it could be anything in particulary to do with profiling. In my opinion, this patch certainly does not belong in beta. What kind of approval do I need to back it out? Meanwhile I am going to try a different fix.

An alternate possibility is that Ionmonkey is broken under PGO, but --enable-profiling manages to hide that - that would fit with bug 799295.

Backed out because of bug 805862... https://hg.mozilla.org/integration/mozilla-inbound/rev/df94ebb4b929

Perhaps it would be best to just solve this particular failure by manually adding the Anchor? We can always revisit the auto-anchoring in a non-security bug.

(if/when this re-lands, please merge bug 805883 into the fix here)

Comment 54 is probably the expedient solution. :-\

Here we go again.

Comment on attachment 676791 [details] [diff] [review] v8 Review of attachment 676791 [details] [diff] [review]: ----------------------------------------------------------------- Works for me. It's sad-making that we need to duplicate so much trivial code to satisfy C++, but the approach is pretty much ideal otherwise.

(In reply to Ehsan Akhgari [:ehsan] from comment #53) > Backed out because of bug 805862... > > https://hg.mozilla.org/integration/mozilla-inbound/rev/df94ebb4b929 Merged to m-c: https://hg.mozilla.org/mozilla-central/rev/df94ebb4b929

Took a while to get a green try run. Finally got one today. https://hg.mozilla.org/integration/mozilla-inbound/rev/7c56dd925c25

Can this bug be marked verified now that we have in-testsuite+?

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #62) > Can this bug be marked verified now that we have in-testsuite+? Yes. This may also need re-landing on the aurora branch.

(In reply to Gary Kwong [:gkw, :nth10sd] from comment #63) > (In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #62) > > Can this bug be marked verified now that we have in-testsuite+? > > Yes. Assuming the same for Beta, please correct me if this is incorrect.

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #64) > (In reply to Gary Kwong [:gkw, :nth10sd] from comment #63) > > (In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #62) > > > Can this bug be marked verified now that we have in-testsuite+? > > > > Yes. > > Assuming the same for Beta, please correct me if this is incorrect. It's probably better to re-verify this for beta since (1) it is not yet landed on aurora and (2) 17 is the upcoming ESR.

Good point, Gary. Adding this to our queue.

Setting checkin-needed for aurora.

Erm, wait, jumped the gun here. v8 needs nomination for aurora approval first.

Comment on attachment 676791 [details] [diff] [review] v8 Yes, we should land this on aurora and beta. [Approval Request Comment] Bug caused by (feature/regressing bug #): Source compression. User impact if declined: Possible use-after-free. Testing completed (on m-c, etc.): On m-c for 9 days. Risk to taking this patch (and alternatives if risky): Low. String or UUID changes made by this patch: None.

Comment on attachment 676791 [details] [diff] [review] v8 only needed for bustage fixes on aurora, clearing the beta flag (confirmed with jorendorff in IRC).

approval obtained - setting checkin-needed for aurora.