Proposal to ban the usage of refcounted objects inside C++ lambdas in Gecko

[Ehsan Akhgari]

I would like to propose that we should ban the usage of refcounted objects
inside lambdas in Gecko. Here is the reason:

Consider the following code:

nsINode* myNode;
TakeLambda([&]() {
myNode->Foo();
});

There is nothing that guarantees that the lambda passed to TakeLambda is
executed before myNode is destroyed somehow. While it's possible to verify
on a case by case basis that this code is indeed safe, I think it's too
error prone to keep these invariants over time. Given the fact that if
things go wrong we'd deal with a UAF bug which will be sec-critical, I
think it's better to be safe than sorry here.

If nobody objects, I'll update the style guide in the next few days. If
you do object, please make a case for why the above analysis is incorrect
and why the usage of refcounted objects in lambdas in general is safe.

Cheers,
--
Ehsan

[Markus Stange]

On 2015-04-10 11:46 AM, Ehsan Akhgari wrote:
> If nobody objects, I'll update the style guide in the next few days. If
> you do object

I object to the the formulation of this guideline. If you change it to
"usage of raw pointers to refcounted objects", I retract the objection.
I'd even be fine with banning the use of [&].

But I'd like the guideline to clarify that there is nothing wrong with doing

nsCOMPtr<nsINode> myNode;
TakeLambda([=]() { ... });

-Markus

[Ehsan Akhgari]

This pattern is not affected by the UAF risk, but the issue there is
maintaining myNode as an nsCOMPtr. IOW, if in the future someone
changes myNode into a raw nsINode*, the same issue occurs.

If we adopt an analysis that turns the usage of raw refcounted pointers
inside lambdas in the body of lambdas, I think I would be fine with
relaxing this to allow the above case.

[Andrew McCreight]

On Fri, Apr 10, 2015 at 8:46 AM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

> I would like to propose that we should ban the usage of refcounted objects
> inside lambdas in Gecko. Here is the reason:
>
> Consider the following code:
>
> nsINode* myNode;
> TakeLambda([&]() {
> myNode->Foo();
> });
>

How is this any worse than any other usage of raw pointers on the stack?

nsINode* myNode;
doSomething();
myNode->Foo();

is the same thing.

I also don't see how "don't use any refcounted objects inside lambdas" is
any easier to enforce.

I also don't see why refcounted objects in particular are worth of
exclusion based on their lifetime in closures, but not references to other
allocated things. I guess we don't have local variables like
|nsTArray<Foo>* myArray;| as often, but that would be just as dangerous.

Andrew

> There is nothing that guarantees that the lambda passed to TakeLambda is
> executed before myNode is destroyed somehow. While it's possible to verify
> on a case by case basis that this code is indeed safe, I think it's too
> error prone to keep these invariants over time. Given the fact that if
> things go wrong we'd deal with a UAF bug which will be sec-critical, I
> think it's better to be safe than sorry here.
>

> If nobody objects, I'll update the style guide in the next few days. If

> you do object, please make a case for why the above analysis is incorrect
> and why the usage of refcounted objects in lambdas in general is safe.
>
> Cheers,
> --
> Ehsan

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[David Rajchenbach-Teller]

Just to be sure I understand: how is this worse than using
non-refcounted-but-manually-freed objects in a lambda?

Also, what is the alternative? Acquiring a nsCOMPtr/nsRefPtr inside the
Lambda constructor (or whatever it's called)?

On 10/04/15 17:46, Ehsan Akhgari wrote:
> I would like to propose that we should ban the usage of refcounted objects
> inside lambdas in Gecko. Here is the reason:
>
> Consider the following code:
>
> nsINode* myNode;
> TakeLambda([&]() {
> myNode->Foo();
> });
>

> There is nothing that guarantees that the lambda passed to TakeLambda is
> executed before myNode is destroyed somehow. While it's possible to verify
> on a case by case basis that this code is indeed safe, I think it's too
> error prone to keep these invariants over time. Given the fact that if
> things go wrong we'd deal with a UAF bug which will be sec-critical, I
> think it's better to be safe than sorry here.
>
> If nobody objects, I'll update the style guide in the next few days. If
> you do object, please make a case for why the above analysis is incorrect
> and why the usage of refcounted objects in lambdas in general is safe.
>
> Cheers,
>


--

David Rajchenbach-Teller, PhD
Performance Team, Mozilla

[smaug]

On 04/10/2015 07:33 PM, Andrew McCreight wrote:
> On Fri, Apr 10, 2015 at 8:46 AM, Ehsan Akhgari <ehsan....@gmail.com>
> wrote:
>
>> I would like to propose that we should ban the usage of refcounted objects
>> inside lambdas in Gecko. Here is the reason:
>>
>> Consider the following code:
>>
>> nsINode* myNode;
>> TakeLambda([&]() {
>> myNode->Foo();
>> });
>>
>
> How is this any worse than any other usage of raw pointers on the stack?

Ehsan's example misses the reason why we started to talk about this:
What if TakeLambda processes the lambda asynchronously. Lambda hides very effectively that
nothing is keeping myNode alive while event loop spins.


-Olli

[Ehsan Akhgari]

On 2015-04-10 12:33 PM, Andrew McCreight wrote:
>
>
> On Fri, Apr 10, 2015 at 8:46 AM, Ehsan Akhgari <ehsan....@gmail.com
> <mailto:ehsan....@gmail.com>> wrote:
>
> I would like to propose that we should ban the usage of refcounted
> objects
> inside lambdas in Gecko. Here is the reason:
>
> Consider the following code:
>
> nsINode* myNode;
> TakeLambda([&]() {
> myNode->Foo();
> });
>
>
> How is this any worse than any other usage of raw pointers on the stack?
>

> nsINode* myNode;
> doSomething();
> myNode->Foo();
>
> is the same thing.

It is worse since the lambda may be held on to and executed later in the
future (i.e., past the point in time when the function returns.)

But also do note that raw pointers to refcounted objects on the stack
are very dangerous as well (for example in your above code,
doSomething() could destroy myNode.

> I also don't see how "don't use any refcounted objects inside lambdas"
> is any easier to enforce.

It's easier because we don't yet have piles of code which have this
pattern in them. I would like to prevent us getting there so that we
don't have to start solving this issue on a massive scale in a few years
once we grow more lambda usage.

> I also don't see why refcounted objects in particular are worth of
> exclusion based on their lifetime in closures, but not references to
> other allocated things. I guess we don't have local variables like
> |nsTArray<Foo>* myArray;| as often, but that would be just as dangerous.

Yes, I have nothing against coming up with further restrictions to make
more things safe, but mentioning other dangerous things is not an
argument against this proposal. ;-)

[Nicolas B. Pierron]

On 04/10/2015 07:02 PM, David Rajchenbach-Teller wrote:
> Just to be sure I understand: how is this worse than using
> non-refcounted-but-manually-freed objects in a lambda?

This is worse because you have no idea when the code of the lambda would be
executed. There is no longer any enforcement on the order between the
count-down (destructor), and the execution of the code.

> Also, what is the alternative? Acquiring a nsCOMPtr/nsRefPtr inside the
> Lambda constructor (or whatever it's called)?

Yes, another option would be to ensure that the lambda cannot be used after
a specific point.

nsINode* myNode;
auto callFoo = MakeScopedLambda([&]() {
myNode->Foo();
})
TakeLambda(callFoo);

Any reference to the lambda after the end of the innermost scope where
MakeScopedLambda is used can cause a MOZ_CRASH.

> On 10/04/15 17:46, Ehsan Akhgari wrote:
>> I would like to propose that we should ban the usage of refcounted objects
>> inside lambdas in Gecko. Here is the reason:
>>
>> Consider the following code:
>>
>> nsINode* myNode;
>> TakeLambda([&]() {
>> myNode->Foo();
>> });

--
Nicolas B. Pierron

[Ehsan Akhgari]

On 2015-04-10 1:02 PM, David Rajchenbach-Teller wrote:
> Just to be sure I understand: how is this worse than using
> non-refcounted-but-manually-freed objects in a lambda?

In a lot of cases, it's _impossible_ (and I mean really impossible, not
just very difficult!) to know exactly when a refcounted object gets
destroyed, which makes this issue worse for those types of objects.

> Also, what is the alternative? Acquiring a nsCOMPtr/nsRefPtr inside the
> Lambda constructor (or whatever it's called)?

I think a better alternative would be to have a closure class with an
nsCOMPtr/nsRefPtr member pointing to the refcounted object, or at least
capturing the smart pointers by value (see my reply to Markus where he
brought this up as well.)

> On 10/04/15 17:46, Ehsan Akhgari wrote:
>> I would like to propose that we should ban the usage of refcounted objects
>> inside lambdas in Gecko. Here is the reason:
>>
>> Consider the following code:
>>
>> nsINode* myNode;
>> TakeLambda([&]() {
>> myNode->Foo();
>> });
>>

[Ehsan Akhgari]

On 2015-04-10 1:41 PM, Nicolas B. Pierron wrote:
>> Also, what is the alternative? Acquiring a nsCOMPtr/nsRefPtr inside the
>> Lambda constructor (or whatever it's called)?
>
> Yes, another option would be to ensure that the lambda cannot be used
> after a specific point.
>
> nsINode* myNode;
> auto callFoo = MakeScopedLambda([&]() {
> myNode->Foo();
> })
> TakeLambda(callFoo);
>
> Any reference to the lambda after the end of the innermost scope where
> MakeScopedLambda is used can cause a MOZ_CRASH.

How would you detect that at compile/run time?

[Seth Fowler]

> On Apr 10, 2015, at 8:46 AM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
>
> I would like to propose that we should ban the usage of refcounted objects
> inside lambdas in Gecko. Here is the reason:
>
> Consider the following code:
>
> nsINode* myNode;

> TakeLambda([&]() {
> myNode->Foo();

> });
>
> There is nothing that guarantees that the lambda passed to TakeLambda is
> executed before myNode is destroyed somehow.

I agree that this pattern is bad, but I don’t think that means that we should ban lambda capture of refcounted objects.

This alternative formulation would work just fine today, AFAIK:

> nsCOMPtr<nsINode> myNode;
> TakeLambda([=]() {

> myNode->Foo();
> });

This captures by value, so we end up with a copy of myNode in the lambda, with the refcount incremented appropriately.

Once we have C++14 support everywhere, we can also do this:

> nsCOMPtr<nsINode> myNode;
> TakeLambda([myNode = Move(myNode)]() {
> myNode->Foo();
> });

To capture by move (and avoid the cost of a refcount increment).

Using either of these approaches is enough to make refcounted objects safe to use in lambda capture expressions. Lambdas will be much less useful if they can’t capture refcounted objects, so I’m strongly against banning that.

- Seth

[Seth Fowler]

Ah, I see that Markus made the same point that I did. Didn’t see this branch of the thread before replying.

> On Apr 10, 2015, at 9:06 AM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
>
> This pattern is not affected by the UAF risk, but the issue there is maintaining myNode as an nsCOMPtr. IOW, if in the future someone changes myNode into a raw nsINode*, the same issue occurs.

Surely people don’t make that kind of change lightly? Doing that almost anywhere is quite risky.

> If we adopt an analysis that turns the usage of raw refcounted pointers inside lambdas in the body of lambdas, I think I would be fine with relaxing this to allow the above case.

I’m very supportive of a static analysis to make sure we catch bad usages.

- Seth

[Kyle Huey]

On Fri, Apr 10, 2015 at 11:09 AM, Seth Fowler <se...@mozilla.com> wrote:
>
>> On Apr 10, 2015, at 8:46 AM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
>>
>> I would like to propose that we should ban the usage of refcounted objects
>> inside lambdas in Gecko. Here is the reason:
>>
>> Consider the following code:
>>
>> nsINode* myNode;
>> TakeLambda([&]() {
>> myNode->Foo();
>> });
>>
>> There is nothing that guarantees that the lambda passed to TakeLambda is
>> executed before myNode is destroyed somehow.
>
> I agree that this pattern is bad, but I don’t think that means that we should ban lambda capture of refcounted objects.
>
> This alternative formulation would work just fine today, AFAIK:
>
>> nsCOMPtr<nsINode> myNode;
>> TakeLambda([=]() {
>> myNode->Foo();
>> });
>
> This captures by value, so we end up with a copy of myNode in the lambda, with the refcount incremented appropriately.

Unfortunately that distinction is pretty subtle, so we're likely to
make mistakes.

We definitely need some sort of automated analysis assisting us here.

- Kyle

[Seth Fowler]

> On Apr 10, 2015, at 11:14 AM, Kyle Huey <m...@kylehuey.com> wrote:
>
> Unfortunately that distinction is pretty subtle, so we're likely to
> make mistakes.

It’s the same difference as passing something to a function by value vs. by reference, so I’d hope C++ programmers could handle it. =) But it’s true that as a community we’re less experienced with lambda syntax.

> We definitely need some sort of automated analysis assisting us here.

Agreed. Trust, but verify.

- Seth

[Ehsan Akhgari]

On 2015-04-10 2:17 PM, Seth Fowler wrote:
>
>> On Apr 10, 2015, at 11:14 AM, Kyle Huey <m...@kylehuey.com

I'll write the analysis in bug 1153304. Let's not keep arguing this
part. :-) The analysis should be simple to write.

[Ehsan Akhgari]

On 2015-04-10 2:12 PM, Seth Fowler wrote:
> Ah, I see that Markus made the same point that I did. Didn’t see this branch of the thread before replying.
>
>> On Apr 10, 2015, at 9:06 AM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
>>
>> This pattern is not affected by the UAF risk, but the issue there is maintaining myNode as an nsCOMPtr. IOW, if in the future someone changes myNode into a raw nsINode*, the same issue occurs.
>
> Surely people don’t make that kind of change lightly? Doing that almost anywhere is quite risky.

I would like to believe you, but I don't even trust myself to never make
this mistake, so I can hardly believe the same for other people. But
this is moot with bug 1153304 anyway.

[Andrew McCreight]

On Fri, Apr 10, 2015 at 10:37 AM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

> On 2015-04-10 12:33 PM, Andrew McCreight wrote:
>
>>
>>

>> On Fri, Apr 10, 2015 at 8:46 AM, Ehsan Akhgari <ehsan....@gmail.com
>> <mailto:ehsan....@gmail.com>> wrote:
>>
>> I would like to propose that we should ban the usage of refcounted
>> objects
>> inside lambdas in Gecko. Here is the reason:
>>
>> Consider the following code:
>>
>> nsINode* myNode;
>> TakeLambda([&]() {
>> myNode->Foo();
>> });
>>
>>

>> How is this any worse than any other usage of raw pointers on the stack?
>>
>> nsINode* myNode;
>> doSomething();
>> myNode->Foo();
>>
>> is the same thing.
>>
>
> It is worse since the lambda may be held on to and executed later in the
> future (i.e., past the point in time when the function returns.)
>
> But also do note that raw pointers to refcounted objects on the stack are
> very dangerous as well (for example in your above code, doSomething() could
> destroy myNode.
>
> I also don't see how "don't use any refcounted objects inside lambdas"
>> is any easier to enforce.
>>
>
> It's easier because we don't yet have piles of code which have this
> pattern in them. I would like to prevent us getting there so that we don't
> have to start solving this issue on a massive scale in a few years once we
> grow more lambda usage.
>
> I also don't see why refcounted objects in particular are worth of
>> exclusion based on their lifetime in closures, but not references to
>> other allocated things. I guess we don't have local variables like
>> |nsTArray<Foo>* myArray;| as often, but that would be just as dangerous.
>>
>
>
Yes, I have nothing against coming up with further restrictions to make
> more things safe, but mentioning other dangerous things is not an argument
> against this proposal. ;-)
>

Well, I don't really see what is difficult about this in particular, but
I've been writing code in functional programming languages for a long time,
so maybe I'm just used to the lifetime issues around closures already.

[smaug]

I'd say that is rather painful for reviewers, since both Move() (I prefer .swap()) and lambda hide what is actually happening to the refcnt.
So easy to forget to use nsCOMPtr explicitly there.

We should emphasize easy-to-read-and-understand code over fast-to-write.




-Olli



>
> - Seth
>

[Gregory Szorc]

On Fri, Apr 10, 2015 at 11:46 AM, Ehsan Akhgari <ehsan....@gmail.com>

wrote:

> I would like to propose that we should ban the usage of refcounted objects
> inside lambdas in Gecko. Here is the reason:
>
> Consider the following code:
>
> nsINode* myNode;
> TakeLambda([&]() {
> myNode->Foo();
> });
>
> There is nothing that guarantees that the lambda passed to TakeLambda is

> executed before myNode is destroyed somehow. While it's possible to verify
> on a case by case basis that this code is indeed safe, I think it's too
> error prone to keep these invariants over time. Given the fact that if
> things go wrong we'd deal with a UAF bug which will be sec-critical, I
> think it's better to be safe than sorry here.
>
> If nobody objects, I'll update the style guide in the next few days. If
> you do object, please make a case for why the above analysis is incorrect
> and why the usage of refcounted objects in lambdas in general is safe.
>

Why do we merely update the style guide? We have a custom Clang compiler
plugin. I think we should have the Clang compiler plugin enforce our style
guidelines by refusing to compile code that is against policy. This would
enable reviewers to stop focusing on policing the style guide and enable
them to focus more on the correctness of code. This will result in faster
reviews and/or higher quality code.

We already do some of this with the existing Clang compiler plugin. But
it's far from comprehensive. I think we should strive for a world where
machines, not humans, are enforcing the style guide.

[Eric Shepherd]

> Gregory Szorc <mailto:g...@mozilla.com>
> April 10, 2015 at 9:43 PM

> On Fri, Apr 10, 2015 at 11:46 AM, Ehsan Akhgari <ehsan....@gmail.com>
>

> Why do we merely update the style guide? We have a custom Clang compiler
> plugin. I think we should have the Clang compiler plugin enforce our style
> guidelines by refusing to compile code that is against policy. This would
> enable reviewers to stop focusing on policing the style guide and enable
> them to focus more on the correctness of code. This will result in faster
> reviews and/or higher quality code.
>
> We already do some of this with the existing Clang compiler plugin. But
> it's far from comprehensive. I think we should strive for a world where
> machines, not humans, are enforcing the style guide.

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

> Ehsan Akhgari <mailto:ehsan....@gmail.com>
> April 10, 2015 at 11:46 AM

> I would like to propose that we should ban the usage of refcounted objects
> inside lambdas in Gecko. Here is the reason:
>
> Consider the following code:
>
> nsINode* myNode;
> TakeLambda([&]() {
> myNode->Foo();
> });
>
> There is nothing that guarantees that the lambda passed to TakeLambda is
> executed before myNode is destroyed somehow. While it's possible to verify
> on a case by case basis that this code is indeed safe, I think it's too
> error prone to keep these invariants over time. Given the fact that if
> things go wrong we'd deal with a UAF bug which will be sec-critical, I
> think it's better to be safe than sorry here.
>
> If nobody objects, I'll update the style guide in the next few days. If
> you do object, please make a case for why the above analysis is incorrect
> and why the usage of refcounted objects in lambdas in general is safe.
>

> Cheers,
> ------------------------------------------------------------------------
Ideally, this sounds like a great idea to me (says the tech writer who
isn't having to deal with this stuff directly in any significant way).
My concern would be impact on project timelines. Is this something that
would bust so many existing allocations/deallocations that things would
take weeks to bring up to snuff? What are the odds that in the process
of fixing these refcounted objects, new (and potentially insidiously
hard to spot) bugs are introduced?

Would it be possible to enforce this rule gradually, over time, by
enforcing it one or two modules (or other code areas) at a time?
--

Eric Shepherd
Senior Technical Writer
Mozilla <https://www.mozilla.org/>
Blog: http://www.bitstampede.com/
Twitter: http://twitter.com/sheppy

[Nicolas B. Pierron]

Simply by replacing the reference to the lambda inside callFoo at the end of
the scope, and replace it by a constructing a dummy function which expects
the same type of arguments as the lambda, but calls MOZ_CRASH instead.

I guess I would have wrote the auto as ScopedLambda<void (*)()> for the
example.

--
Nicolas B. Pierron

[Ehsan Akhgari]

On 2015-04-13 5:26 AM, Nicolas B. Pierron wrote:
> On 04/10/2015 07:47 PM, Ehsan Akhgari wrote:
>> On 2015-04-10 1:41 PM, Nicolas B. Pierron wrote:
>>>> Also, what is the alternative? Acquiring a nsCOMPtr/nsRefPtr inside the
>>>> Lambda constructor (or whatever it's called)?
>>>
>>> Yes, another option would be to ensure that the lambda cannot be used
>>> after a specific point.
>>>
>>> nsINode* myNode;
>>> auto callFoo = MakeScopedLambda([&]() {
>>> myNode->Foo();
>>> })
>>> TakeLambda(callFoo);
>>>
>>> Any reference to the lambda after the end of the innermost scope where
>>> MakeScopedLambda is used can cause a MOZ_CRASH.
>>
>> How would you detect that at compile/run time?
>>
>
> Simply by replacing the reference to the lambda inside callFoo at the
> end of the scope, and replace it by a constructing a dummy function
> which expects the same type of arguments as the lambda, but calls
> MOZ_CRASH instead.

Sorry, my question was: how do you implement this with C++? (As in, how
would an actual implementation work?)

[Jan-Ivar Bruaroey]

On 4/10/15 2:09 PM, Seth Fowler wrote:
>> On Apr 10, 2015, at 8:46 AM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
>>
>> I would like to propose that we should ban the usage of refcounted objects
>> inside lambdas in Gecko. Here is the reason:
>>
>> Consider the following code:
>>
>> nsINode* myNode;
>> TakeLambda([&]() {
>> myNode->Foo();
>> });
>>
>> There is nothing that guarantees that the lambda passed to TakeLambda is
>> executed before myNode is destroyed somehow.

The above is a raw pointer bug, not a lambda bug. The above wo/lambdas:

class MyRunnable
{
public:
MyRunnable(nsINode* aMyNode) : mMyNode(aMyNode) {}
void Run() { myNode->Foo(); }
private:
nsINode* mMyNode;
};

nsINode* myNode;
TakeFunc(new MyRunnable(myNode));

That's just as bad, and harder to spot! [1]

IMHO the use of lambdas helps spot the problem, by

1. Being more precise (less boilerplate junk for bugs to hide in), and

2. lambda capture use safer copy construction by default (hence the
standout [&] above for reviewers).

> Lambdas will be much less useful if they can’t capture refcounted
objects, so I’m strongly against banning that.

+1.

Case in point, we use raw pointers with most runnables, a practice
established in NS_DispatchToMainThread [2]. Look in mxr/dxr for the 100+
uses of NS_DispatchToMainThread(new SomeRunnable()).

The new ban would prevent us from passing runnables to lambdas, like [3]

MyRunnableBackToMain* runnable = new MyRunnableBackToMain();

nsRefPtr<media::ChildPledge<nsCString>> p = SomeAsyncFunc();
p->Then([runnable](nsCString result) mutable {
runnable->mResult = result;
NS_DispatchToMainThread(runnable);
});

So I think this ban is misguided. These are old sins not new to lambdas.

> - Seth

.: Jan-Ivar :.

[1] Bonus if you caught that new MyRunnable() is a raw pointer as well.

[2]
http://mxr.mozilla.org/mozilla-central/source/xpco/glue/nsThreadUtils.cpp?mark=164-168#164

[3]
http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp?mark=1516-1521#1501

[Trevor Saunders]

On Mon, Apr 13, 2015 at 01:28:05PM -0400, Ehsan Akhgari wrote:
> On 2015-04-13 5:26 AM, Nicolas B. Pierron wrote:

> >On 04/10/2015 07:47 PM, Ehsan Akhgari wrote:
> >>On 2015-04-10 1:41 PM, Nicolas B. Pierron wrote:
> >>>>Also, what is the alternative? Acquiring a nsCOMPtr/nsRefPtr inside the
> >>>>Lambda constructor (or whatever it's called)?
> >>>
> >>>Yes, another option would be to ensure that the lambda cannot be used
> >>>after a specific point.
> >>>
> >>>nsINode* myNode;
> >>>auto callFoo = MakeScopedLambda([&]() {
> >>> myNode->Foo();
> >>>})
> >>>TakeLambda(callFoo);
> >>>
> >>>Any reference to the lambda after the end of the innermost scope where
> >>>MakeScopedLambda is used can cause a MOZ_CRASH.
> >>
> >>How would you detect that at compile/run time?
> >>
> >
> >Simply by replacing the reference to the lambda inside callFoo at the
> >end of the scope, and replace it by a constructing a dummy function
> >which expects the same type of arguments as the lambda, but calls
> >MOZ_CRASH instead.
>

> Sorry, my question was: how do you implement this with C++? (As in, how
> would an actual implementation work?)

That actually seems kind of straight forward. You want to have an
object that wraps the provided lambda in callFoo and then nukes the
wrapping when the scope exits. So I guess it would look something like
this (totally untested).

template<typename T>
class ScopedLambda
{
template<typename U>
class LambdaHolder
{
LambdaHolder(const LambdaHolder& other)
{
if (!other.valid) {
valid = false;
return;
}

other.master->Add(this);
mLambda = other.mLambda;
valid = true;
}

...

void Revoke() { valid = false; }

void Call()
{
if (valid) /* do magic with vargs to pass
arguments to mLambda */ ;
}

private:
T mLambda;
ScopedLambda<T>* master;
bool valid;
};

ScopedLambda(T lambda) : mLambda(lambda) {}
~ScopedLambda()
{
for (LambdaHolder* h: holders) { h->revoke(); }
}

// Try to force passing ScopedLambda foo to a function to result in the
// function getting a LambdaHolder.
operator LambdaHolder()
{
LambdaHolder l;
l.master = this;
l.mLambda = mLambda;
l.valid = true;
return l;
}

ScopedLambda(const ScopedLamba&) = delete;
ScopedLambda(ScopedLambda&&) = delete;

T mLambda;
List<LambdaHolder<T>*> holders;
};

or actually you might be able to do the same thing my having
ScopedLambda allocate an object on the heap that's ref counted.

Trev

[Jan-Ivar Bruaroey]

On 4/13/15 1:36 PM, Jan-Ivar Bruaroey wrote:
> [2]
> http://mxr.mozilla.org/mozilla-central/source/xpco/glue/nsThreadUtils.cpp?mark=164-168#164

working link I hope:

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.cpp?mark=164-168#164

[Jan-Ivar Bruaroey]

On 4/10/15 4:26 PM, smaug wrote:
> I'd say that is rather painful for reviewers, since both Move() (I
> prefer .swap()) and lambda hide what is actually happening to the refcnt.

Wanna ban copy construction? ;)

Higher-level constructs inherently "hide" something, but I disagree they
make things harder to understand and read, quite the opposite.

And nsRefPtr's copy constructor is probably the safest part of that class.

> So easy to forget to use nsCOMPtr explicitly there.
>
> We should emphasize easy-to-read-and-understand code over fast-to-write.

I agree with your emphasis, however I draw the opposite conclusion.

The death to readability is the boilerplate that lambdas replace IMHO.

I find our existing runnable code hard to reason about because I have to
jump between indirections to lots of disjunct classes, just to follow
the flow of code, not to mention all the boiler plate needed simply to
pass values forward.

I find a parallel here with callbacks vs promises in JavaScript. The
Promise chaining pattern relies on inline function definitions to make
the flow of code match the flow of reading, making it easier to reason
about and review.

.: Jan-Ivar :.

> -Olli
>
>
>
>>
>> - Seth
>>
>

[Ehsan Akhgari]

On 2015-04-13 1:55 PM, Trevor Saunders wrote:
> On Mon, Apr 13, 2015 at 01:28:05PM -0400, Ehsan Akhgari wrote:
>> On 2015-04-13 5:26 AM, Nicolas B. Pierron wrote:

>>> On 04/10/2015 07:47 PM, Ehsan Akhgari wrote:
>>>> On 2015-04-10 1:41 PM, Nicolas B. Pierron wrote:
>>>>>> Also, what is the alternative? Acquiring a nsCOMPtr/nsRefPtr inside the
>>>>>> Lambda constructor (or whatever it's called)?
>>>>>
>>>>> Yes, another option would be to ensure that the lambda cannot be used
>>>>> after a specific point.
>>>>>
>>>>> nsINode* myNode;
>>>>> auto callFoo = MakeScopedLambda([&]() {
>>>>> myNode->Foo();
>>>>> })
>>>>> TakeLambda(callFoo);
>>>>>
>>>>> Any reference to the lambda after the end of the innermost scope where
>>>>> MakeScopedLambda is used can cause a MOZ_CRASH.
>>>>
>>>> How would you detect that at compile/run time?
>>>>
>>>
>>> Simply by replacing the reference to the lambda inside callFoo at the
>>> end of the scope, and replace it by a constructing a dummy function
>>> which expects the same type of arguments as the lambda, but calls
>>> MOZ_CRASH instead.
>>

>> Sorry, my question was: how do you implement this with C++? (As in, how
>> would an actual implementation work?)
>
> That actually seems kind of straight forward. You want to have an
> object that wraps the provided lambda in callFoo and then nukes the
> wrapping when the scope exits. So I guess it would look something like
> this (totally untested).

Does this work though? The |operator LambdaHolder| part is wrong, that
operator will never be used since there is nothing which would call it
when you want to pass the lambda to a different function. Also, I don't
know what the master member variable is supposed to do.

If someone can come up with an implementation of this idea which
actually compiles and works fine, then by all means please do file a bug
and add it.

But such a class, while useful, will not actually prevent the issues
that this proposal is intended to prevent (since for example in the
above code, other things besides just returning from the function can
still invalidate myNode.)

[Ehsan Akhgari]

On 2015-04-10 9:43 PM, Gregory Szorc wrote:

> On Fri, Apr 10, 2015 at 11:46 AM, Ehsan Akhgari <ehsan....@gmail.com
> <mailto:ehsan....@gmail.com>> wrote:
>
> I would like to propose that we should ban the usage of refcounted
> objects
> inside lambdas in Gecko. Here is the reason:
>
> Consider the following code:
>
> nsINode* myNode;

> TakeLambda([&]() {
> myNode->Foo();

> });
>
> There is nothing that guarantees that the lambda passed to TakeLambda is

> executed before myNode is destroyed somehow. While it's possible to
> verify
> on a case by case basis that this code is indeed safe, I think it's too
> error prone to keep these invariants over time. Given the fact that if
> things go wrong we'd deal with a UAF bug which will be sec-critical, I
> think it's better to be safe than sorry here.
>
> If nobody objects, I'll update the style guide in the next few days. If
> you do object, please make a case for why the above analysis is
> incorrect
> and why the usage of refcounted objects in lambdas in general is safe.
>
>

> Why do we merely update the style guide? We have a custom Clang compiler
> plugin. I think we should have the Clang compiler plugin enforce our
> style guidelines by refusing to compile code that is against policy.
> This would enable reviewers to stop focusing on policing the style guide
> and enable them to focus more on the correctness of code. This will
> result in faster reviews and/or higher quality code.

Yes, I'm aware of the clang plugin, as I've written most of the analyses
in it. ;-) See bug 1153304. But that is orthogonal, since that plugin
doesn't cover every single line of code in the code base. Obvious
examples are Windows/B2G specific code.

[Ehsan Akhgari]

[Ehsan Akhgari]

On 2015-04-13 1:36 PM, Jan-Ivar Bruaroey wrote:
> On 4/10/15 2:09 PM, Seth Fowler wrote:
>>> On Apr 10, 2015, at 8:46 AM, Ehsan Akhgari <ehsan....@gmail.com>
>>> wrote:
>>>
>>> I would like to propose that we should ban the usage of refcounted
>>> objects
>>> inside lambdas in Gecko. Here is the reason:
>>>
>>> Consider the following code:
>>>
>>> nsINode* myNode;
>>> TakeLambda([&]() {
>>> myNode->Foo();
>>> });
>>>
>>> There is nothing that guarantees that the lambda passed to TakeLambda is
>>> executed before myNode is destroyed somehow.
>
> The above is a raw pointer bug, not a lambda bug.

Yes, I'm aware. Please see bug 1114683.

> IMHO the use of lambdas helps spot the problem, by
>
> 1. Being more precise (less boilerplate junk for bugs to hide in), and
>
> 2. lambda capture use safer copy construction by default (hence the
> standout [&] above for reviewers).

There is nothing safe about copying raw pointers to refcounted objects.
So the assertion that lambda capture is safe by default is clearly
false. Point #1 is objective, and I don't think has any bearing on what
is safe and what unsafe operations need to be banned.

> > Lambdas will be much less useful if they can’t capture refcounted
> objects, so I’m strongly against banning that.
>
> +1.
>
> Case in point, we use raw pointers with most runnables, a practice
> established in NS_DispatchToMainThread [2]. Look in mxr/dxr for the 100+
> uses of NS_DispatchToMainThread(new SomeRunnable()).

That is a terrible pattern that needs to be eliminated and not
replicated in new code. You can't argue against doing more terrible
things because we do bad things elsewhere in the code.

> The new ban would prevent us from passing runnables to lambdas, like [3]
>
> MyRunnableBackToMain* runnable = new MyRunnableBackToMain();
>
> nsRefPtr<media::ChildPledge<nsCString>> p = SomeAsyncFunc();
> p->Then([runnable](nsCString result) mutable {
> runnable->mResult = result;
> NS_DispatchToMainThread(runnable);

// future Gecko hacker comes around and does:
runnable->FooBar(); // oops, is runnable still valid? maybe not!

> });
>
> So I think this ban is misguided. These are old sins not new to lambdas.

Thanks for the great example of this unsafe code. I modified it to
inject a possible use after free in it.

I think you're missing the intent of my proposal. My goal is to prevent
a new class of sec-critical bugs that will creep into our code base
through this pattern. You seem to think that this is somehow a ban of
lambda usage; please re-read the original post. The only thing that you
need to do in the code above to be able to still use lambdas is to make
runnable an nsRefPtr, and that makes the code safe against the issue
under discussion.

Cheers,
Ehsan

[Jan-Ivar Bruaroey]

On 4/14/15 10:06 AM, Ehsan Akhgari wrote:
> On 2015-04-13 1:36 PM, Jan-Ivar Bruaroey wrote:
>> The above is a raw pointer bug, not a lambda bug.
>
> Yes, I'm aware. Please see bug 1114683.

Thanks, I was not aware of this larger effort. This somewhat addresses
my concern that we seem overly focused on lambdas and calling them
unsafe when they're just another heap instantiation pattern.

>> 2. lambda capture use safer copy construction by default (hence the
>> standout [&] above for reviewers).
>
> There is nothing safe about copying raw pointers to refcounted objects.

There's nothing safe about copying raw pointers to heap objects, period.
Not buying that refcounted objects or lambdas are worse.

> So the assertion that lambda capture is safe by default is clearly
> false.

I said "safer", as in lambdas' defaults are safer for many copyable
types like nsRefPtr than manual instantiation is. They are not less safe
than other things because of raw pointers.

>> > Lambdas will be much less useful if they can’t capture refcounted
>> objects, so I’m strongly against banning that.
>>
>> +1.
>>
>> Case in point, we use raw pointers with most runnables, a practice
>> established in NS_DispatchToMainThread [2]. Look in mxr/dxr for the 100+
>> uses of NS_DispatchToMainThread(new SomeRunnable()).
>
> That is a terrible pattern that needs to be eliminated and not
> replicated in new code.

This is a terrible pattern that we should not grandfather any code in
under, so why is https://bugzil.la/833797 WONTFIX?

NS_DispatchToMainThread encourages use of raw pointers, so lets fix that
for everyone. Until then, this seems to be the pattern.

> You can't argue [I think you mean 'for'] doing more terrible things

> because we do bad things elsewhere in the code.

I write non-exception c++ code every day for that very reason.

>> The new ban would prevent us from passing runnables to lambdas, like [3]
>>
>> MyRunnableBackToMain* runnable = new MyRunnableBackToMain();
>>
>> nsRefPtr<media::ChildPledge<nsCString>> p = SomeAsyncFunc();
>> p->Then([runnable](nsCString result) mutable {
>> runnable->mResult = result;
>> NS_DispatchToMainThread(runnable);
> // future Gecko hacker comes around and does:
> runnable->FooBar(); // oops, is runnable still valid? maybe not!
>> });
>>
>> So I think this ban is misguided. These are old sins not new to lambdas.
>
> Thanks for the great example of this unsafe code. I modified it to
> inject a possible use after free in it.

Thanks for making my point! The ban would not catch the general problem:

MyRunnableBackToMain* runnable = new MyRunnableBackToMain();

NS_DispatchToMainThread(runnable);
// future Gecko hacker comes around and does:
runnable->FooBar(); // oops, is runnable still valid? maybe not!

I hope this shows that lambdas are a red herring here.

> I think you're missing the intent of my proposal. My goal is to prevent
> a new class of sec-critical bugs that will creep into our code base
> through this pattern.

It's not a new class, and the new kids on the bus are not why the bus is
broken.

> You seem to think that this is somehow a ban of
> lambda usage; please re-read the original post. The only thing that you
> need to do in the code above to be able to still use lambdas is to make
> runnable an nsRefPtr, and that makes the code safe against the issue
> under discussion.

Can't hold nsRefPtr to main-destined runnables off main-thread.

I've filed https://bugzil.la/1154337 to discuss a solution to that.

> Cheers,
> Ehsan

.: Jan-Ivar :.

[Ehsan Akhgari]

On 2015-04-14 12:41 PM, Jan-Ivar Bruaroey wrote:
>>> 2. lambda capture use safer copy construction by default (hence the
>>> standout [&] above for reviewers).
>>
>> There is nothing safe about copying raw pointers to refcounted objects.
>
> There's nothing safe about copying raw pointers to heap objects, period.
> Not buying that refcounted objects or lambdas are worse.

OK, feel free to disagree. There is data that supports my position
though, but I cannot talk about it on a public list. If you have
security access, feel free to go through the list of sec-critical bugs
that we fix to get a sense of how common the pattern of UAF with
refcounted objects is.

>> So the assertion that lambda capture is safe by default is clearly
>> false.
>
> I said "safer", as in lambdas' defaults are safer for many copyable
> types like nsRefPtr than manual instantiation is. They are not less safe
> than other things because of raw pointers.

I have no position for or against what the lambda's defaults are.
Again, it seems like you're under the impression that I'm against the
usage of lambdas and are trying to defend lambdas.

>>> > Lambdas will be much less useful if they can’t capture refcounted
>>> objects, so I’m strongly against banning that.
>>>
>>> +1.
>>>
>>> Case in point, we use raw pointers with most runnables, a practice
>>> established in NS_DispatchToMainThread [2]. Look in mxr/dxr for the 100+
>>> uses of NS_DispatchToMainThread(new SomeRunnable()).
>>
>> That is a terrible pattern that needs to be eliminated and not
>> replicated in new code.
>
> This is a terrible pattern that we should not grandfather any code in
> under, so why is https://bugzil.la/833797 WONTFIX?

I don't know, I have not been involved in that.

> NS_DispatchToMainThread encourages use of raw pointers,

Not sure why you think that. Can you clarify why it does? (Perhaps in
a new thread since this is way off topic.)

> so lets fix that
> for everyone. Until then, this seems to be the pattern.

I agree. If there is a problem with NS_DispatchToMainThread, it needs
to be fixed. I have no idea why you think that should block fixing the
issue that this thread is discussing.

>>> The new ban would prevent us from passing runnables to lambdas, like [3]
>>>
>>> MyRunnableBackToMain* runnable = new MyRunnableBackToMain();
>>>
>>> nsRefPtr<media::ChildPledge<nsCString>> p = SomeAsyncFunc();
>>> p->Then([runnable](nsCString result) mutable {
>>> runnable->mResult = result;
>>> NS_DispatchToMainThread(runnable);
>> // future Gecko hacker comes around and does:
>> runnable->FooBar(); // oops, is runnable still valid? maybe not!
>>> });
>>>
>>> So I think this ban is misguided. These are old sins not new to lambdas.
>>
>> Thanks for the great example of this unsafe code. I modified it to
>> inject a possible use after free in it.
>
> Thanks for making my point! The ban would not catch the general problem:
>
> MyRunnableBackToMain* runnable = new MyRunnableBackToMain();
>
> NS_DispatchToMainThread(runnable);
> // future Gecko hacker comes around and does:
> runnable->FooBar(); // oops, is runnable still valid? maybe not!
>
> I hope this shows that lambdas are a red herring here.

No. It just shows that my proposal doesn't completely fix all memory
safety issues that you can have in C++. ;-) That's true, but it's not
really relevant to the topic under discussion here.

>> I think you're missing the intent of my proposal. My goal is to prevent
>> a new class of sec-critical bugs that will creep into our code base
>> through this pattern.
>
> It's not a new class, and the new kids on the bus are not why the bus is
> broken.

We're just talking past each other, I think. Do you agree that the
topic of this thread is a memory safety issue? And if you agree on
that, what exact issues do you see my proposal?

>> You seem to think that this is somehow a ban of
>> lambda usage; please re-read the original post. The only thing that you
>> need to do in the code above to be able to still use lambdas is to make
>> runnable an nsRefPtr, and that makes the code safe against the issue
>> under discussion.
>
> Can't hold nsRefPtr to main-destined runnables off main-thread.

You can if the object has thread safe reference counting. For example,
nsRunnable does, so you can hold nsRefPtrs to anything that inherits
from it from any threads that you want.

For the places where you have main-thread only objects besides
runnables, all you need to do is to ensure they are released on the
right thread, as bug 1154389 shows.

[Randell Jesup]

(was: Re: Proposal to ban the usage of refcounted objects inside C++
lambdas in Gecko)

tl;dr: We should make DispatchToMainThread take already_AddRefed and
move away from raw ptrs for Dispatches in general.

So:

What I want to avoid is this pattern for runnables that hold
thread-restricted pointers (mostly MainThread-restricted, such as JS
callbacks):

FooRunnable::FooRunnable(nsCOMPtr<nsIFoo>& aFoo)
{
mFoo.swap(aFoo); // Never AddRef/Release off MainThread
}
...
{
nsRefPtr<FooRunnable> foo = new FooRunnable(myFoo);
// RefCnt == 1, myFoo.get() == nullptr now
NS_DispatchToMainThread(foo); // infallible (soon), takes a ref to foo (refcnt 2)

// XXX what is foo's refcnt here? You don't know!
}

The reason is that foo may go out of scope here *after* it has Run() and
had the event ref dropped on mainthread. I.e.: at the XXX comment, it
may "usually" be 2, but sometimes may be 1, and when foo goes out of
scope we delete it here, and violate thread-safety for mFoo.

Blindly taking an NS_DispatchToMainThread(new FooRunnable) and
converting it to the pattern above (with nsRefPtr) will introduce a
timing-based thread safety violation IF it holds thread-locked items
(which isn't super-common). But also, it's extra thread-safe
addref/releasing when we don't need to.

A safe pattern is this:
{
nsRefPtr<FooRunnable> foo = new FooRunnable(myFoo);
// RefCnt == 1, myFoo.get() == nullptr now
// This requires adding a version of
// NS_DispatchToMainThread(already_AddRefed<nsIRunnable>) or some such
NS_DispatchToMainThread(foo.forget()); // infallible (soon)
// foo is nullptr
}

And to be honest, that's generally a better pattern for runnables
anyways - we *want* to give them away on Dispatch()/DispatchToMainThread().

Note that you can also do ehsan's trick for safety instead:

~FooRunnable()
{
if (!NS_IsMainThread()) {
// get mainthread
NS_ProxyRelease(mainthread, mFoo);
}
}

I don't love this though. It adds almost-never-executed code, we
addref/release extra times, it could bitrot or forget to grow new
mainthread-only refs. And it wastes code and adds semantic boilerplate
to ignore when reading code.

You could add a macro to build these and hide the boilerplate some, but
that's not wonderful either.

So, if I have my druthers: (And really only #1 is needed, probably)

1) Add already_AddRefed variants of Dispatch/DispatchToMainThread, and
convert things as needed to .forget() (Preferably most
Dispatch/DispatchToMainThreads would become this.) If the Dispatch()
can fail and it's not a logic error somewhere (due to Dispatch to a
doomed thread perhaps in some race conditions), then live with a leak
- we can't send it there to die. DispatchToMainThread() isn't
affected by this and will be infallible soon.

2) If you're building a runnable with a complex lifetime and a
MainThread or thread-locked object, also add the ProxyRelease
destructor. Otherwise consider simple boilerplate (for
MainThread-only runnables) to do:

~FooRunnable() { if (!NS_IsMainThread()) { MOZ_CRASH(); } }

(Perhaps DESTROYED_ON_MAINTHREAD_ONLY(FooRunnable)). Might not be a
bad thing to have in our pockets for other reasons.

3) Consider replacing nsCOMPtr<nsIFoo> mFoo in the runnable with
nsReleaseOnMainThread<nsCOMPtr<nsIFoo>> mFoo and have
~nsReleaseOnMainThread() do NS_ProxyRelease, and also have
nsReleaseOnMainThread block attempts to do assignments that AddRef
(only set via swap or assign from already_AddRefed). Unlike the
MainThreadPtrHolder this could be created on other threads since it
never AddRefs; it only inherits references and releases them on MainThread

I wonder if we could move to requiring already_AddRefed for
DispatchToMainThread (and Dispatch?), and thus block all attempts to do
DispatchToMainThread(new FooRunnable), etc. :-)

--
Randell Jesup, Mozilla Corp
remove "news" for personal email

[Kyle Huey]

On Tue, Apr 14, 2015 at 2:39 PM, Randell Jesup <rjesu...@jesup.org> wrote:
> (was: Re: Proposal to ban the usage of refcounted objects inside C++
> lambdas in Gecko)
>
> tl;dr: We should make DispatchToMainThread take already_AddRefed and
> move away from raw ptrs for Dispatches in general.

Agreed.

- Kyle

[Bobby Holley]

+1.

On Tue, Apr 14, 2015 at 2:42 PM, Kyle Huey <m...@kylehuey.com> wrote:

> On Tue, Apr 14, 2015 at 2:39 PM, Randell Jesup <rjesu...@jesup.org>
> wrote:

> > (was: Re: Proposal to ban the usage of refcounted objects inside C++
> > lambdas in Gecko)
> >
> > tl;dr: We should make DispatchToMainThread take already_AddRefed and
> > move away from raw ptrs for Dispatches in general.
>

> Agreed.
>
> - Kyle

[Jan-Ivar Bruaroey]

On 4/14/15 5:39 PM, Randell Jesup wrote:
> I wonder if we could move to requiring already_AddRefed for
> DispatchToMainThread (and Dispatch?), and thus block all attempts to do
> DispatchToMainThread(new FooRunnable), etc. :-)

Yes! +1.

I like the .forget() semantics, but just to have options, here's a
variation that might even be simpler: https://bugzil.la/1154337#c13

.: Jan-Ivar :.

[Ehsan Akhgari]

Sounds good to me as well.

On 2015-04-14 5:44 PM, Bobby Holley wrote:
> +1.
>
> On Tue, Apr 14, 2015 at 2:42 PM, Kyle Huey <m...@kylehuey.com> wrote:
>
>> On Tue, Apr 14, 2015 at 2:39 PM, Randell Jesup <rjesu...@jesup.org>
>> wrote:

>>> (was: Re: Proposal to ban the usage of refcounted objects inside C++
>>> lambdas in Gecko)
>>>
>>> tl;dr: We should make DispatchToMainThread take already_AddRefed and
>>> move away from raw ptrs for Dispatches in general.
>>