bz found a real live hazard in the wild that the analysis should catch but does not. See bug 1009675. Consider: Value foo() { CallSetup s; return bar(); } where CallSetup::~CallSetup can gc. The relevant bit of the CFG is: pentry: 1 pexit: 14 Call(11,12, __temp_3 := this*.Call(__temp_4*,__temp_5*,__temp_6*,aRv*)) Assign(12,13, return := __temp_3*) Call(13,14, s.~CallSetup()) The problem is that __temp_3 gets stored into a magical 'return' variable, which is never considered to be used, and only then does ~CallSetup get invoked. Given that we always have a single unique exit point (14, here), it seems like I should be able to just say that the exit point implicitly uses 'return'. (Or at least, I'm assuming from the existence of a 'pexit' field that there's always a single unique exit point.)

Treat the final point in a function as using the return value.

Comment on attachment 8444620 [details] [diff] [review] Include return values in analysis Doh! Sorry bhackett, this patch is wrong. (Nice and simple, but wrong.) I have later changes that fix it -- which endpoint of the return value edge you want is different for this "last edge uses the return value" case vs the normal case. Cancelling review request.

Ok, here's the proper patch. The full description of the tricky part is in a comment.

Shouldn't really be in this same bug, but I introduced another hole in the analysis when I switched to matching mangled names. When gcc generates a "not in charge" constructor/destructor, and it is identical to the "in charge" body, then it only generates the code for the "not in charge" variant and somehow aliases the "in charge" variant to it. But the call graph will only show the "in charge" version, since the analysis knows nothing about the aliasing. That loses a call edge that is very important when you have an RAII destructor that can GC. I fix it up by pretending that the "in charge" version is a simple stub function that calls the "not in charge" version. To be fully precise, I really shouldn't do this in the cases where they *are* different and therefore both get emitted, but that's a PITA and very probably irrelevant, so I'm just making it safe and conservative.

I keep putting this stuff back in manually. I think I'll leave it there. r=me.

An annoying source of false positives with the return value is the pattern RAII_GC foo; if (!SomeOperation()) return nullptr; because this is putting a JSObject* (nullptr) on the stack, then running a destructor that could GC. But as long as we don't relocate NULL, we're safe. :-) It turns out to be annoying to restructure things to avoid these. If necessary, I'll extend this to cover |UndefinedValue()| and |NullValue()|, but those may not be so easy to pattern match so I'm not going to bother until I know they're needed.

Continuing on the pattern of piling unrelated crap into this bug, here's another set of changes that I made to the analysis while working on this stuff. It adds some entries to the CFG dumps in gcFunctions.txt that make it a lot easier to understand what's going on. (Right now, it's kind of missing the critical endpoints, so it all just seems like irrelevant noise.)

I meant to upload this first, but this is the interesting patch that adds the return value handling.

The hazards here are in the JS engine, but I had to update several things in Gecko. The changes are pretty trivial, but people might have opinions on error handling. (I could also do this entirely internal to the engine, but this seems less footgunny.) Function '_ZN2JS7CompileEP9JSContextNS_6HandleIP8JSObjectEERKNS_22ReadOnlyCompileOptionsERNS_18SourceBufferHolderE|JSScript* JS::Compile(JSContext*, class JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions*, JS::SourceBufferHolder*)' has unrooted '<returnvalue>' of type 'JSScript*' live across GC call '_ZN18AutoLastFrameCheckD1Ev|void AutoLastFrameCheck::~AutoLastFrameCheck()' at js/src/jsapi.cpp:4626 Function '_ZN2JS15CompileFunctionEP9JSContextNS_6HandleIP8JSObjectEERKNS_22ReadOnlyCompileOptionsEPKcjPKSA_RNS_18SourceBufferHolderE|JSFunction* JS::CompileFunction(JSContext*, class JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions*, int8*, uint32, int8**, JS::SourceBufferHolder*)' has unrooted '<returnvalue>' of type 'JSFunction*' live across GC call '_ZN18AutoLastFrameCheckD1Ev|void AutoLastFrameCheck::~AutoLastFrameCheck()' at js/src/jsapi.cpp:4833

One ugliness of the previous patch is that it makes one variant of JS::Compile mismatch the rest. If we don't mind the churn, we could change all of them. I don't know if it's worth it, so I'll punt the decision to my reviewer. :-)

Comment on attachment 8444834 [details] [diff] [review] Return value rooting for JS engine Review of attachment 8444834 [details] [diff] [review]: ----------------------------------------------------------------- These are amazingly subtle. ::: js/src/jsfun.cpp @@ +1541,5 @@ > > bool isStarGenerator = generatorKind == StarGenerator; > JS_ASSERT(generatorKind != LegacyGenerator); > > + RootedScript maybeScript(cx, nullptr); Remove the ,nullptr. ::: js/src/vm/Stack.cpp @@ +666,5 @@ > #ifdef JS_ION > , ionInlineFrames_(cx, (js::jit::JitFrameIterator*) nullptr) > #endif > { > + // settleOnActivation can only GC if principals are given End with a period. @@ +678,5 @@ > #ifdef JS_ION > , ionInlineFrames_(cx, (js::jit::JitFrameIterator*) nullptr) > #endif > { > + // settleOnActivation can only GC if principals are given Ditto.

Comment on attachment 8444827 [details] [diff] [review] Switch all JS Compile functions to use MutableHandle Review of attachment 8444827 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi-tests/testCloneScript.cpp @@ +109,5 @@ > JSAutoCompartment a(cx, A); > JS::CompileOptions options(cx); > options.setFileAndLine(__FILE__, 1); > + JS::RootedFunction fun(cx); > + JS_CompileFunction(cx, A, "f", We should probably check these for clarity, even though we're generally also checking the outparam after. Please audit the uses in this patch and add a CHECK on any that are missing it now.

Comment on attachment 8444832 [details] [diff] [review] Return value rooting for dom/ >+ NS_ENSURE_TRUE(unrootedScopeObj, JSVAL_NULL); JS::NullValue. r=me

Comment on attachment 8444651 [details] [diff] [review] hazard analysis output improvements Review of attachment 8444651 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/devtools/rootAnalysis/analyzeRoots.js @@ +112,5 @@ > if (ignoreEdgeUse(edge, variable)) > return 0; > > if (variable.Kind == "Return" && body.Index[1] == edge.Index[1] && body.BlockId.Kind == "Function") > + return edge.Index[1]; // Last point in function body uses the return value Period at the end, since you're here.

Comment on attachment 8444634 [details] [diff] [review] Consider internal function names in analysis Review of attachment 8444634 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/devtools/rootAnalysis/computeCallgraph.js @@ +257,5 @@ > > var minStream = xdb.min_data_stream(); > var maxStream = xdb.max_data_stream(); > > +const internalMarker = " *INTERNAL* "; Maybe put this in utility.js and avoid the explicit use in analyzeRoots.js @@ +289,5 @@ > + // This is slightly conservative in the case where they are *not* > + // identical, but that should be rare enough that we don't care. (The other > + // much simpler but unsafe alternative would be to simply delete any > + // " *INTERNAL* " substring, but I wasn't sure how possible it was for one > + // version of the constructor to be different enough to GC.) Remove parenthetical

Comment on attachment 8444645 [details] [diff] [review] Ignore trivial |return nullptr;| statements that cross GC boundaries Review of attachment 8444645 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/devtools/rootAnalysis/analyzeRoots.js @@ +126,5 @@ > + else if (expressionUsesVariable(edge.Exp[1], variable)) > + rv = src; > + if (rv && isReturningImmobileValue(edge, variable)) > + rv = 0; > + return rv; It looks like you can restructure this to avoid the rv variable by putting the test for isReturningImmobileValue first.

Comment on attachment 8444656 [details] [diff] [review] Include return values in analysis Review of attachment 8444656 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/devtools/rootAnalysis/analyzeRoots.js @@ +84,5 @@ > } > return false; > } > > +// If the edge uses the given variable, return the point at earliest point at mangled comment

(In reply to Terrence Cole [:terrence] from comment #17) > Comment on attachment 8444834 [details] [diff] [review] > Return value rooting for JS engine > > Review of attachment 8444834 [details] [diff] [review]: > ----------------------------------------------------------------- > > These are amazingly subtle. > > ::: js/src/jsfun.cpp > @@ +1541,5 @@ > > > > bool isStarGenerator = generatorKind == StarGenerator; > > JS_ASSERT(generatorKind != LegacyGenerator); > > > > + RootedScript maybeScript(cx, nullptr); > > Remove the ,nullptr. Ok, but fair warning -- at some point, I'd like to add valgrind annotations to these to mark them as uninitialized if you don't pass in a value. (valgrind annotations instead of compiler annotations because I don't think the compiler will be able to figure out the common use patterns statically.) I suspect we have bugs where we use things that weren't set to a value. As long as we're force-initializing to InitialValue when you don't pass any arguments, those are safe, but I've seen enough funky error handling patterns to want as much automated checking on them as possible.

(In reply to Steve Fink [:sfink] from comment #24) > (In reply to Terrence Cole [:terrence] from comment #17) > > Comment on attachment 8444834 [details] [diff] [review] > > Return value rooting for JS engine > > > > Review of attachment 8444834 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > These are amazingly subtle. > > > > ::: js/src/jsfun.cpp > > @@ +1541,5 @@ > > > > > > bool isStarGenerator = generatorKind == StarGenerator; > > > JS_ASSERT(generatorKind != LegacyGenerator); > > > > > > + RootedScript maybeScript(cx, nullptr); > > > > Remove the ,nullptr. > > Ok, but fair warning -- at some point, I'd like to add valgrind annotations > to these to mark them as uninitialized if you don't pass in a value. > (valgrind annotations instead of compiler annotations because I don't think > the compiler will be able to figure out the common use patterns statically.) > I suspect we have bugs where we use things that weren't set to a value. As > long as we're force-initializing to InitialValue when you don't pass any > arguments, those are safe, but I've seen enough funky error handling > patterns to want as much automated checking on them as possible. Oh, neat!

Comment on attachment 8444819 [details] [diff] [review] Resolve hazard by switching Compile to MutableHandle [Security approval request comment] > How easily could an exploit be constructed based on the patch? Seems very hard but not impossible. You'd have to get GC triggered at just the right time, then somehow cause a new script to be created with a different security or something. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? It mentions a "hazard" in the check-in comment, which describes the general sort of problem it addresses. But that's obvious from the patch too. > Which older supported branches are affected by this flaw? Exact rooting landed in 28. > If not all supported branches, which bug introduced the flaw? bug 753203 made it relevant. Before then, conservative stack scanning would have rooted the pointer. > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? For the release branch, it's somewhat different, but I don't think we need to land there. It applied cleanly to beta, so I'm assuming it'll be easy everywhere else. > How likely is this patch to cause regressions; how much testing does it need? Pretty unlikely. The only thing I can think of is if I messed up somewhere in getting the script assigned to the right place, but that would immediately show up in tests. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 753203 User impact if declined: difficult to trigger, but a GC at the wrong time could collect a newborn script and something else might get put into that location and used. Basically, a use-after-free. With GGC, the problems are slightly more likely. But either way, it would probably crash somehow. Testing completed (on m-c, etc.): try pushes Risk to taking this patch (and alternatives if risky): low risk, it's just registering more pointers with the GC. String or IDL/UUID changes made by this patch: none

Comment on attachment 8444819 [details] [diff] [review] Resolve hazard by switching Compile to MutableHandle sec-approval+ for trunk. Once it is green there, please land on Aurora and Beta branches.

Comment on attachment 8444831 [details] [diff] [review] Return value rooting for content/ [Security approval request comment] > How easily could an exploit be constructed based on the patch? My best guess is "difficult". This is a use-after-free, but the timing is hard to control (you need a GC at a very weird time). And beta only has one problematic site; most of the problems were introduced later. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? It's a rooting hazard. That can be figured out from the patch. The fact that it's a security problem (assuming it is?) is not obvious. > Which older supported branches are affected by this flaw? Hard to say. Beta doesn't have most of the problems, but it does have one instance. Conservatively, everything since 28 has it. > If not all supported branches, which bug introduced the flaw? Bug 753203 made the flaw matter. > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I have a backport for beta. I haven't made one for aurora yet. It'll be the mozilla-central patch with some pieces removed, for stuff that hadn't been added yet. The variation is low risk. > How likely is this patch to cause regressions; how much testing does it need? It's just adding some roots, so low risk for regressions. If it passes the test suite, it shouldn't break anything. The higher risk is in missing hazards that existed in earlier versions but have since been removed. Once I have a full set of backports, I'll have to rerun the updated hazard analysis on each branch. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 753203, sort of User impact if declined: rare crashes, maybe a high effort exploit Testing completed: try server Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none

content/ patch for beta. Hardly anything left. I don't think this needs re-review; it's just deleting the irrelevant chunks from the m-c patch.

Comment on attachment 8444832 [details] [diff] [review] Return value rooting for dom/ [Approval Request Comment] Bug caused by (feature/regressing bug #): exact rooting, bug 753203 User impact if declined: crashes, possibly exploitable Testing completed: try server Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none [Security approval request comment] Same as the other patches in this bug. I have all of the backports done. They are very minor variations. Almost everything is just removing fixes that weren't needed earlier.

Comment on attachment 8444833 [details] [diff] [review] Return value rooting for workers [Approval Request Comment] Bug caused by (feature/regressing bug #): exact rooting, bug 753203 User impact if declined: crashes, possibly exploitable Testing completed: try server Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none [Security approval request comment] Same as the other patches in this bug. I have all of the backports done. They are very minor variations. Almost everything is just removing fixes that weren't needed earlier.

Comment on attachment 8444834 [details] [diff] [review] Return value rooting for JS engine [Approval Request Comment] Bug caused by (feature/regressing bug #): exact rooting, bug 753203 User impact if declined: crashes, possibly exploitable Testing completed: try server Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none [Security approval request comment] Same as the other patches in this bug. I have all of the backports done. They are very minor variations. Almost everything is just removing fixes that weren't needed earlier.

Comment on attachment 8444839 [details] [diff] [review] Return value rooting for XPConnect [Approval Request Comment] Bug caused by (feature/regressing bug #): exact rooting, bug 753203 User impact if declined: crashes, possibly exploitable Testing completed: try server Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none [Security approval request comment] Same as the other patches in this bug. I have all of the backports done. They are very minor variations. Almost everything is just removing fixes that weren't needed earlier.

Comment on attachment 8444841 [details] [diff] [review] Return value rooting for SPS [Approval Request Comment] Bug caused by (feature/regressing bug #): exact rooting, bug 753203 User impact if declined: crashes, possibly exploitable Testing completed: try server Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none [Security approval request comment] Same as the other patches in this bug. I have all of the backports done. They are very minor variations. Almost everything is just removing fixes that weren't needed earlier.

Comment on attachment 8446153 [details] [diff] [review] Return value rooting for content/ [Approval Request Comment] Bug caused by (feature/regressing bug #): exact rooting, bug 753203 User impact if declined: crashes, possibly exploitable Testing completed: try server Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none [Security approval request comment] Same as the other patches in this bug. I have all of the backports done. They are very minor variations. Almost everything is just removing fixes that weren't needed earlier.

So many approvals.

Oh. Come to think of it, b2g doesn't need these unless it has exact rooting, which only landed in 32.

dom/ patch for aurora

While I've been flailing around trying to get this finished up, a few more rooting hazards have been introduced that are only detected with the analysis fixes. This is one. The hazard itself is simple -- AutoJSAPI::InitInternal holds aGlobal live across the AutoCxPusher constructor, which can GC. But the fix isn't completely straightforward because the cx used to root must be in a request already, and the request is currently only entered in the middle of the AutoCxPusher construction. So I've just added an additional request entry just for the rooting.

Comment on attachment 8447850 [details] [diff] [review] AutoJSAPI initialization rooting Review of attachment 8447850 [details] [diff] [review]: ----------------------------------------------------------------- Talked with sfink about this on IRC. AutoCxPusher shouldn't GC, and we're going to make sure the analysis understands that.

http://hg.mozilla.org/integration/mozilla-inbound/rev/f8feedcca432 http://hg.mozilla.org/integration/mozilla-inbound/rev/92065bea99d2 http://hg.mozilla.org/integration/mozilla-inbound/rev/a76b54b14c7e http://hg.mozilla.org/integration/mozilla-inbound/rev/210a1a8bfe7d http://hg.mozilla.org/integration/mozilla-inbound/rev/965c8f8bce9a http://hg.mozilla.org/integration/mozilla-inbound/rev/3bff0ddf73b3 http://hg.mozilla.org/integration/mozilla-inbound/rev/70eab71d392b http://hg.mozilla.org/integration/mozilla-inbound/rev/635b82606806 http://hg.mozilla.org/integration/mozilla-inbound/rev/073ff2d2db9c http://hg.mozilla.org/integration/mozilla-inbound/rev/830b491a8fa3 http://hg.mozilla.org/integration/mozilla-inbound/rev/d4bd5c2fdce2 http://hg.mozilla.org/integration/mozilla-inbound/rev/5885dc571d0f http://hg.mozilla.org/integration/mozilla-inbound/rev/c01a6dcd4b22 http://hg.mozilla.org/integration/mozilla-inbound/rev/4ef6aeac24e1

Backed out on Aurora for build bustage across the board apparently from a bad merge: https://hg.mozilla.org/releases/mozilla-aurora/rev/ee727fe51f06 https://tbpl.mozilla.org/php/getParsedLog.php?id=42877395&tree=Mozilla-Aurora

Aurora initial doomed landing: https://hg.mozilla.org/releases/mozilla-aurora/rev/5b44fa1cea34 https://hg.mozilla.org/releases/mozilla-aurora/rev/60e8b99055e9 https://hg.mozilla.org/releases/mozilla-aurora/rev/127429f7dd19 https://hg.mozilla.org/releases/mozilla-aurora/rev/2505eb1fd336 https://hg.mozilla.org/releases/mozilla-aurora/rev/9b5ade54f72a https://hg.mozilla.org/releases/mozilla-aurora/rev/cde190482d72 https://hg.mozilla.org/releases/mozilla-aurora/rev/232abc366696 Aurora backout: https://hg.mozilla.org/releases/mozilla-aurora/rev/ee727fe51f06 Aurora re-landing: https://hg.mozilla.org/releases/mozilla-aurora/rev/ac90b02a034a https://hg.mozilla.org/releases/mozilla-aurora/rev/adec0f465782 https://hg.mozilla.org/releases/mozilla-aurora/rev/87216353f2dc https://hg.mozilla.org/releases/mozilla-aurora/rev/17e442446904 https://hg.mozilla.org/releases/mozilla-aurora/rev/07d419bd7fd0 https://hg.mozilla.org/releases/mozilla-aurora/rev/97ef8ef6033f https://hg.mozilla.org/releases/mozilla-aurora/rev/d0275ba5f438 Beta landing: remote: https://hg.mozilla.org/releases/mozilla-beta/rev/f95da240e806 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/0c9489046e05 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/60476fb8c23a remote: https://hg.mozilla.org/releases/mozilla-beta/rev/5ec5fca25325 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/6f5f91a5dae3 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/1dc6d67685a8 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/0db64aa7af8d

https://hg.mozilla.org/mozilla-central/rev/4ef6aeac24e1 https://hg.mozilla.org/mozilla-central/rev/92065bea99d2 https://hg.mozilla.org/mozilla-central/rev/a76b54b14c7e https://hg.mozilla.org/mozilla-central/rev/210a1a8bfe7d https://hg.mozilla.org/mozilla-central/rev/965c8f8bce9a https://hg.mozilla.org/mozilla-central/rev/3bff0ddf73b3 https://hg.mozilla.org/mozilla-central/rev/70eab71d392b https://hg.mozilla.org/mozilla-central/rev/635b82606806 https://hg.mozilla.org/mozilla-central/rev/073ff2d2db9c https://hg.mozilla.org/mozilla-central/rev/830b491a8fa3 https://hg.mozilla.org/mozilla-central/rev/d4bd5c2fdce2 https://hg.mozilla.org/mozilla-central/rev/5885dc571d0f https://hg.mozilla.org/mozilla-central/rev/c01a6dcd4b22 https://hg.mozilla.org/mozilla-central/rev/4ef6aeac24e1

Marking [qa-] for verification purposes based on comment 26. Feel free to add STR or other info if you feel you need QA to test this. Thank you.