Closed Bug 1022773 Opened 10 years ago Closed 10 years ago

Static hazard analysis does not detect a return value held live across a GC

Categories

(Core :: JavaScript: GC, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox30 --- wontfix
firefox31 --- fixed
firefox32 --- fixed
firefox33 + fixed
firefox-esr24 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

(Keywords: sec-high, Whiteboard: [qa-][adv-main31+])

Attachments

(16 files, 3 obsolete files)

3.65 KB, patch
bhackett1024
: review+
sfink
: checkin+
Details | Diff | Splinter Review
2.91 KB, patch
sfink
: checkin+
Details | Diff | Splinter Review
3.50 KB, patch
bhackett1024
: review+
sfink
: checkin+
Details | Diff | Splinter Review
7.11 KB, patch
terrence
: review+
sfink
: checkin+
Details | Diff | Splinter Review
7.10 KB, patch
bhackett1024
: review+
sfink
: checkin+
Details | Diff | Splinter Review
17.36 KB, patch
terrence
: review+
smaug
: review+
bholley
: review+
abillings
: sec-approval+
sfink
: checkin+
Details | Diff | Splinter Review
63.39 KB, patch
terrence
: review+
sfink
: checkin+
Details | Diff | Splinter Review
6.74 KB, patch
smaug
: review+
abillings
: sec-approval+
sfink
: checkin+
Details | Diff | Splinter Review
6.86 KB, patch
bzbarsky
: review+
abillings
: sec-approval+
sfink
: checkin+
Details | Diff | Splinter Review
4.86 KB, patch
bent.mozilla
: review+
abillings
: sec-approval+
sfink
: checkin+
Details | Diff | Splinter Review
24.62 KB, patch
terrence
: review+
sfink
: checkin+
Details | Diff | Splinter Review
7.40 KB, patch
bholley
: review+
abillings
: sec-approval+
sfink
: checkin+
Details | Diff | Splinter Review
2.50 KB, patch
ehsan.akhgari
: review+
abillings
: sec-approval+
sfink
: checkin+
Details | Diff | Splinter Review
1.62 KB, patch
abillings
: sec-approval+
sfink
: checkin+
Details | Diff | Splinter Review
5.99 KB, patch
sfink
: checkin+
Details | Diff | Splinter Review
2.32 KB, patch
bholley
: review+
Details | Diff | Splinter Review
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.)
See Also: → 1009675
Keywords: sec-high
Depends on: 1028629
Treat the final point in a function as using the return value.
Attachment #8444620 - Flags: review?(bhackett1024)
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.
Attachment #8444620 - Flags: review?(bhackett1024)
Ok, here's the proper patch. The full description of the tricky part is in a comment.
Attachment #8444630 - Flags: review?(bhackett1024)
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.
Attachment #8444634 - Flags: review?(bhackett1024)
Attachment #8444630 - Attachment is obsolete: true
Attachment #8444630 - Flags: review?(bhackett1024)
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.
Attachment #8444645 - Flags: review?(bhackett1024)
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.)
Attachment #8444651 - Flags: review?(terrence)
I meant to upload this first, but this is the interesting patch that adds the return value handling.
Attachment #8444656 - Flags: review?(bhackett1024)
Attachment #8444620 - Attachment is obsolete: true
Depends on: 1029227
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
Attachment #8444819 - Flags: review?(terrence)
Attachment #8444819 - Flags: review?(bugs)
Attachment #8444819 - Flags: review?(bobbyholley)
Attachment #8444819 - Flags: review?(bugs) → review+
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. :-)
Attachment #8444827 - Flags: review?(terrence)
Attachment #8444831 - Flags: review?(bugs)
Attachment #8444832 - Flags: review?(bzbarsky)
Attachment #8444833 - Flags: review?(bent.mozilla)
Attachment #8444834 - Flags: review?(terrence)
Attachment #8444839 - Flags: review?(bobbyholley)
Attachment #8444841 - Flags: review?(ehsan)
Attachment #8444831 - Flags: review?(bugs) → review+
Attachment #8444841 - Flags: review?(ehsan) → review+
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.
Attachment #8444834 - Flags: review?(terrence) → review+
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.
Attachment #8444827 - Flags: review?(terrence) → review+
Attachment #8444833 - Flags: review?(bent.mozilla) → review+
Comment on attachment 8444832 [details] [diff] [review]
Return value rooting for dom/

>+  NS_ENSURE_TRUE(unrootedScopeObj, JSVAL_NULL);

JS::NullValue.

r=me
Attachment #8444832 - Flags: review?(bzbarsky) → review+
Attachment #8444839 - Flags: review?(bobbyholley) → review+
Attachment #8444819 - Flags: review?(terrence) → review+
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.
Attachment #8444651 - Flags: review?(terrence) → review+
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
Attachment #8444634 - Flags: review?(bhackett1024) → review+
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.
Attachment #8444645 - Flags: review?(bhackett1024) → review+
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
Attachment #8444656 - Flags: review?(bhackett1024) → review+
(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.
Summary: Static hazard analysis misses does not detect a return value held live across a GC → Static hazard analysis does not detect a return value held live across a GC
Attachment #8444819 - Flags: review?(bobbyholley) → review+
(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
Attachment #8444819 - Flags: sec-approval?
Attachment #8444819 - Flags: approval-mozilla-beta?
Attachment #8444819 - Flags: approval-mozilla-b2g30?
Attachment #8444819 - Flags: approval-mozilla-b2g28?
Attachment #8444819 - Flags: approval-mozilla-aurora?
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.
Attachment #8444819 - Flags: sec-approval?
Attachment #8444819 - Flags: sec-approval+
Attachment #8444819 - Flags: approval-mozilla-beta?
Attachment #8444819 - Flags: approval-mozilla-beta+
Attachment #8444819 - Flags: approval-mozilla-aurora?
Attachment #8444819 - Flags: approval-mozilla-aurora+
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
Attachment #8444831 - Flags: sec-approval?
Attachment #8444831 - Flags: approval-mozilla-beta?
Attachment #8444831 - Flags: approval-mozilla-b2g30?
Attachment #8444831 - Flags: approval-mozilla-b2g28?
Attachment #8444831 - Flags: approval-mozilla-aurora?
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.
Attachment #8444831 - Flags: sec-approval?
Attachment #8444831 - Flags: sec-approval+
Attachment #8444831 - Flags: approval-mozilla-beta?
Attachment #8444831 - Flags: approval-mozilla-beta+
Attachment #8444831 - Flags: approval-mozilla-aurora?
Attachment #8444831 - Flags: approval-mozilla-aurora+
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.
Attachment #8444832 - Flags: sec-approval?
Attachment #8444832 - Flags: approval-mozilla-beta?
Attachment #8444832 - Flags: approval-mozilla-b2g30?
Attachment #8444832 - Flags: approval-mozilla-b2g28?
Attachment #8444832 - Flags: approval-mozilla-aurora?
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.
Attachment #8444833 - Flags: sec-approval?
Attachment #8444833 - Flags: approval-mozilla-beta?
Attachment #8444833 - Flags: approval-mozilla-b2g30?
Attachment #8444833 - Flags: approval-mozilla-b2g28?
Attachment #8444833 - Flags: approval-mozilla-aurora?
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.
Attachment #8444834 - Flags: approval-mozilla-beta?
Attachment #8444834 - Flags: approval-mozilla-b2g30?
Attachment #8444834 - Flags: approval-mozilla-b2g28?
Attachment #8444834 - Flags: approval-mozilla-aurora?
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.
Attachment #8444839 - Flags: sec-approval?
Attachment #8444839 - Flags: approval-mozilla-beta?
Attachment #8444839 - Flags: approval-mozilla-b2g30?
Attachment #8444839 - Flags: approval-mozilla-b2g28?
Attachment #8444839 - Flags: approval-mozilla-aurora?
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.
Attachment #8444841 - Flags: sec-approval?
Attachment #8444841 - Flags: approval-mozilla-beta?
Attachment #8444841 - Flags: approval-mozilla-b2g30?
Attachment #8444841 - Flags: approval-mozilla-b2g28?
Attachment #8444841 - Flags: approval-mozilla-aurora?
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.
Attachment #8446153 - Flags: sec-approval?
Attachment #8446153 - Flags: approval-mozilla-beta?
Attachment #8446153 - Flags: approval-mozilla-b2g30?
Attachment #8446153 - Flags: approval-mozilla-b2g28?
Attachment #8446153 - Flags: approval-mozilla-aurora?
So many approvals.
Attachment #8444832 - Flags: sec-approval?
Attachment #8444832 - Flags: sec-approval+
Attachment #8444832 - Flags: approval-mozilla-beta?
Attachment #8444832 - Flags: approval-mozilla-beta+
Attachment #8444832 - Flags: approval-mozilla-aurora?
Attachment #8444832 - Flags: approval-mozilla-aurora+
Attachment #8444833 - Flags: sec-approval?
Attachment #8444833 - Flags: sec-approval+
Attachment #8444833 - Flags: approval-mozilla-beta?
Attachment #8444833 - Flags: approval-mozilla-beta+
Attachment #8444833 - Flags: approval-mozilla-aurora?
Attachment #8444833 - Flags: approval-mozilla-aurora+
Attachment #8444834 - Flags: approval-mozilla-beta?
Attachment #8444834 - Flags: approval-mozilla-beta+
Attachment #8444834 - Flags: approval-mozilla-aurora?
Attachment #8444834 - Flags: approval-mozilla-aurora+
Attachment #8444839 - Flags: sec-approval?
Attachment #8444839 - Flags: sec-approval+
Attachment #8444839 - Flags: approval-mozilla-beta?
Attachment #8444839 - Flags: approval-mozilla-beta+
Attachment #8444839 - Flags: approval-mozilla-aurora?
Attachment #8444839 - Flags: approval-mozilla-aurora+
Attachment #8444841 - Flags: sec-approval?
Attachment #8444841 - Flags: sec-approval+
Attachment #8444841 - Flags: approval-mozilla-beta?
Attachment #8444841 - Flags: approval-mozilla-beta+
Attachment #8444841 - Flags: approval-mozilla-aurora?
Attachment #8444841 - Flags: approval-mozilla-aurora+
Attachment #8446153 - Flags: sec-approval?
Attachment #8446153 - Flags: sec-approval+
Attachment #8446153 - Flags: approval-mozilla-beta?
Attachment #8446153 - Flags: approval-mozilla-beta+
Attachment #8446153 - Flags: approval-mozilla-aurora?
Attachment #8446153 - Flags: approval-mozilla-aurora+
Oh. Come to think of it, b2g doesn't need these unless it has exact rooting, which only landed in 32.
Attachment #8444819 - Flags: approval-mozilla-b2g30?
Attachment #8444819 - Flags: approval-mozilla-b2g28?
Attachment #8444831 - Flags: approval-mozilla-b2g30?
Attachment #8444831 - Flags: approval-mozilla-b2g28?
Attachment #8444832 - Flags: approval-mozilla-b2g30?
Attachment #8444832 - Flags: approval-mozilla-b2g28?
Attachment #8444833 - Flags: approval-mozilla-b2g30?
Attachment #8444833 - Flags: approval-mozilla-b2g28?
Attachment #8444834 - Flags: approval-mozilla-b2g30?
Attachment #8444834 - Flags: approval-mozilla-b2g28?
Attachment #8444839 - Flags: approval-mozilla-b2g30?
Attachment #8444839 - Flags: approval-mozilla-b2g28?
Attachment #8444841 - Flags: approval-mozilla-b2g30?
Attachment #8444841 - Flags: approval-mozilla-b2g28?
Attachment #8446153 - Flags: approval-mozilla-b2g30?
Attachment #8446153 - Flags: approval-mozilla-b2g28?
dom/ patch for aurora
Attached patch AutoJSAPI initialization rooting (obsolete) — Splinter Review
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.
Attachment #8447850 - Flags: review?(bobbyholley)
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.
Attachment #8447850 - Flags: review?(bobbyholley) → review-
Attachment #8448368 - Flags: review?(bobbyholley) → review+
Attachment #8447850 - Attachment is obsolete: true
Attachment #8444634 - Flags: checkin+
Attachment #8444639 - Flags: checkin+
Attachment #8444645 - Flags: checkin+
Attachment #8444651 - Flags: checkin+
Attachment #8444656 - Flags: checkin+
Attachment #8444819 - Flags: checkin+
Attachment #8444827 - Flags: checkin+
Attachment #8444831 - Flags: checkin+
Attachment #8444832 - Flags: checkin+
Attachment #8444833 - Flags: checkin+
Attachment #8444834 - Flags: checkin+
Attachment #8444839 - Flags: checkin+
Attachment #8444841 - Flags: checkin+
Attachment #8446153 - Flags: checkin+
Attachment #8447274 - Flags: checkin+
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
Depends on: 1035092
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.
Whiteboard: [qa-]
Whiteboard: [qa-] → [qa-][adv-main31+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.