Closed Bug 1201275 Opened 6 years ago Closed 8 months ago

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

Categories

(Firefox Build System :: Source Code Analysis, defect, P3)

defect

Tracking

(firefox43 affected)

RESOLVED WONTFIX
Tracking Status
firefox43 --- affected

People

(Reporter: botond, Unassigned)

References

Details

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).
How are we going to check that the MOZ_NOESCAPE is not a bunch of lies?
(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.
(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.
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.
(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.
See Also: → 1281935
Botond, Seth, Bobby, with bug 1281935, is there anything else that you guys would like to see improved here?
Flags: needinfo?(seth)
Flags: needinfo?(botond)
Flags: needinfo?(bobbyholley)
Given mystor's patch, I think bug 1281935 solved my use case.
Flags: needinfo?(bobbyholley)
(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.)
Flags: needinfo?(botond)
Product: Core → Firefox Build System
Flags: needinfo?(seth.bugzilla)

Do you think we still need this?

Flags: needinfo?(botond)
Priority: -- → P3

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.

Status: NEW → RESOLVED
Closed: 8 months ago
Flags: needinfo?(botond)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.