1421435 - "Refcounted variable 'this' of type cannot be captured by a lambda" is overly strict

Status: NEW

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

Description

[Ben Kelly [:bkelly, not reviewing]]

I'm trying to write some code like this:

  Foo::Foo()
  {
    OnShutdownPromise()->Then(GetCurrentThreadSerialEventTarget(), __func__,
      [this] () {
        mPromiseHolder.Complete();
        this->Shutdown;
      })->Track(mPromiseHolder);
  }

  Foo::~Foo()
  {
    mPromiseHolder.DisconnectIfExists();
  }

That static analysis blows up because Foo is ref-counted.

In this case I don't want the shutdown promise to hold Foo alive longer than necessary.  I really don't want a strong ref.

Also, this code is safe.  The MozPromiseRequestHolder avoids invoking the lambda past the ~Foo() destructor.

Currently we have code that uses ugly work arounds like this to get past the static analysis:

https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/dom/media/MediaEventSource.h#117-128

It would be nice if we could add some kind of annotation or other opt-opt for cases where we know that something like MozPromiseRequestHolder makes it safe.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Maybe something like this?

  [MOZ_PROMISE_HOLDER_PROTECTED(this)] () { }

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Need to be of course very very careful with this kind of stuff. MOZ_PROMISE_HOLDER_PROTECTED looks ugly enough that it should be fine.

That MediaEventSource looks scary. So error prone to just start using RawPtr without thinking the reasons why we have static analysis here.

Comment 3

[Simon Giesecke [:sg] [he/him]]

I also ran into this. I also think it should either be possible to opt-out, or the reliability should be reduced such that this doesn't fail the analysis build. Employing workarounds doesn't make this safer.

I assume the check does not only refer to capturing this. For this case, it is particularly annoying.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Async lambda usage is super error prone, so forcing code writer and reviewer to think about ownership model is a good thing, so workarounds could make it safer.

Comment 5

[Simon Giesecke [:sg] [he/him]]

The analysis is not restricted to async lambda usages. My case is simply a local use with std::any_of, the lambda is never passed to another scope:

  if (std::any_of(aCallback->mDirectoryIds.begin(),
                  aCallback->mDirectoryIds.end(),
                  [this](const nsCString& directoryId) {
                    MOZ_ASSERT(!directoryId.IsEmpty());

                    return mDirectoryInfos.Get(directoryId, nullptr);
                  })) {
    return false;
   }

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #5)

The analysis is not restricted to async lambda usages. My case is simply a local use with std::any_of, the lambda is never passed to another scope:

  if (std::any_of(aCallback->mDirectoryIds.begin(),
                  aCallback->mDirectoryIds.end(),
                  [this](const nsCString& directoryId) {
                    MOZ_ASSERT(!directoryId.IsEmpty());

                    return mDirectoryInfos.Get(directoryId, nullptr);
                  })) {
    return false;
   }

That is indeed annoying. Does:

// Work around lambda refcounting analysis.
const auto& directoryInfos = mDirectoryInfos;
if (std::any_of(..., [&directoryInfos](const nsCString& directoryId) { return directoryInfos.Get(directoryId, nullptr); })) {
...
}

work? If it does, that's great, but it also points to holes in the lambda safety analysis. :(

I guess we could whitelist functions where we know the lambda cannot escape?

Comment 7

[Simon Giesecke [:sg] [he/him]]

(In reply to Nathan Froyd [:froydnj] from comment #6)

(In reply to Simon Giesecke [:sg] [he/him] from comment #5)

That is indeed annoying. Does:

// Work around lambda refcounting analysis.
const auto& directoryInfos = mDirectoryInfos;
if (std::any_of(..., [&directoryInfos](const nsCString& directoryId) { return directoryInfos.Get(directoryId, nullptr); })) {
...
}

work? If it does, that's great, but it also points to holes in the lambda safety analysis. :(

Yes, with your suggested workaround, it does not trigger mozilla-refcounted-inside-lambda.

I guess we could whitelist functions where we know the lambda cannot escape?

I think this is not very helpful, since this would not be restricted to library functions, but any user-defined algorithm that accepts a function object.

Comment 8

[Nathan Froyd [:froydnj]]

A slightly more inclusive approach is to annotate could-be-lambda function objects as MOZ_GUARANTEED_TO_NOT_ESCAPE or something in addition to the list of library functions where we cannot add the annotation. It's not great, as people can change how the function works and fail to remove the annotation, but it might encourage people to not do workarounds like the above.

Comment 9

[Simon Giesecke [:sg] [he/him]]

(In reply to Nathan Froyd [:froydnj] from comment #8)

A slightly more inclusive approach is to annotate could-be-lambda function objects as MOZ_GUARANTEED_TO_NOT_ESCAPE or something in addition to the list of library functions where we cannot add the annotation. It's not great, as people can change how the function works and fail to remove the annotation, but it might encourage people to not do workarounds like the above.

Enabling an override via such an annotation on the lambda would be acceptable IMO, but I am not completely sure where this annotation would be placed.

I just had another similar instance of the issue, which I worked around by dereferencing in the capture list:

  auto foundIt = std::find_if(
      mDelayedEnqueueInfos.cbegin(), mDelayedEnqueueInfos.cend(),
      [& fileHandle = *aFileHandle](const auto& delayedEnqueueInfo) {
        return delayedEnqueueInfo.mFileHandle == &fileHandle;
      });

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #9)

(In reply to Nathan Froyd [:froydnj] from comment #8)

A slightly more inclusive approach is to annotate could-be-lambda function objects as MOZ_GUARANTEED_TO_NOT_ESCAPE or something in addition to the list of library functions where we cannot add the annotation. It's not great, as people can change how the function works and fail to remove the annotation, but it might encourage people to not do workarounds like the above.

Enabling an override via such an annotation on the lambda would be acceptable IMO, but I am not completely sure where this annotation would be placed.

Sorry, I should have been more precise: the annotation would be placed on the function argument in the function declaration. The lambda analysis would therefore avoid analyzing lambdas passed as that particular argument to said function.

Comment 11

[Simon Giesecke [:sg] [he/him]]

Ah, that makes sense, so e.g.

template<typename Func>
void DoSomething(const Func &aFunc MOZ_GUARANTEED_TO_NOT_ESCAPE);

Comment 12

[Byron Campen [:bwc]]

I should also point out that for types that use NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_DESTROY, the reason is usually because it is only safe to destroy the object on a particular thread. Doing this with a lambda does not work right now, and so existing code has to use other methods like NewNonOwningRunnableMethod:

https://searchfox.org/mozilla-central/source/dom/media/MediaResource.cpp#26

Comment 13

[Simon Giesecke [:sg] [he/him]]

While the more widespread use of lamdba expressions, this is getting increasingly annoying, and requires extensive workarounds that impair readability :(

Comment 14

[Simon Giesecke [:sg] [he/him]]

Please either disable this check, move it to the experimental checks or change it to be less strict.

Comment 15

[Simon Giesecke [:sg] [he/him]]

Can we please disable this? It is very annoying for the kind of code we are writing in the DOM W&S components.

Comment 16

[Andi [:andi]]

Attachment: Bug 1421435 - move "refcounted-inside-lambda" checker to alpha.

Because of lack on consensus on how this checker should be further developed in order
to mittigate false-positives, we move it for now to alpha stage.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

What does alpha stage mean?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

If this means the checker isn't run by default, that means async lambda usage will need to get r- by default.
Raw pointer usage is and has been traditionally so common case for security critical issues.

Comment 19

[Andi [:andi]]

(In reply to Olli Pettay [:smaug] from comment #18)

If this means the checker isn't run by default, that means async lambda usage will need to get r- by default.
Raw pointer usage is and has been traditionally so common case for security critical issues.

True and also this will not be present during the review phase analysis.

Comment 20

[Andi [:andi]]

Is this something acceptable until we reach a consensus on how to move further? Because right now I think it creates more frustration for our engineer.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

I don't think there should be any question here when it is about security vs. frustration.

Personally I get rather frustrated when fixing security bugs caused by C++ features which could have been avoided either by not using them or having good static analysis.

Would it be hard to add some annotation to bypass the checks? Something which would scream to the patch author and reviewer that the relevant code needs to be reviewed particularly carefully.

Comment 22

[Simon Giesecke [:sg] [he/him]]

Two things about this: I think "frustration" does not hit the point here. The problem is that it forces to make workarounds that impair readability/maintainability of the code, which rather increases the chance of introducing security bugs.

While this might be a valid trade-off if the check had only few false positives, this is not the case here: The situation where a function accepts a functor (which might or might not be a lambda expression, it's just the current check only applies to lambda expressions) that is run in another thread is a very special case. While it might make sense to warn specifically about this, the check currently assumes this is the default case, which it is not, neither in general nor in our codebase.

I don't think it would be a good idea either to make this a hint/note in the reviewbot with the current implementation, since given the high rate of false positives it would not be taken seriously.

Note that I don't suggest removing the check. When it is made more accurate, it should be moved out of alpha again.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Thread doesn't matter here. Async handling is enough, and that happens all the time with MozPromise.

Comment 24

[Simon Giesecke [:sg] [he/him]]

(In reply to Olli Pettay [:smaug] from comment #23)

Thread doesn't matter here. Async handling is enough,
That's true.

and that happens all the time with MozPromise.

Right. The MozPromise methods should opt in into the check.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Given how frequently MozPromises are used currently, I'd expect that we'd had tens of security bugs without the check.
And we're adding more and more MozPromises all the time.

Safer behavior shouldn't be opt-in.

Comment 26

[Andi [:andi]]

(In reply to Olli Pettay [:smaug] from comment #25)

Given how frequently MozPromises are used currently, I'd expect that we'd had tens of security bugs without the check.
And we're adding more and more MozPromises all the time.

Safer behavior shouldn't be opt-in.

Would it be possible to provide an example of such an opt-out scenario?

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Well, something like comment 1.

Comment 28

[Andi [:andi]]

(In reply to Olli Pettay [:smaug] from comment #27)

Well, something like comment 1.

Are you sure we want this? Don't we want to have marked the functor from the declaration that takes the lambda, as there is an example at comment 11.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

That could also work (hopefully with more scarier annotation name).

Comment 30

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

FWIW I have seen this analysis help catch multiple major use-after-free bugs in Gecko code written by less experienced C++ developers. It seems to be a very common mistake to capture a local Document* or similar in a lambda when trying to work async, and not realize that one needs to hold a strong reference, so I would generally be opposed to disabling this check.

While this might be a valid trade-off if the check had only few false positives, this is not the case here: The situation where a function accepts a functor (which might or might not be a lambda expression, it's just the current check only applies to lambda expressions) that is run in another thread is a very special case. While it might make sense to warn specifically about this, the check currently assumes this is the default case, which it is not, neither in general nor in our codebase

The reasoning behind this check is quite simple:

• If the lambda is capturing a raw pointer to a refcounted value by reference, it is assumed that the lambda is not going to escape the current stack frame, so it does not assertions about safe ownership of values
• If the lambda is capturing by value, it assumes that this is because it cannot capture by reference, and thus that the lambda is going to escape the current stack frame, so it asserts that ownership is preserved.

In general, if you're running into what seems like a false-positive with this lint, you can probably get around it by using [&] captures, which capture stack local frames. I think the one exception to this is if you are defining a lambda which does not escape the creating stack frame, you want to explicilty write out your captures, and you are capturing this (as this is not a local variable). For most local variables, you can capture them like [&foo, &bar], but you can't capture [&this], as this is special.

Based on a short conversation with :sg on slack, I think that the vast majority of the issues they ran into are related to capturing [this] in a situation where they could capture with [&] but chose not to. In general, I tend to use [&] capturing when dealing with lambdas which won't escape the current stack frame, which will already handle this situation, but in cases where you want an explicit capture list, the lambda is not going to escape the current frame, and the only required capture is the this pointer, I can see that it could cause issues.

I would generally prefer to not risk circumstances where someone writes [this] instead of [self = RefPtr{this}] when doing a lambda capture which may escape the current stack frame, especially as there's an easy fix for lambdas which don't (use [&]), and I definitely don't think we should relax the assertion for non-this values, as those can be captured as [&foo] already to bypass the lint, and explicitly declare your scoping assumptions.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Obviously using [&] is just a way to hack around the current check. And I very recently was reviewing a patch which was trying to use that to avoid the check (in an unsafe way).
Ideally we should improve the checker to catch also cases when [&] to refcounted object is passed to lambda.
But at least that requires some extra work from the patch author, so probably a pointer has been tried first and if it hasn't worked,
hopefully one thinks about why.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Can we resurrect this, Andi?

Comment 33

[Andi [:andi]]

I agree with what :nika is saying I'm not convinced we should modify the current behavior at least not for the time being when we don't havea consistent majority of developers who want this modify. I will keep this open to track it but I will change it's priority.