Closed
Bug 1336510
Opened 7 years ago
Closed 7 years ago
lambda analysis for raw pointers misses references to |this|
Categories
(Developer Infrastructure :: Source Code Analysis, defect, P1)
Developer Infrastructure
Source Code Analysis
Tracking
(firefox-esr4552+ fixed, firefox51 wontfix, firefox52+ fixed, firefox-esr5252+ fixed, firefox53+ fixed, firefox54+ fixed)
People
(Reporter: froydnj, Assigned: nika)
References
Details
(Keywords: csectype-uaf, sec-critical, Whiteboard: [post-critsmash-triage][adv-main52+][adv-esr45.8+])
Attachments
(20 files, 1 obsolete file)
Over in bug 1328768, we had the following code: http://searchfox.org/mozilla-central/rev/bf98cd4315b5efa1b28831001ad27d54df7bbb68/ipc/glue/CrashReporterHost.cpp#113 NS_DispatchToMainThread(NS_NewRunnableFunction([=] () -> void { nsCOMPtr<nsICrashService> crashService = do_GetService("@mozilla.org/crashservice;1"); if (crashService) { crashService->AddCrash(processType, crashType, childDumpID); } nsCOMPtr<nsIAsyncShutdownClient> barrier = GetShutdownBarrier(); if (barrier) { barrier->RemoveBlocker(this); // XXX } })); |this| is an instance of a refcounted type, yet the lambda capture analysis doesn't complain about the capture of it, so we're capturing the raw pointer, rather than a strong reference. That seems bad. Was this the result of relaxing the rules in bug 1281935, or just an oversight in the analysis?
Flags: needinfo?(michael)
Assignee | ||
Comment 1•7 years ago
|
||
wrong |
The problem is in this line of code: if (Capture.capturesVariable() && Capture.getCaptureKind() != LCK_ByRef) { QualType Pointee = Capture.getCapturedVar()->getType()->getPointeeType(); Capture.capturesVariable() doesn't return `true` for captures of `this`. I'm pretty sure that the reason why we didn't consider captures on `this` to violate the analysis is because unless `this` is captured, you cannot call private methods on objects of your type. We wanted you to be able to do: nsRefPtr<ThisType> self = this; [this, self] () { self->CallSomePrivateMethod(); } Where it only captures `this` to be able to call private methods. I'll look into rejecting places where you actually use the `this` pointer. I imagine it shouldn't be _too_ hard.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → michael
Flags: needinfo?(michael)
Assignee | ||
Updated•7 years ago
|
Priority: -- → P1
Assignee | ||
Comment 2•7 years ago
|
||
MozReview-Commit-ID: 19jiqArKgxo
Attachment #8833507 -
Flags: review?(ehsan)
Attachment #8833507 -
Flags: feedback?(nfroyd)
Assignee | ||
Comment 3•7 years ago
|
||
MozReview-Commit-ID: 2fuMHyspjhs
Assignee | ||
Comment 4•7 years ago
|
||
MozReview-Commit-ID: KdGBYEAC0dW
Assignee | ||
Comment 5•7 years ago
|
||
MozReview-Commit-ID: 4lVGrGzhVXh
Assignee | ||
Updated•7 years ago
|
Attachment #8833509 -
Flags: review?(bugs)
Assignee | ||
Updated•7 years ago
|
Attachment #8833510 -
Flags: review?(bugmail)
Assignee | ||
Updated•7 years ago
|
Attachment #8833511 -
Flags: review?(jwwang)
Updated•7 years ago
|
Attachment #8833509 -
Flags: feedback+
Comment 6•7 years ago
|
||
Botond, can you please take a look at this change, since in the past you have been a critic of this analysis. :-)
Flags: needinfo?(botond)
Comment 7•7 years ago
|
||
Comment on attachment 8833507 [details] [diff] [review] Part 1: Correct the RefCountedInsideLambda Check to complain about capturing and using `this` without a backing strong reference Review of attachment 8833507 [details] [diff] [review]: ----------------------------------------------------------------- Thank you! ::: build/clang-plugin/RefCountedInsideLambdaChecker.cpp @@ +141,5 @@ > +bool RefCountedInsideLambdaChecker::ThisVisitor::VisitCXXThisExpr(CXXThisExpr *This) { > + QualType Pointee = This->getType()->getPointeeType(); > + if (!Pointee.isNull() && isClassRefCounted(Pointee)) { > + Checker.diag(This->getLocStart(), > + "Refcounted variable %0 of type %1 cannot be captured by a lambda", It sucks to copy these strings. How about adding a helper that takes a source location and diagnoses both of these and use it both here and inside check() above? ::: build/clang-plugin/RefCountedInsideLambdaChecker.h @@ +17,5 @@ > + > +private: > + class ThisVisitor : public RecursiveASTVisitor<ThisVisitor> { > + public: > + ThisVisitor(RefCountedInsideLambdaChecker& Checker) : Checker(Checker) {} Nit: explicit. ::: build/clang-plugin/tests/TestNoRefcountedInsideLambdas.cpp @@ +601,5 @@ > + std::function<void()>([=]() { > + method(); // expected-error{{Refcounted variable 'this' of type 'R' cannot be captured by a lambda}} expected-note{{Please consider using a smart pointer}} > + }); > + std::function<void()>([=]() { > + privateMethod(); // expected-error{{Refcounted variable 'this' of type 'R' cannot be captured by a lambda}} expected-note{{Please consider using a smart pointer}} Nit: please fix indentation.
Attachment #8833507 -
Flags: review?(ehsan) → review+
Comment 8•7 years ago
|
||
Comment on attachment 8833511 [details] [diff] [review] Part 4: Capture a strong reference to this in dom/media Review of attachment 8833511 [details] [diff] [review]: ----------------------------------------------------------------- f+ for webrtc-related things; r? to jib
Attachment #8833511 -
Flags: review?(jib)
Attachment #8833511 -
Flags: feedback+
Comment 9•7 years ago
|
||
Comment on attachment 8833511 [details] [diff] [review] Part 4: Capture a strong reference to this in dom/media Review of attachment 8833511 [details] [diff] [review]: ----------------------------------------------------------------- r? gcp for Cameras*
Attachment #8833511 -
Flags: review?(gpascutto)
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Michael Layzell [:mystor] from comment #1) > nsRefPtr<ThisType> self = this; > [this, self] () { > self->CallSomePrivateMethod(); > } Just a note: I was wrong about this, this works fine: RefPtr<ThisType> self = this; [self] () { self->CallSomePrivateMethod(); }
Comment 11•7 years ago
|
||
(In reply to :Ehsan Akhgari from comment #6) > Botond, can you please take a look at this change, since in the past you > have been a critic of this analysis. :-) Thanks for flagging me :) I think this is a reasonable (and valuable) change.
Flags: needinfo?(botond)
Comment 12•7 years ago
|
||
Comment on attachment 8833510 [details] [diff] [review] Part 3: Capture a strong reference to this in CompositorBridgeParent::FlushApzRepaints Review of attachment 8833510 [details] [diff] [review]: ----------------------------------------------------------------- Stealing review.
Attachment #8833510 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 13•7 years ago
|
||
MozReview-Commit-ID: 19jiqArKgxo
Assignee | ||
Updated•7 years ago
|
Attachment #8833507 -
Attachment is obsolete: true
Attachment #8833507 -
Flags: feedback?(nfroyd)
Comment 14•7 years ago
|
||
Comment on attachment 8833509 [details] [diff] [review] Part 2: Fix lambda captures of this in PresentationConnection And modern C++ bites us again. I wonder if any good (== not total security hazard) features have been added to C++ recently.
Attachment #8833509 -
Flags: review?(bugs) → review+
Comment 15•7 years ago
|
||
Comment on attachment 8833511 [details] [diff] [review] Part 4: Capture a strong reference to this in dom/media Review of attachment 8833511 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaManager.cpp @@ +2298,5 @@ > RefPtr<Refcountable<UniquePtr<SourceSet>>> devices( > new Refcountable<UniquePtr<SourceSet>>(aDevices)); // grab result > > // Ensure that the captured 'this' pointer and our windowID are still good. > if (!MediaManager::Exists() || Now redundant. ::: dom/media/ogg/OggDemuxer.cpp @@ +133,3 @@ > OGG_DEBUG("Reporting telemetry MEDIA_OGG_LOADED_IS_CHAINED=%d", isChained); > Telemetry::Accumulate(Telemetry::ID::MEDIA_OGG_LOADED_IS_CHAINED, isChained); > }); this and self appear unused here. Would [isChained]() suffice?
Attachment #8833511 -
Flags: review?(jib) → review+
Comment 16•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #14) > Comment on attachment 8833509 [details] [diff] [review] > Part 2: Fix lambda captures of this in PresentationConnection > > And modern C++ bites us again. > I wonder if any good (== not total security hazard) features have been added > to C++ recently. These lambdas (to a degree) replace methods plus code tricks like WrapRunnable, but even with that you had to use thread->Dispatch(WrapRunnable(RefPtr<foo>(this), &foo::method, ...) if you were passing 'this' in and there wasn't something else keeping it alive. NewRunnableMethod() doesn't let you do the RefPtr<foo>(this) trick. Any chance this analysis could be extended to NewRunnableMethod(this, ...)?
Flags: needinfo?(michael)
Updated•7 years ago
|
Attachment #8833511 -
Flags: review?(jwwang) → review+
Reporter | ||
Comment 17•7 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #16) > Any chance this analysis could be extended to NewRunnableMethod(this, ...)? NewRunnableMethod takes a strong reference to `this`, so we don't need an analysis like this.
Comment 18•7 years ago
|
||
Looking at the blame, a lot of the code issues found by this analysis affect 52+. I'm assuming this code pattern is serious enough that'll want to uplift?
Updated•7 years ago
|
Attachment #8833511 -
Flags: review?(gpascutto) → review+
Comment 19•7 years ago
|
||
Are we sure we won't want to consider these as at least ride-along patches for any future 51 point release?
Comment 20•7 years ago
|
||
I do think we want to uplift the fixes before landing the analysis itself, because it can easily be run on an older tree.
Comment 21•7 years ago
|
||
(In reply to :Ehsan Akhgari from comment #20) > I do think we want to uplift the fixes before landing the analysis itself, > because it can easily be run on an older tree. I'd happily hold the analysis patches until a few weeks after 52 ships, and have someone just run them occasionally on a local tree. I would strongly suggest we consider these for any 51 point release, and otherwise land them later in the 52 beta cycle. All the fixes should be very safe.
Assignee | ||
Comment 22•7 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #15) > ::: dom/media/ogg/OggDemuxer.cpp > @@ +133,3 @@ > > OGG_DEBUG("Reporting telemetry MEDIA_OGG_LOADED_IS_CHAINED=%d", isChained); > > Telemetry::Accumulate(Telemetry::ID::MEDIA_OGG_LOADED_IS_CHAINED, isChained); > > }); > > this and self appear unused here. Would [isChained]() suffice? Nope! Unfortunately, OGG_DEBUG expands to code which references a member variable.
Flags: needinfo?(michael)
Assignee | ||
Comment 23•7 years ago
|
||
Only part 4 needed to be rebased onto aurora, and one other patch was needed as well. MozReview-Commit-ID: 4lVGrGzhVXh
Assignee | ||
Comment 24•7 years ago
|
||
MozReview-Commit-ID: 4j2TfyyISUg
Assignee | ||
Updated•7 years ago
|
Attachment #8834562 -
Flags: review?(dkeeler)
Comment on attachment 8834562 [details] [diff] [review] Part 5: AURORA: Capture a strong reference to this in dom/u2f Review of attachment 8834562 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. ::: dom/u2f/U2F.cpp @@ -660,2 @@ > mAbstractMainThread->Dispatch(NS_NewRunnableFunction( > - [status, this] () { It looks like there are some other closures that capture `this` but don't actually use it - would this be a good opportunity to also remove those captures?
Attachment #8834562 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 26•7 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #25) > It looks like there are some other closures that capture `this` but don't > actually use it - would this be a good opportunity to also remove those > captures? Perhaps - This patch is intended for AURORA, so I don't think it's worth doing that type of code cleanup, and I think that if we want to clean this up we should do it in another bug. I think keeping this bug focused on the sec changes is a good idea.
Assignee | ||
Comment 27•7 years ago
|
||
MozReview-Commit-ID: H5MVVPtj5aM
Assignee | ||
Comment 28•7 years ago
|
||
MozReview-Commit-ID: 2fuMHyspjhs
Assignee | ||
Comment 29•7 years ago
|
||
MozReview-Commit-ID: KdGBYEAC0dW
Assignee | ||
Comment 30•7 years ago
|
||
MozReview-Commit-ID: 4lVGrGzhVXh
Assignee | ||
Comment 31•7 years ago
|
||
MozReview-Commit-ID: 4j2TfyyISUg
Assignee | ||
Comment 32•7 years ago
|
||
Hey, I've attached patches for central, aurora and beta for this bug. How do you want me to handle sec-approval / landing them?
Flags: needinfo?(dveditz)
Updated•7 years ago
|
Group: core-security → dom-core-security
status-firefox-esr45:
--- → affected
Flags: needinfo?(dveditz)
Keywords: csectype-uaf,
sec-critical
Assignee | ||
Comment 33•7 years ago
|
||
These are the patches rebased onto ESR45. Fortunately there are less lambdas in use here :). MozReview-Commit-ID: H5MVVPtj5aM
Assignee | ||
Comment 34•7 years ago
|
||
MozReview-Commit-ID: Jn4DXFzFnXa
Assignee | ||
Comment 35•7 years ago
|
||
You recently modified nsMultiMixedConv(), and added the following lines: http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/netwerk/streamconv/converters/nsMultiMixedConv.cpp#810-812 These capture `this` by value, while allowing the closure to escape. This will be rejected when this analysis lands. I would simply correct the code to capture a `RefPtr<nsMultiMixedConv> self = this`, but I believe that that will create an unbreakable reference cycle and leak the datastructure. How would you like me to refactor this to make the ownership more clear and avoid the static analysis?
Flags: needinfo?(honzab.moz)
Assignee | ||
Comment 36•7 years ago
|
||
Valentin, you reviewed the above patch. You might also have some insight as to what the correct solution here would be.
Flags: needinfo?(valentin.gosu)
Assignee | ||
Comment 37•7 years ago
|
||
This is a patch which works around the recently added lambda in nsMultiMixedConv. This particular usage was completely safe, but the analysis was rejecting it. This patch uses another mechanism to construct a std::function object which captures a pointer to this in order to avoid the static analysis failure. MozReview-Commit-ID: 7Mvh9aeCKOh
Attachment #8837289 -
Flags: review?(valentin.gosu)
Updated•7 years ago
|
Attachment #8837289 -
Flags: review?(valentin.gosu) → review+
Updated•7 years ago
|
Flags: needinfo?(valentin.gosu)
Comment 39•7 years ago
|
||
(In reply to Michael Layzell [:mystor] from comment #35) > You recently modified nsMultiMixedConv(), and added the following lines: > > http://searchfox.org/mozilla-central/rev/ > d3307f19d5dac31d7d36fc206b00b686de82eee4/netwerk/streamconv/converters/ > nsMultiMixedConv.cpp#810-812 > > These capture `this` by value, while allowing the closure to escape. This > will be rejected when this analysis lands. > > I would simply correct the code to capture a `RefPtr<nsMultiMixedConv> self > = this`, but I believe that that will create an unbreakable reference cycle > and leak the datastructure. How would you like me to refactor this to make > the ownership more clear and avoid the static analysis? BTW, the tokenizer has a "Finish" method that could release (if not already) the lambda (function) to prevent cycle when captured as refptr. We can tho exploit here the fact that mTokenizer won't live longer than this, as you said before. There is also move argument capture like: RefPtr<T> self = this; [self = Move(self)]() -> ... but that is c++14 specific which is only avail on windows, AFAIK. anyway, thanks.
Assignee | ||
Comment 40•7 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #39) > BTW, the tokenizer has a "Finish" method that could release (if not already) > the lambda (function) to prevent cycle when captured as refptr. We can tho > exploit here the fact that mTokenizer won't live longer than this, as you > said before. I saw that but didn't understand the machinations of nsMultiMixedConv well enough to be confident that Finish would always be called before the last non-loop reference to nsMultiMixedConv was dropped unconditionally, so I wanted to avoid that leak and didn't make that change. > There is also move argument capture like: > > RefPtr<T> self = this; > [self = Move(self)]() -> ... > > but that is c++14 specific which is only avail on windows, AFAIK. I'm aware of that and most of the places where we capture `self` I would want to use this, but we can't right now unfortunately as you noted because we don't support C++14 features yet on all of our compilers.
Assignee | ||
Comment 41•7 years ago
|
||
Comment on attachment 8833537 [details] [diff] [review] Part 1: Correct the RefCountedInsideLambda Check to complain about capturing and using `this` without a backing strong reference [Security approval request comment] How easily could an exploit be constructed based on the patch? I imagine that this patch would point out potential exploits, but as far as I know, this patch doesn't exactly paint a complete picture of how one would perform the exploit. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? It is very easy to see what the problem being fixed is by looking at the contents of the patches. Which older supported branches are affected by this flaw? 45esr, 61, 52, 53 If not all supported branches, which bug introduced the flaw? N/A Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Yes - they are attached on this bug. How likely is this patch to cause regressions; how much testing does it need? This patch is fairly unlikely to cause regressions.
Attachment #8833537 -
Flags: sec-approval?
Assignee | ||
Updated•7 years ago
|
Attachment #8833509 -
Flags: sec-approval?
Assignee | ||
Updated•7 years ago
|
Attachment #8833510 -
Flags: sec-approval?
Assignee | ||
Updated•7 years ago
|
Attachment #8833511 -
Flags: sec-approval?
Assignee | ||
Updated•7 years ago
|
Attachment #8837289 -
Flags: sec-approval?
Assignee | ||
Comment 42•7 years ago
|
||
Comment on attachment 8833537 [details] [diff] [review] Part 1: Correct the RefCountedInsideLambda Check to complain about capturing and using `this` without a backing strong reference Approval Request Comment [Feature/Bug causing the regression]: N/A [User impact if declined]: Potential Security Holes [Is this code covered by automated tests?]: This is a series of systematic changes to existing changes, so yes and no. [Has the fix been verified in Nightly?]: sec-approval? is currently pending on the nightly versions of these patches. [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: Series of systematic changes which elimates potential use-after-free errors. [String changes made/needed]: None
Attachment #8833537 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•7 years ago
|
Attachment #8833509 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•7 years ago
|
Attachment #8833510 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•7 years ago
|
Attachment #8834559 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•7 years ago
|
Attachment #8834562 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 43•7 years ago
|
||
Comment on attachment 8837289 [details] [diff] [review] Part 5: Workaround lambda static analysis in nsMultiMixedConv My understanding is that the changes which caused this change to be required were uplifted, and thus are needed on all branches.
Attachment #8837289 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 44•7 years ago
|
||
Comment on attachment 8834614 [details] [diff] [review] Part 1: BETA Correct the RefCountedInsideLambda Check to complain about capturing and using `this` without a backing strong reference Approval Request Comment [Feature/Bug causing the regression]: N/A [User impact if declined]: Potential Security Holes [Is this code covered by automated tests?]: This is a series of systematic changes to existing changes, so yes and no. [Has the fix been verified in Nightly?]: sec-approval? is currently pending on the nightly versions of these patches. [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: Series of systematic changes which elimates potential use-after-free errors. [String changes made/needed]: None
Attachment #8834614 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•7 years ago
|
Attachment #8834615 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•7 years ago
|
Attachment #8834616 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•7 years ago
|
Attachment #8834617 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•7 years ago
|
Attachment #8834618 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•7 years ago
|
Attachment #8837289 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 45•7 years ago
|
||
Comment on attachment 8836217 [details] [diff] [review] Part 1: ESR45 Correct the RefCountedInsideLambda Check to complain about capturing and using `this` without a backing strong reference [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high bugh User impact if declined: Potential Security Holes Fix Landed on Version: Currently awaiting sec-approval and uplifts to beta, aurora, and nightly. Risk to taking this patch (and alternatives if risky): Low risk - small number of changes which are mechanical and low risk. String or UUID changes made by this patch: None. See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8836217 -
Flags: approval-mozilla-esr45?
Assignee | ||
Updated•7 years ago
|
Attachment #8836218 -
Flags: approval-mozilla-esr45?
Comment 46•7 years ago
|
||
OMG, so many flags. sec-approval+ for trunk. You only need to request sec-approval? on a single trunk patch in the future. :-) I'll approve aurora and beta as well, leaving ESR45 to release management but we should take it there.
tracking-firefox52:
--- → +
tracking-firefox53:
--- → +
tracking-firefox54:
--- → +
tracking-firefox-esr45:
--- → 52+
Updated•7 years ago
|
Attachment #8833509 -
Flags: sec-approval?
Attachment #8833509 -
Flags: sec-approval+
Attachment #8833509 -
Flags: approval-mozilla-aurora?
Attachment #8833509 -
Flags: approval-mozilla-aurora+
Updated•7 years ago
|
Attachment #8833510 -
Flags: sec-approval?
Attachment #8833510 -
Flags: sec-approval+
Attachment #8833510 -
Flags: approval-mozilla-aurora?
Attachment #8833510 -
Flags: approval-mozilla-aurora+
Updated•7 years ago
|
Attachment #8833511 -
Flags: sec-approval? → sec-approval+
Updated•7 years ago
|
Attachment #8833537 -
Flags: sec-approval?
Attachment #8833537 -
Flags: sec-approval+
Attachment #8833537 -
Flags: approval-mozilla-aurora?
Attachment #8833537 -
Flags: approval-mozilla-aurora+
Updated•7 years ago
|
Attachment #8834559 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•7 years ago
|
Attachment #8834562 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•7 years ago
|
Attachment #8834614 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
Attachment #8834615 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
Attachment #8834616 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
Attachment #8834617 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
Attachment #8834618 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
Attachment #8837289 -
Flags: sec-approval?
Attachment #8837289 -
Flags: sec-approval+
Attachment #8837289 -
Flags: approval-mozilla-beta?
Attachment #8837289 -
Flags: approval-mozilla-beta+
Attachment #8837289 -
Flags: approval-mozilla-aurora?
Attachment #8837289 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 47•7 years ago
|
||
(In reply to Al Billings [:abillings] from comment #46) > OMG, so many flags. Sorry :S - I couldn't remember if I needed to request it for each patch, so I just went through and requested all (19?) flags. I imagine that it was a pain to clear all of them. It was a pain to request them so :P > sec-approval+ for trunk. Thanks
This had build failures like https://treeherder.mozilla.org/logviewer.html#?job_id=77706219&repo=mozilla-inbound on some platforms, and mass test bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=77710882&repo=mozilla-inbound on others, so all backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b1d2452c7c17
Flags: needinfo?(michael)
Comment 49•7 years ago
|
||
Once this lands, it should fix bug 1339933 as a side-effect.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(michael)
Assignee | ||
Comment 50•7 years ago
|
||
I've managed to get everything green again - it turned out that there were 3 main problems: 1) There was a small amount of windows & OS X specific code which failed the static analysis. I have written a patch for each. 2) CamerasChild is not safe to addref everywhere, and the callsites which I modified blocked their calling scope anyway for the response to return, so I would run into problems due to AddRefing CamerasChild on the wrong thread when I didn't need to. 3) There was a lifecycle problem with the decoder which meant that firefox would hang on shutdown if I took a (another unnecessary) reference in the Init() function. These should be fixed in the 4 patches which I am attaching. MozReview-Commit-ID: 8b5KK7sL6wb
Attachment #8838692 -
Flags: review?(jwwang)
Assignee | ||
Comment 51•7 years ago
|
||
MozReview-Commit-ID: B4dYo4ETzkL
Attachment #8838694 -
Flags: review?(aklotz)
Assignee | ||
Comment 52•7 years ago
|
||
MozReview-Commit-ID: BHWJMHNgdRu
Attachment #8838695 -
Flags: review?(schien)
Assignee | ||
Comment 53•7 years ago
|
||
MozReview-Commit-ID: ElH27usjxlj
Attachment #8838696 -
Flags: review?(jwwang)
Comment 54•7 years ago
|
||
Comment on attachment 8838696 [details] [diff] [review] Part 9: Avoid MediaDecoder shutdown hang due to unnecessary strong reference capture Review of attachment 8838696 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderStateMachine.cpp @@ +2807,2 @@ > mOnMediaNotSeekable = mReader->OnMediaNotSeekable().Connect( > + OwnerThread(), this, &MediaDecoderStateMachine::SetMediaNotSeekable); i dont see how that could make a difference, as the two conde are in effect identical: https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaEventSource.h#433 this continues to the same MediaEventSource::ConnectInternal, which store this into a RefPtr and capture it.
Comment 55•7 years ago
|
||
doh, forget my earlier comment, it's RawPtr...
Comment 56•7 years ago
|
||
Comment on attachment 8838694 [details] [diff] [review] Part 7: Stop capturing this by value in windows-only code Review of attachment 8838694 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/mscom/MainThreadHandoff.cpp @@ +12,5 @@ > #include "mozilla/mscom/Utils.h" > #include "mozilla/Assertions.h" > #include "mozilla/DebugOnly.h" > #include "nsThreadUtils.h" > +#include "nsProxyRelease.h" nit: please move this up a line to keep the includes in alphabetical order. ::: ipc/mscom/WeakRef.cpp @@ +11,5 @@ > #include "mozilla/DebugOnly.h" > #include "mozilla/RefPtr.h" > #include "nsThreadUtils.h" > #include "nsWindowsHelpers.h" > +#include "nsProxyRelease.h" nit: please move this up two lines to keep the includes in alphabetical order.
Attachment #8838694 -
Flags: review?(aklotz) → review+
Assignee | ||
Comment 57•7 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #54) > this continues to the same MediaEventSource::ConnectInternal, which store > this into a RefPtr and capture it. It captures it in a RawPtr, not a RefPtr. a RawPtr is a wrapper type which they use to bypass this static analysis. https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/dom/media/MediaEventSource.h#143-154
Comment 58•7 years ago
|
||
Comment on attachment 8838696 [details] [diff] [review] Part 9: Avoid MediaDecoder shutdown hang due to unnecessary strong reference capture Review of attachment 8838696 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderStateMachine.cpp @@ +2807,2 @@ > mOnMediaNotSeekable = mReader->OnMediaNotSeekable().Connect( > + OwnerThread(), this, &MediaDecoderStateMachine::SetMediaNotSeekable); It is different. https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/dom/media/MediaEventSource.h#144 RawPtr allows capturing a ref-counting pointer without causing SA errors.
Attachment #8838696 -
Flags: review?(jwwang) → review+
Comment 59•7 years ago
|
||
FWIW, RawPtr sounds very worrisome class to have.
Comment 60•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #59) > FWIW, RawPtr sounds very worrisome class to have. Perhaps the template for RawPtr should be something like template<typename T, const char* ReasonThisIsSafeToUseWithProof> class RawPtr... 1/2 :-)
Comment 61•7 years ago
|
||
Comment on attachment 8838692 [details] [diff] [review] Part 6: Avoid AddRefing/Releasing CamerasChild on the wrong thread Review of attachment 8838692 [details] [diff] [review]: ----------------------------------------------------------------- Forward CamerasChild.cpp to Randell.
Attachment #8838692 -
Flags: review?(rjesup)
Attachment #8838692 -
Flags: review?(jwwang)
Attachment #8838692 -
Flags: review+
Comment 62•7 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #60) > (In reply to Olli Pettay [:smaug] from comment #59) > > FWIW, RawPtr sounds very worrisome class to have. > > Perhaps the template for RawPtr should be something like > template<typename T, const char* ReasonThisIsSafeToUseWithProof> class > RawPtr... > 1/2 :-) detail::RawPtr is for internal use only and shouldn't be a concern of the client code. MediaEventSource asserts the listener is disconnect before deletion to ensure no dangling pointers are used.
Comment 63•7 years ago
|
||
Comment on attachment 8838692 [details] [diff] [review] Part 6: Avoid AddRefing/Releasing CamerasChild on the wrong thread Review of attachment 8838692 [details] [diff] [review]: ----------------------------------------------------------------- I didn't search down through all the fun layers of calls, but my only concern is the code that in the Lambda version went out of it's way to convert the bool returns from SendFoo() to nsresult returns... which is now gone. I want to ensure that all the failure paths for broken IPC/etc still work correctly without it. If that's ok, great. Forwarding review to gcp who wrote this
Attachment #8838692 -
Flags: review?(rjesup) → review?(gpascutto)
Comment 64•7 years ago
|
||
Comment on attachment 8838695 [details] [diff] [review] Part 8: Stop capturing this by value in OSX only code Review of attachment 8838695 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for catching this one. :)
Attachment #8838695 -
Flags: review?(schien) → review+
Comment 65•7 years ago
|
||
Comment on attachment 8838692 [details] [diff] [review] Part 6: Avoid AddRefing/Releasing CamerasChild on the wrong thread Review of attachment 8838692 [details] [diff] [review]: ----------------------------------------------------------------- It doesn't look like there's anything that checks for the failures of the Send* methods. If the IPC channel is so broken, looks like we're pretty hosed anyway (child might hang). https://dxr.mozilla.org/mozilla-central/rev/a180b976c165b6cd174d24bbb77941919cdc53cb/dom/media/systemservices/CamerasChild.cpp#262 So I think that losing the return values is OK, assuming this has been properly tested etc etc.
Attachment #8838692 -
Flags: review?(gpascutto) → review+
Assignee | ||
Comment 66•7 years ago
|
||
Pushed all parts except part 1 to central in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=419ada2f9e81fdabcd3423381990231bcfb61c0c
I had to back these all out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=79788816&repo=mozilla-inbound on mn-e10s on Windows debug https://hg.mozilla.org/integration/mozilla-inbound/rev/3fee518242f6
Flags: needinfo?(michael)
Also appears to have caused some win debug accessibility failures like https://treeherder.mozilla.org/logviewer.html#?job_id=79729982&repo=mozilla-inbound The job might show as (a) or (a11y) depending on when you push related to the most recent treeherder deployment.
Comment 69•7 years ago
|
||
Go ahead and re-land with the affected assertion removed. It is of limited use anyway. r=me
Assignee | ||
Comment 70•7 years ago
|
||
MozReview-Commit-ID: Fx7CmxgiJpo
Assignee | ||
Comment 71•7 years ago
|
||
Re-landing with the change suggested by aklotz in https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=57c46960a5f3
Flags: needinfo?(michael)
https://hg.mozilla.org/mozilla-central/rev/57c46960a5f3 https://hg.mozilla.org/mozilla-central/rev/2b47a14041f5 https://hg.mozilla.org/mozilla-central/rev/9730f8b59968 https://hg.mozilla.org/mozilla-central/rev/54cc45b7dd55 https://hg.mozilla.org/mozilla-central/rev/b4cb7a1e84a8 https://hg.mozilla.org/mozilla-central/rev/a545bcb544a7 https://hg.mozilla.org/mozilla-central/rev/62ea86b5a4e2 https://hg.mozilla.org/mozilla-central/rev/e83d73a055fe https://hg.mozilla.org/mozilla-central/rev/a15e49742ebd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 73•7 years ago
|
||
Comment on attachment 8836217 [details] [diff] [review] Part 1: ESR45 Correct the RefCountedInsideLambda Check to complain about capturing and using `this` without a backing strong reference Fix a sec-critical. ESR45+.
Attachment #8836217 -
Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Updated•7 years ago
|
Attachment #8836218 -
Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Comment 74•7 years ago
|
||
has not landed in beta or aurora so far, do we still want this in esr45 when 52 is just about to go out of the door soon ?
Flags: needinfo?(jcristau)
Flags: needinfo?(gchang)
Comment 75•7 years ago
|
||
Given that this has already landed and stuck on m-c, I personally think we need this to land before we ship 52. But I'm also not sure where we stand on the branch patches with respect to whether they need rebasing or not. If Julian is on board, I would suggest that Michael rebase the branch patches and land them himself today still. But yeah, I definitely don't think we want this landing on ESR45 in the mean time.
Flags: needinfo?(michael)
Comment 76•7 years ago
|
||
Yes, probably not a good idea to leave a sec-crit fixed in trunk but not release for 6 weeks, so we should get some clarity on whether the branch patches here are still good to go and if possible land on aurora and beta today.
Flags: needinfo?(jcristau)
Assignee | ||
Comment 77•7 years ago
|
||
This fixes the last of the failures I was seeing on beta. I talked with jwwang on irc about this and made the change and he said that it was safe. MozReview-Commit-ID: Ll32AzpBXHo
Assignee | ||
Comment 78•7 years ago
|
||
uplift |
Landed in beta: https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=cd0313192866
Flags: needinfo?(michael)
Comment 79•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-release/rev/cd0313192866 https://hg.mozilla.org/releases/mozilla-release/rev/e7295c953254 https://hg.mozilla.org/releases/mozilla-release/rev/f3bc9bc70e8a https://hg.mozilla.org/releases/mozilla-release/rev/16c34293786b https://hg.mozilla.org/releases/mozilla-release/rev/0e8fd2021c04 https://hg.mozilla.org/releases/mozilla-release/rev/320a347cb290 https://hg.mozilla.org/releases/mozilla-release/rev/d202cd98eaae https://hg.mozilla.org/releases/mozilla-release/rev/22d64241953f https://hg.mozilla.org/releases/mozilla-release/rev/992c5ca5d919 https://hg.mozilla.org/releases/mozilla-release/rev/f6462becdfe1 https://hg.mozilla.org/releases/mozilla-esr52/rev/cd0313192866 https://hg.mozilla.org/releases/mozilla-esr52/rev/e7295c953254 https://hg.mozilla.org/releases/mozilla-esr52/rev/f3bc9bc70e8a https://hg.mozilla.org/releases/mozilla-esr52/rev/16c34293786b https://hg.mozilla.org/releases/mozilla-esr52/rev/0e8fd2021c04 https://hg.mozilla.org/releases/mozilla-esr52/rev/320a347cb290 https://hg.mozilla.org/releases/mozilla-esr52/rev/d202cd98eaae https://hg.mozilla.org/releases/mozilla-esr52/rev/22d64241953f https://hg.mozilla.org/releases/mozilla-esr52/rev/992c5ca5d919 https://hg.mozilla.org/releases/mozilla-esr52/rev/f6462becdfe1
Updated•7 years ago
|
Assignee | ||
Comment 80•7 years ago
|
||
Landed in ESR: https://hg.mozilla.org/releases/mozilla-esr45/pushloghtml?changeset=eec69810d80e and aurora: https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=101239f350b5
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•7 years ago
|
Updated•7 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+][adv-esr45.8+]
Assignee | ||
Comment 81•7 years ago
|
||
part 1 landing on trunk: https://hg.mozilla.org/integration/mozilla-inbound/rev/ed9b6ec1a0e8789b793bccf1426113e1c78f9ad5
Updated•7 years ago
|
Group: dom-core-security → core-security-release
Updated•7 years ago
|
Group: core-security-release
Updated•6 years ago
|
Product: Core → Firefox Build System
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•