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

[Ehsan Akhgari]

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

On Fri, Apr 10, 2015 at 11:46 AM, Ehsan Akhgari ehsan.akhg...@gmail.com
mailto:ehsan.akhg...@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.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

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

[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();

   nsRefPtrmedia::ChildPledgensCString 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 :.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

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

[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();

   nsRefPtrmedia::ChildPledgensCString 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.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

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

[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.akhg...@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();

   nsRefPtrmedia::ChildPledgensCString 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

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

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

[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





___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

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

[Jan-Ivar Bruaroey]

On 4/10/15 2:09 PM, Seth Fowler wrote:

On Apr 10, 2015, at 8:46 AM, Ehsan Akhgari ehsan.akhg...@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();

  nsRefPtrmedia::ChildPledgensCString 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


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

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

[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

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

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

[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).

templatetypename T
class ScopedLambda
{
templatetypename 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;
ScopedLambdaT* 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;
ListLambdaHolderT* 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

 
 ___
 dev-platform mailing list
 dev-platform@lists.mozilla.org
 https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

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

[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?)


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

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

[Nicolas B. Pierron]

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.


I guess I would have wrote the auto as  ScopedLambdavoid (*)()  for the 
example.


--
Nicolas B. Pierron
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

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

[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.akhg...@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-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Ehsan Akhgari mailto:ehsan.akhg...@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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

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

[Andrew McCreight]

On Fri, Apr 10, 2015 at 10:37 AM, Ehsan Akhgari ehsan.akhg...@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.akhg...@gmail.com
 mailto:ehsan.akhg...@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
 |nsTArrayFoo* 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.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

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

[smaug]

On 04/10/2015 09:09 PM, Seth Fowler wrote:



On Apr 10, 2015, at 8:46 AM, Ehsan Akhgari ehsan.akhg...@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:


nsCOMPtrnsINode 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:


nsCOMPtrnsINode 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.


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



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

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

[Gregory Szorc]

On Fri, Apr 10, 2015 at 11:46 AM, Ehsan Akhgari ehsan.akhg...@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.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform