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)

defect
Not set
normal

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)

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: nobody → ehsan
Flags: needinfo?(ehsan)
Blocks: 1152753
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.
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 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 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-
Adding jib since he wrote the code in question
Flags: needinfo?(jib)
(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)
(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
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.
(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)
> > 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)
Depends on: 1154389
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)
Attachment #8591179 - Flags: review?(jmuizelaar) → review+
Pushed a fixed version.
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(ehsan)
https://hg.mozilla.org/mozilla-central/rev/c606088b70aa
https://hg.mozilla.org/mozilla-central/rev/0b226fae7a45
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1170388
Product: Core → Firefox Build System
Regressions: 1421435
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: