1201275 - Make the "raw pointer to refcounted object captured by lambda" analysis more nuanced

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, defect, P3)

Description

[Botond Ballo [:botond]]

Specifically, capturing a raw pointer to a refcounted object is only problematic if the lambda escapes the function that created it.

We should try to determine whether or not the lambda escapes, and allow the code if we can prove it doesn't.

Instrumental to being able to prove in non-trivial cases that the lambda doesn't escape, is adding a MOZ_NOESCAPE attribute to a function parameter which means "this function doesn't stash away the value of this parameter anywhere" (see bug 1200413 comment 5).

Comment 1

[(no longer active)]

How are we going to check that the MOZ_NOESCAPE is not a bunch of lies?

Comment 2

[Botond Ballo [:botond]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #1)
> How are we going to check that the MOZ_NOESCAPE is not a bunch of lies?

When processing the translation unit containing the definition of a function with a MOZ_NOESCAPE parameter, the analysis could check that it's not lying.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

(In reply to Botond Ballo [:botond] from comment #2)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #1)
> > How are we going to check that the MOZ_NOESCAPE is not a bunch of lies?
> 
> When processing the translation unit containing the definition of a function
> with a MOZ_NOESCAPE parameter, the analysis could check that it's not lying.

Going with a really basic analysis, we could confirm that the MOZ_NOESCAPE parameter is not used in any way except to pass to noescape functions, or basic things like dereferencing. The tricky part is that we'd have to make the closure's call operator consider the this parameter to be MOZ_NOESCAPE, which isn't always true (the closure could capture itself and cause itself to escape). So we'd probably have to blacklist closures which capture themselves somehow.

Comment 4

[Kelsey Gilbert [:jgilbert]]

I will note that for some refcounted Foo, if I have pass a `Foo* foo` pointer into the function (as the lifetime of the Foo is already guaranteed without RefPtrs), it's silly to disallow capturing [&foo] in a local lambda within the function.

In this case, preventing [&foo] capture doesn't protect me from shooting myself in the foot unless I've already blown off my leg.

Comment 5

[Botond Ballo [:botond]]

(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> I will note that for some refcounted Foo, if I have pass a `Foo* foo`
> pointer into the function (as the lifetime of the Foo is already guaranteed
> without RefPtrs), it's silly to disallow capturing [&foo] in a local lambda
> within the function.
> 
> In this case, preventing [&foo] capture doesn't protect me from shooting
> myself in the foot unless I've already blown off my leg.

I completely agree. Filed bug 1242789 to relax the static analysis for this case.

Comment 6

[(no longer active)]

Botond, Seth, Bobby, with bug 1281935, is there anything else that you guys would like to see improved here?

Comment 7

[Bobby Holley (:bholley)]

Given mystor's patch, I think bug 1281935 solved my use case.

Comment 8

[Botond Ballo [:botond]]

(In reply to :Ehsan Akhgari from comment #6)
> Botond, Seth, Bobby, with bug 1281935, is there anything else that you guys
> would like to see improved here?

I think the change being made in bug 1281935 is a great improvement, and might be all we need. (I'm not sure if there are any good use cases for wrapping a lambda into std::function or similar if you're _not_ going to return it or store it somewhere that outlives the function; if such use cases come up, we can always revisit the analysis to try to accommodate them.)

Comment 9

[Andi [:andi]]

Do you think we still need this?

Comment 10

[Botond Ballo [:botond]]

I haven't run into situations where we would want this in cases that bug 1281935 didn't address.

Given that handling other cases would involve more complexity to the check, I suggest we wontfix this for now. If motivating cases come up frequently enough, we can reconsider and reopen.