Closed Bug 806433 Opened 8 years ago Closed 8 years ago
Use-after-free of XPCIncremental
I'm pretty sure the assertion failing in bug 802471 indicates a use-after-free bug: we're calling the destructor on an object that the XPCJSRuntime still has a pointer to. At a first glance, it seems sketchy that mReleaseRunnable is a raw pointer instead of a refcounted pointer. Is there any reason why it can't be a refcounted pointer? I'm looking into precisely how this is happening to try to figure out the lifetime, in case this assertion indicates something else is actually going wrong. I'm not sure why this assertion suddenly started showing up. Maybe bug 791798 jumbled up the behavior enough to cause some new ordering, though the timing isn't quite right. I'm speculatively marking this as affecting 17, 18, 19, and as a sec-critical.
Try run for a patch that replaces the raw pointer with a nsRefPtr: https://tbpl.mozilla.org/?tree=Try&rev=0bf2a42144d4
Seems okay on try so far...
Comment on attachment 676381 [details] [diff] [review] make mReleaseRunnable a ref ptr Remove mReleaseRunnable(nullptr) from the ctor.
Attachment #676381 - Flags: review?(bugs) → review+
Carrying forward Olli's r+. [Security approval request comment] How easily can the security issue be deduced from the patch? I changed a weak pointer to a strong pointer, which generally indicates some kind of UAF. But if you know what to look for, the indication that there's a UAF here is already present in the public bug 802471. 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? This was initially inspired by a public random orange, bug 802471, which has only shown up on 19, but I'll want to land this on 17 and 18 to be safe. If not all supported branches, which bug introduced the flaw? bug 743112 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Same patch should work, low risk. How likely is this patch to cause regressions; how much testing does it need? The only possible problem would be leaks, but that seems unlikely given the nature of the object we're creating a strong ref to.
I'm still not exactly sure why this is happening on 19, but not earlier. Peter touched this code around the start of October, but it was 2 weeks before it started showing up on Tinderbox.
Furthermore, the particular sequence of events that would lead to this is unclear. Somehow we're calling XPCIncrementalReleaseRunnable::Run() but we end up returning while (a) there are still items to process and (b) we haven't AddRefed |this|, which it seems like NS_DispatchToMainThread should do on success.
Comment on attachment 676765 [details] [diff] [review] make mReleaseRunnable a ref ptr Please nominate for affected branches as well.
Attachment #676765 - Flags: sec-approval? → sec-approval+
I'll land it myself once the tree has settled down.
Comment on attachment 676765 [details] [diff] [review] make mReleaseRunnable a ref ptr [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 743112 User impact if declined: possible security problems, crashes Testing completed (on m-c, etc.): it has been on m-c for a day or two, with no signs of regressions Risk to taking this patch (and alternatives if risky): low, only thing I can think of is that it maybe could cause leaks String or UUID changes made by this patch: none
Thanks. https://hg.mozilla.org/releases/mozilla-beta/rev/3baf42fac879 Aurora is closed right now, so I'll land it there later.
You need to log in before you can comment on or make changes to this bug.