Closed
Bug 1153304
Opened 9 years ago
Closed 9 years ago
prohibit refcounted things from being captured by C++ lambdas unless the lambdas refcount them
Categories
(Developer Infrastructure :: Source Code Analysis, defect)
Developer Infrastructure
Source Code Analysis
Tracking
(firefox40 fixed)
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: froydnj, Assigned: ehsan.akhgari)
References
(Regressed 1 open bug)
Details
Attachments
(1 file)
10.21 KB,
patch
|
jrmuizel
:
review+
jesup
:
review-
|
Details | Diff | Splinter Review |
Lambdas can be dangerous to use with refcounted things, because you can capture a refcounted thing for later use without holding a reference to it, which leads to a UAF. We have these problems in other contexts, but it ought to be feasible to prevent them from happening with lambdas by adding appropriate smarts to the static analyzer.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8591179 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 2•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3624fe11ae0c
Flags: needinfo?(ehsan)
Comment 3•9 years ago
|
||
Comment on attachment 8591179 [details] [diff] [review] Add an analysis to prohibit the usage of pointers to refcounted types inside C++ lambdas Review of attachment 8591179 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by: there is a comment in MediaManager about intentionally leaking the runnable just above the line you modified, which I think will make it stop leaking.
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8591179 [details] [diff] [review] Add an analysis to prohibit the usage of pointers to refcounted types inside C++ lambdas Review of attachment 8591179 [details] [diff] [review]: ----------------------------------------------------------------- Good point. Chris, can you look this over, please? What should be done here?
Attachment #8591179 -
Flags: review?(cpearce)
Comment 5•9 years ago
|
||
Comment on attachment 8591179 [details] [diff] [review] Add an analysis to prohibit the usage of pointers to refcounted types inside C++ lambdas Review of attachment 8591179 [details] [diff] [review]: ----------------------------------------------------------------- MediaManager is jesup's territory...
Attachment #8591179 -
Flags: review?(cpearce) → review?(rjesup)
Comment 6•9 years ago
|
||
Comment on attachment 8591179 [details] [diff] [review] Add an analysis to prohibit the usage of pointers to refcounted types inside C++ lambdas Review of attachment 8591179 [details] [diff] [review]: ----------------------------------------------------------------- kats is correct - this will lead to release-of-mainthread-only objects on non-mainthread (in bizarre situations, but still...) Either don't do this and use some way to suppress the static analysis warning, or if that blocks landing the static analysis code, I suggest rewriting the failure cases involving p/etc to either a) purposely leak by something like (void) runnable.forget(); or add some variant to send the OnSuccess/OnFailure references in the runnable to MainThread to be Release()ed.
Attachment #8591179 -
Flags: review?(rjesup) → review-
Comment 8•9 years ago
|
||
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #0) > Lambdas can be dangerous to use with refcounted things, because you can > capture a refcounted thing for later use without holding a reference to it, > which leads to a UAF. I don't see how lambdas are inherently more dangerous than say runnables, or just about any object with initialized members. Lambda capture is by copy by default, and nsRefPtr's copy-constructor seems to addRef just fine [1]. Of course *pointers* are dangerous to use, but that's true in almost any context, and not exclusive to reference counted targets either, so why pick on lambdas? Can someone point me to the rationale for the static analysis? [1] http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsRefPtr.h?rev=d30eb154f8fa#72
Flags: needinfo?(jib)
Comment 9•9 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #6) > Either don't do this FWIW the pattern of holding runnables in raw pointers is established in NS_DispatchToMainThread [1]. [1] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.cpp?mark=164-168#164
Assignee | ||
Comment 10•9 years ago
|
||
The rationale is on dev-platform under the thread "Proposal to ban the usage of refcounted objects inside C++ lambdas in Gecko". The reason to pick up on lambdas is that they're a new C++ feature without too many existing places where we use them (1 is affected by this issue right now, as can be seen in the patch), so it's better to protect against the upcoming in flux on sec-critical bugs before we end up shipping them.
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #6) > Comment on attachment 8591179 [details] [diff] [review] > Add an analysis to prohibit the usage of pointers to refcounted types inside > C++ lambdas > > Review of attachment 8591179 [details] [diff] [review]: > ----------------------------------------------------------------- > > kats is correct - this will lead to release-of-mainthread-only objects on > non-mainthread (in bizarre situations, but still...) If that's the reason, why not just NS_ProxyRelease the success and failure callback objects on the main thread? There is no need to leak the object just because we're on the wrong thread when its last reference is dropped.
Flags: needinfo?(rjesup)
Comment 12•9 years ago
|
||
> > kats is correct - this will lead to release-of-mainthread-only objects on
> > non-mainthread (in bizarre situations, but still...)
>
> If that's the reason, why not just NS_ProxyRelease the success and failure
> callback objects on the main thread? There is no need to leak the object
> just because we're on the wrong thread when its last reference is dropped.
First, ProxyRelease can fail too, and then we'd need to intentionally leak anyways
Second, if aTarget is no longer gettable for some reason, ProxyRelease will release on the current thread (instead it should leak; I consider this a bug as it explicitly avoids this for Dispatch() failures). However, this is somewhat neither here nor there.
Third, yes, you could do ProxyRelease if you can recode it such that on a failure (and only on a failure) we ProxyRelease the (nsRefPtr'd) runnable instead of just letting it fall out of scope. I'm unfamiliar enough with the way Promises and lambdas intact here in c++ land to be sure how to do that reliably. Also, the first if (mPrivileged) would need to test NS_DispatchToMainThread for failure and leak if it failed (since ProxyRelease would likely fail too).
Smacks of "needs helper class", which kinda kills the appeal of lambdas I'd guess. Maybe just a helper that owns the runnable and handles dispatching the lambda or some such.
Fourth: not sure how lambdas work enough to be sure, but if we could pass the lambda a forgotten runnable, then what happens? Would that "fix" the static analysis thing and let it safely leak?
Smacks me as a lot of work to avoid a purposeful leak in a likely-only-in-weird-shutdown case. And code with a lot of fancy paths to handle leak-avoidance in the process of shutdown actually can increase security risks; if you're in shutdown and not involved in saving state to disk, leaking things can actually be a generally safer thing to do.
Other lambdas may be different, and have reasonable failure causes when in (non-shutdown)/normal operation, and so need to ensure refcounting. And the issues here only show up because it needs to own MainThreadOnly references.
Flags: needinfo?(rjesup)
Assignee | ||
Comment 13•9 years ago
|
||
I moved the WebRTC bit to bug 1154389. Jeff, can you please review the rest? I'd like to land this ASAP. Thanks!
Flags: needinfo?(jmuizelaar)
Updated•9 years ago
|
Attachment #8591179 -
Flags: review?(jmuizelaar) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f812e2a5d5e9 for static analysis build failure: https://treeherder.mozilla.org/logviewer.html#?job_id=8847436&repo=mozilla-inbound
Flags: needinfo?(ehsan)
Assignee | ||
Comment 15•9 years ago
|
||
Pushed a fixed version.
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(ehsan)
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c606088b70aa https://hg.mozilla.org/mozilla-central/rev/0b226fae7a45
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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
•