We need the hazard analysis to be on GCC 7 before we can require GCC 7.

I guess the JS engine is not exactly the right place for this...?

I put the hazard stuff under the GC component.

I have this mostly working now with gcc 8 (I haven't downloaded gcc7, but I notice there's a toolchain task for it so maybe I'll just grab it from there). I will continue working on it.

I have this working locally with gcc 8. I have a try push that builds and runs with gcc 7, but it's failing some self-tests. I'll download and fix that locally next.

Current status: works for the shell using gcc 8.3.0. It runs over the full browser, but find 586 hazards (which is 586 more than expected.) Looking through those now. Giving up on gcc 7.

This is correct; if it's one of the odd cases, then it is not going to be matching the variable we care about, so it's ok to return false.

This includes some annotations to isOverridableField() that are only necessary at this point in the patch stack; the entire function will be removed shortly.

Comment on attachment 9094053 [details]
Bug 1560667 - Collection of fixes for things uncovered by improvements to the hazard analysis. r=bzbarsky!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Very difficult. It is very likely that almost all of these are false positives. They depend on a refcount dropping to zero, but most of the time nothing runs within the relevant scope that could possibly decrement the refcount in question. A few of the fixes here don't depend on that, though, so are the usual for rooting hazards -- you have to do some pretty careful orchestration to get a GC to happen at just the right moment and then have the UAF expose something of value. So I'd say difficult but likely not impossible.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: Some of these go way back. This is stuff the hazard analysis has been ignoring since it was written.
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Give the amount of code touched here, conflicts are likely, but the changes will either be unneeded (because the code didn't exist) or just require manually redoing them on top of the older code. Especially where new { scopes } were added; these are very likely to syntactically conflict with anything in that scope. But they are unlikely to require any different techniques.
• How likely is this patch to cause regressions; how much testing does it need?: Existing tests should be fine. This shouldn't affect behavior.

Ugh. I just realized I tangled up the fixes with the gcc upgrade. They really aren't related, but now I've posted a sec-approval request in a public bug. :-(

I'm marking this bug as a security issue, and taking the sec-approval comment private.

I'm not sure how to disentangle this stuff. The way I have it now, it looks like backporting the gcc upgrade is necessary. It isn't. The only things that would need to be backported are the fixes in https://phabricator.services.mozilla.com/D46534. And those fixes need to land before I can land the GCC upgrade on central.

I really ought to have a separate bug for the fixes. That was my original intent with bug 1577915, but I ended up nerfing that bug down to one of the root causes, one that isn't terribly important. I guess maybe I could mark this comment and the previous private, and delete the attachment for that one patch, but I'm not sure if that would adequately sanitize this bug.

At any rate, the priority should be in landing the security patch here. It might be safest to create a new bug for the gcc upgrade, annoying as that would be.

This comment should perhaps be on the separate bug for the fixes. but for now I'll put it here.

I wanted to explain why these hazards are appearing now.

The hazard analysis has long had the ability to detect hazards involving unrooted return values. (Note that return values are unrootable.) Using Waldo's example, with a reordering to prevent it from looking like there's an easy fix:

  JSString* foo(JSContext* cx) {
      Rooted<JSString*> str(cx, JS_NewStringCopyN(cx, "ohai", 4));
      DtorWillGC willGc;
      return str;
  }

The hazard analysis will complain here: str is nicely rooted until the return statement, at which point the actual JSString* is extracted and placed on the stack. Then ~DtorWillGC runs, invalidating the returned JSString* pointer. Even if the caller immediately stores the returned value into a new Rooted, it's too late. Also, the Rooted str is still alive when the GC runs, but it doesn't help because it's protecting a different stack location.

The problem is two-fold: until recently the analysis did not think that ~nsCOMPtr<T> would ever GC, because it couldn't see the full call chain for the destructor. While working on the GCC upgrade, I had to fix some problems related to destructors, and in the process stumbled across an ancient chunk of code that just completely dropped certain destructor forms because they used a weird construct that crashed sixgill. As part of my fixes, I handled that construct, so was able to stop skipping them. (It may be that older versions of GCC did not use the destructors in the same way, so it didn't matter? There has been a lot of churn around destructors across GCC versions.) That was the fix for the first problem.

But that fix revealed that ~nsCOMPtr calls Release, which I had annotated as safe because it was a large source of false positives and all the call chains I had looked at would never happen in practice. That's still largely true; see bug 1579303. But it's not completely true, so I removed that annotation. This fixed the second problem.

The two fixes together enabled the analysis to see that ~nsCOMPtr can potentially invalidate return values. Which makes it problematic to return bare GC pointers if you have an nsCOMPtr (or RefPtr) in scope at the end of the function.

The majority of these situations are perfectly safe, because the instantiation of the nsCOMPtr increments the refcount from something that already has a refcount >= 1. So the decrement at the end of the scope merely drops it back down to its original value. The problem would be when something within that scope runs and releases whatever was originally holding onto the object -- in that case, the end-of-scope ~nsCOMPtr will call Destroy, which could in theory either do something that GCs itself, or could break a cycle and invoke the cycle collector to delete other unrelated objects whose Destroy could GC.

(In reply to Steve Fink [:sfink] [:s:] from comment #10)

I'm marking this bug as a security issue, and taking the sec-approval comment private.

You don't really need to do both, at least not with a small enough number of CCs that we can know them all. I'm going to unhide comment 9. If we want to make the bug public then we could re-hide comment 9 (and a bunch more besides), which is probably a good argument for not making the bug public for now.

I need a rating to think about the sec-approval. Since most cases appear to be fine in practice, to the extent we can reason about them, sec-moderate seems about right. go ahead and land when you're ready. sec-approval+

https://hg.mozilla.org/integration/autoland/rev/ef6db5e180346ec17f0f039e60f4e4505572ccaa
https://hg.mozilla.org/integration/autoland/rev/55b80c2e2228aeddd724898e6f2511b9099c5f24
https://hg.mozilla.org/integration/autoland/rev/dfecb23de876e3aa1dfa4907af1859cf3fd023e8
https://hg.mozilla.org/integration/autoland/rev/5a8a7c48c850766084ca90175b9641e34e2fdc6b
https://hg.mozilla.org/mozilla-central/rev/ef6db5e18034
https://hg.mozilla.org/mozilla-central/rev/55b80c2e2228
https://hg.mozilla.org/mozilla-central/rev/dfecb23de876
https://hg.mozilla.org/mozilla-central/rev/5a8a7c48c850

Do any of these patches need backport to Beta/ESR68?

Comment on attachment 9094053 [details]
Bug 1560667 - Collection of fixes for things uncovered by improvements to the hazard analysis. r=bzbarsky!

Beta/Release Uplift Approval Request

• User impact if declined: Potential for small number of crashes, smaller potential for exploitable flaws.

Many of these fixes are likely to be false positives, but it's difficult to be 100% sure. The script loader stuff is the only one that jumps out at me as being controllable enough to intentionally trigger.

The underlying thing going on here is that the hazard analysis was not seeing some cases where destructors could GC. But most of these destructors will only GC if a ref count drops to zero, which can only happen if something runs within the scope of the RAII refcount object that releases the underlying object from wherever else it's being held. This is often impossible, but the hazard analysis isn't smart enough (yet!) to know that. Some of these are possible, if unlikely, though.

• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The fixes are all minor code restructurings (mostly moving implicit destructor calls earlier).
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: See above.
• Fix Landed on Version: 71
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): See above
• String or UUID changes made by this patch:

This is going to need rebased patches for both Beta and ESR68.

I'm inclined to let this ride with 71 unless you feel very strongly about it.

Comment on attachment 9094053 [details]
Bug 1560667 - Collection of fixes for things uncovered by improvements to the hazard analysis. r=bzbarsky!

Let's give this some bake time. Then we'd have more validation for 71 release and its corresponding esr.

Comment on attachment 9094053 [details]
Bug 1560667 - Collection of fixes for things uncovered by improvements to the hazard analysis. r=bzbarsky!

Clearing the ESR68 approval request on this patch. Please request approval on the rebased patch when it's ready.

Comment on attachment 9105656 [details] [diff] [review]
Collection of fixes for things uncovered by improvements to the hazard analysis

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Rare crashes, some small potential for exploits.
• Fix Landed on Version: 71
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Minor code reordering, which has no effect when no GC happens and possibly avoids a problem when it does happen.
• String or UUID changes made by this patch: none

https://hg.mozilla.org/releases/mozilla-esr68/rev/1769023cd095