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.
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
