Closed Bug 1336510 Opened 3 years ago Closed 3 years ago

lambda analysis for raw pointers misses references to |this|

Categories

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

defect

Tracking

(firefox-esr4552+ fixed, firefox51 wontfix, firefox52+ fixed, firefox-esr5252+ fixed, firefox53+ fixed, firefox54+ fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 52+ fixed
firefox51 --- wontfix
firefox52 + fixed
firefox-esr52 52+ fixed
firefox53 + fixed
firefox54 + fixed

People

(Reporter: froydnj, Assigned: Nika)

References

Details

(Keywords: csectype-uaf, sec-critical, Whiteboard: [post-critsmash-triage][adv-main52+][adv-esr45.8+])

Attachments

(20 files, 1 obsolete file)

2.16 KB, patch
smaug
: review+
jesup
: feedback+
Details | Diff | Splinter Review
1.23 KB, patch
botond
: review+
Details | Diff | Splinter Review
28.89 KB, patch
jwwang
: review+
jib
: review+
gcp
: review+
jesup
: feedback+
Details | Diff | Splinter Review
10.37 KB, patch
Details | Diff | Splinter Review
28.21 KB, patch
Details | Diff | Splinter Review
2.86 KB, patch
keeler
: review+
Details | Diff | Splinter Review
7.40 KB, patch
Details | Diff | Splinter Review
2.16 KB, patch
Details | Diff | Splinter Review
1.24 KB, patch
Details | Diff | Splinter Review
29.84 KB, patch
Details | Diff | Splinter Review
2.88 KB, patch
Details | Diff | Splinter Review
23.33 KB, patch
Details | Diff | Splinter Review
13.46 KB, patch
Details | Diff | Splinter Review
1.60 KB, patch
valentin
: review+
Details | Diff | Splinter Review
11.34 KB, patch
jwwang
: review+
gcp
: review+
Details | Diff | Splinter Review
3.16 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
1.70 KB, patch
schien
: review+
Details | Diff | Splinter Review
2.97 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
755 bytes, patch
Details | Diff | Splinter Review
5.75 KB, patch
Details | Diff | Splinter Review
Over in bug 1328768, we had the following code:

http://searchfox.org/mozilla-central/rev/bf98cd4315b5efa1b28831001ad27d54df7bbb68/ipc/glue/CrashReporterHost.cpp#113

    NS_DispatchToMainThread(NS_NewRunnableFunction([=] () -> void {
      nsCOMPtr<nsICrashService> crashService =
        do_GetService("@mozilla.org/crashservice;1");
      if (crashService) {
        crashService->AddCrash(processType, crashType, childDumpID);
      }

      nsCOMPtr<nsIAsyncShutdownClient> barrier = GetShutdownBarrier();

      if (barrier) {
        barrier->RemoveBlocker(this); // XXX
      }
    }));

|this| is an instance of a refcounted type, yet the lambda capture analysis doesn't complain about the capture of it, so we're capturing the raw pointer, rather than a strong reference.  That seems bad.  Was this the result of relaxing the rules in bug 1281935, or just an oversight in the analysis?
Flags: needinfo?(michael)
The problem is in this line of code:

    if (Capture.capturesVariable() && Capture.getCaptureKind() != LCK_ByRef) {
      QualType Pointee = Capture.getCapturedVar()->getType()->getPointeeType();

Capture.capturesVariable() doesn't return `true` for captures of `this`.

I'm pretty sure that the reason why we didn't consider captures on `this` to violate the analysis is because unless `this` is captured, you cannot call private methods on objects of your type. We wanted you to be able to do:

nsRefPtr<ThisType> self = this;
[this, self] () {
    self->CallSomePrivateMethod();
}

Where it only captures `this` to be able to call private methods.

I'll look into rejecting places where you actually use the `this` pointer. I imagine it shouldn't be _too_ hard.
Assignee: nobody → michael
Flags: needinfo?(michael)
Priority: -- → P1
Depends on: 1328768
MozReview-Commit-ID: 2fuMHyspjhs
MozReview-Commit-ID: 4lVGrGzhVXh
Attachment #8833509 - Flags: review?(bugs)
Attachment #8833510 - Flags: review?(bugmail)
Attachment #8833511 - Flags: review?(jwwang)
Botond, can you please take a look at this change, since in the past you have been a critic of this analysis.  :-)
Flags: needinfo?(botond)
Comment on attachment 8833507 [details] [diff] [review]
Part 1: Correct the RefCountedInsideLambda Check to complain about capturing and using `this` without a backing strong reference

Review of attachment 8833507 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!

::: build/clang-plugin/RefCountedInsideLambdaChecker.cpp
@@ +141,5 @@
> +bool RefCountedInsideLambdaChecker::ThisVisitor::VisitCXXThisExpr(CXXThisExpr *This) {
> +  QualType Pointee = This->getType()->getPointeeType();
> +  if (!Pointee.isNull() && isClassRefCounted(Pointee)) {
> +    Checker.diag(This->getLocStart(),
> +         "Refcounted variable %0 of type %1 cannot be captured by a lambda",

It sucks to copy these strings.  How about adding a helper that takes a source location and diagnoses both of these and use it both here and inside check() above?

::: build/clang-plugin/RefCountedInsideLambdaChecker.h
@@ +17,5 @@
> +
> +private:
> +  class ThisVisitor : public RecursiveASTVisitor<ThisVisitor> {
> +  public:
> +    ThisVisitor(RefCountedInsideLambdaChecker& Checker) : Checker(Checker) {}

Nit: explicit.

::: build/clang-plugin/tests/TestNoRefcountedInsideLambdas.cpp
@@ +601,5 @@
> +  std::function<void()>([=]() {
> +    method(); // expected-error{{Refcounted variable 'this' of type 'R' cannot be captured by a lambda}} expected-note{{Please consider using a smart pointer}}
> +  });
> +  std::function<void()>([=]() {
> +      privateMethod(); // expected-error{{Refcounted variable 'this' of type 'R' cannot be captured by a lambda}} expected-note{{Please consider using a smart pointer}}

Nit: please fix indentation.
Attachment #8833507 - Flags: review?(ehsan) → review+
Comment on attachment 8833511 [details] [diff] [review]
Part 4: Capture a strong reference to this in dom/media

Review of attachment 8833511 [details] [diff] [review]:
-----------------------------------------------------------------

f+ for webrtc-related things; r? to jib
Attachment #8833511 - Flags: review?(jib)
Attachment #8833511 - Flags: feedback+
Comment on attachment 8833511 [details] [diff] [review]
Part 4: Capture a strong reference to this in dom/media

Review of attachment 8833511 [details] [diff] [review]:
-----------------------------------------------------------------

r? gcp for Cameras*
Attachment #8833511 - Flags: review?(gpascutto)
(In reply to Michael Layzell [:mystor] from comment #1)
> nsRefPtr<ThisType> self = this;
> [this, self] () {
>     self->CallSomePrivateMethod();
> }

Just a note: I was wrong about this, this works fine:

RefPtr<ThisType> self = this;
[self] () { self->CallSomePrivateMethod(); }
(In reply to :Ehsan Akhgari from comment #6)
> Botond, can you please take a look at this change, since in the past you
> have been a critic of this analysis.  :-)

Thanks for flagging me :)

I think this is a reasonable (and valuable) change.
Flags: needinfo?(botond)
Comment on attachment 8833510 [details] [diff] [review]
Part 3: Capture a strong reference to this in CompositorBridgeParent::FlushApzRepaints

Review of attachment 8833510 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing review.
Attachment #8833510 - Flags: review?(bugmail) → review+
Attachment #8833507 - Attachment is obsolete: true
Attachment #8833507 - Flags: feedback?(nfroyd)
Comment on attachment 8833509 [details] [diff] [review]
Part 2: Fix lambda captures of this in PresentationConnection

And modern C++ bites us again.
I wonder if any good (== not total security hazard) features have been added to C++ recently.
Attachment #8833509 - Flags: review?(bugs) → review+
Comment on attachment 8833511 [details] [diff] [review]
Part 4: Capture a strong reference to this in dom/media

Review of attachment 8833511 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaManager.cpp
@@ +2298,5 @@
>      RefPtr<Refcountable<UniquePtr<SourceSet>>> devices(
>           new Refcountable<UniquePtr<SourceSet>>(aDevices)); // grab result
>  
>      // Ensure that the captured 'this' pointer and our windowID are still good.
>      if (!MediaManager::Exists() ||

Now redundant.

::: dom/media/ogg/OggDemuxer.cpp
@@ +133,3 @@
>        OGG_DEBUG("Reporting telemetry MEDIA_OGG_LOADED_IS_CHAINED=%d", isChained);
>        Telemetry::Accumulate(Telemetry::ID::MEDIA_OGG_LOADED_IS_CHAINED, isChained);
>      });

this and self appear unused here. Would [isChained]() suffice?
Attachment #8833511 - Flags: review?(jib) → review+
(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 8833509 [details] [diff] [review]
> Part 2: Fix lambda captures of this in PresentationConnection
> 
> And modern C++ bites us again.
> I wonder if any good (== not total security hazard) features have been added
> to C++ recently.

These lambdas (to a degree) replace methods plus code tricks like WrapRunnable, but even with that you had to use thread->Dispatch(WrapRunnable(RefPtr<foo>(this), &foo::method, ...) if you were passing 'this' in and there wasn't something else keeping it alive.  NewRunnableMethod() doesn't let you do the RefPtr<foo>(this) trick.

Any chance this analysis could be extended to NewRunnableMethod(this, ...)?
Flags: needinfo?(michael)
Attachment #8833511 - Flags: review?(jwwang) → review+
(In reply to Randell Jesup [:jesup] from comment #16)
> Any chance this analysis could be extended to NewRunnableMethod(this, ...)?

NewRunnableMethod takes a strong reference to `this`, so we don't need an analysis like this.
Looking at the blame, a lot of the code issues found by this analysis affect 52+. I'm assuming this code pattern is serious enough that'll want to uplift?
Attachment #8833511 - Flags: review?(gpascutto) → review+
Are we sure we won't want to consider these as at least ride-along patches for any future 51 point release?
I do think we want to uplift the fixes before landing the analysis itself, because it can easily be run on an older tree.
(In reply to :Ehsan Akhgari from comment #20)
> I do think we want to uplift the fixes before landing the analysis itself,
> because it can easily be run on an older tree.

I'd happily hold the analysis patches until a few weeks after 52 ships, and have someone just run them occasionally on a local tree.  I would strongly suggest we consider these for any 51 point release, and otherwise land them later in the 52 beta cycle.  All the fixes should be very safe.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #15)
> ::: dom/media/ogg/OggDemuxer.cpp
> @@ +133,3 @@
> >        OGG_DEBUG("Reporting telemetry MEDIA_OGG_LOADED_IS_CHAINED=%d", isChained);
> >        Telemetry::Accumulate(Telemetry::ID::MEDIA_OGG_LOADED_IS_CHAINED, isChained);
> >      });
> 
> this and self appear unused here. Would [isChained]() suffice?

Nope! Unfortunately, OGG_DEBUG expands to code which references a member variable.
Flags: needinfo?(michael)
Only part 4 needed to be rebased onto aurora, and one other patch was needed as well.

MozReview-Commit-ID: 4lVGrGzhVXh
Attachment #8834562 - Flags: review?(dkeeler)
Comment on attachment 8834562 [details] [diff] [review]
Part 5: AURORA: Capture a strong reference to this in dom/u2f

Review of attachment 8834562 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: dom/u2f/U2F.cpp
@@ -660,2 @@
>      mAbstractMainThread->Dispatch(NS_NewRunnableFunction(
> -      [status, this] () {

It looks like there are some other closures that capture `this` but don't actually use it - would this be a good opportunity to also remove those captures?
Attachment #8834562 - Flags: review?(dkeeler) → review+
(In reply to David Keeler [:keeler] (use needinfo?) from comment #25)
> It looks like there are some other closures that capture `this` but don't
> actually use it - would this be a good opportunity to also remove those
> captures?

Perhaps - This patch is intended for AURORA, so I don't think it's worth doing that type of code cleanup, and I think that if we want to clean this up we should do it in another bug. I think keeping this bug focused on the sec changes is a good idea.
MozReview-Commit-ID: 4lVGrGzhVXh
MozReview-Commit-ID: 4j2TfyyISUg
Hey, I've attached patches for central, aurora and beta for this bug. How do you want me to handle sec-approval / landing them?
Flags: needinfo?(dveditz)
Group: core-security → dom-core-security
Flags: needinfo?(dveditz)
These are the patches rebased onto ESR45. Fortunately there are less lambdas in use here :).

MozReview-Commit-ID: H5MVVPtj5aM
You recently modified nsMultiMixedConv(), and added the following lines:

http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/netwerk/streamconv/converters/nsMultiMixedConv.cpp#810-812

These capture `this` by value, while allowing the closure to escape. This will be rejected when this analysis lands.

I would simply correct the code to capture a `RefPtr<nsMultiMixedConv> self = this`, but I believe that that will create an unbreakable reference cycle and leak the datastructure. How would you like me to refactor this to make the ownership more clear and avoid the static analysis?
Flags: needinfo?(honzab.moz)
Valentin, you reviewed the above patch. You might also have some insight as to what the correct solution here would be.
Flags: needinfo?(valentin.gosu)
This is a patch which works around the recently added lambda in nsMultiMixedConv. This particular usage was completely safe, but the analysis was rejecting it. This patch uses another mechanism to construct a std::function object which captures a pointer to this in order to avoid the static analysis failure.

MozReview-Commit-ID: 7Mvh9aeCKOh
Attachment #8837289 - Flags: review?(valentin.gosu)
Attachment #8837289 - Flags: review?(valentin.gosu) → review+
Flags: needinfo?(valentin.gosu)
:mystor, thanks for the patch!
Flags: needinfo?(honzab.moz)
(In reply to Michael Layzell [:mystor] from comment #35)
> You recently modified nsMultiMixedConv(), and added the following lines:
> 
> http://searchfox.org/mozilla-central/rev/
> d3307f19d5dac31d7d36fc206b00b686de82eee4/netwerk/streamconv/converters/
> nsMultiMixedConv.cpp#810-812
> 
> These capture `this` by value, while allowing the closure to escape. This
> will be rejected when this analysis lands.
> 
> I would simply correct the code to capture a `RefPtr<nsMultiMixedConv> self
> = this`, but I believe that that will create an unbreakable reference cycle
> and leak the datastructure. How would you like me to refactor this to make
> the ownership more clear and avoid the static analysis?

BTW, the tokenizer has a "Finish" method that could release (if not already) the lambda (function) to prevent cycle when captured as refptr.  We can tho exploit here the fact that mTokenizer won't live longer than this, as you said before.  

There is also move argument capture like:

RefPtr<T> self = this;
[self = Move(self)]() -> ...

but that is c++14 specific which is only avail on windows, AFAIK.

anyway, thanks.
(In reply to Honza Bambas (:mayhemer) from comment #39)
> BTW, the tokenizer has a "Finish" method that could release (if not already)
> the lambda (function) to prevent cycle when captured as refptr.  We can tho
> exploit here the fact that mTokenizer won't live longer than this, as you
> said before.  

I saw that but didn't understand the machinations of nsMultiMixedConv well enough to be confident that Finish would always be called before the last non-loop reference to nsMultiMixedConv was dropped unconditionally, so I wanted to avoid that leak and didn't make that change.

> There is also move argument capture like:
> 
> RefPtr<T> self = this;
> [self = Move(self)]() -> ...
> 
> but that is c++14 specific which is only avail on windows, AFAIK.

I'm aware of that and most of the places where we capture `self` I would want to use this, but we can't right now unfortunately as you noted because we don't support C++14 features yet on all of our compilers.
Comment on attachment 8833537 [details] [diff] [review]
Part 1: Correct the RefCountedInsideLambda Check to complain about capturing and using `this` without a backing strong reference

[Security approval request comment]
How easily could an exploit be constructed based on the patch? I imagine that this patch would point out potential exploits, but as far as I know, this patch doesn't exactly paint a complete picture of how one would perform the exploit.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? It is very easy to see what the problem being fixed is by looking at the contents of the patches.

Which older supported branches are affected by this flaw? 45esr, 61, 52, 53

If not all supported branches, which bug introduced the flaw? N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Yes - they are attached on this bug.

How likely is this patch to cause regressions; how much testing does it need? This patch is fairly unlikely to cause regressions.
Attachment #8833537 - Flags: sec-approval?
Attachment #8833509 - Flags: sec-approval?
Attachment #8833510 - Flags: sec-approval?
Attachment #8833511 - Flags: sec-approval?
Attachment #8837289 - Flags: sec-approval?
Comment on attachment 8833537 [details] [diff] [review]
Part 1: Correct the RefCountedInsideLambda Check to complain about capturing and using `this` without a backing strong reference

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Potential Security Holes
[Is this code covered by automated tests?]: This is a series of systematic changes to existing changes, so yes and no.
[Has the fix been verified in Nightly?]: sec-approval? is currently pending on the nightly versions of these patches.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Series of systematic changes which elimates potential use-after-free errors.
[String changes made/needed]: None
Attachment #8833537 - Flags: approval-mozilla-aurora?
Attachment #8833509 - Flags: approval-mozilla-aurora?
Attachment #8833510 - Flags: approval-mozilla-aurora?
Attachment #8834559 - Flags: approval-mozilla-aurora?
Attachment #8834562 - Flags: approval-mozilla-aurora?
Comment on attachment 8837289 [details] [diff] [review]
Part 5: Workaround lambda static analysis in nsMultiMixedConv

My understanding is that the changes which caused this change to be required were uplifted, and thus are needed on all branches.
Attachment #8837289 - Flags: approval-mozilla-aurora?
Comment on attachment 8834614 [details] [diff] [review]
Part 1: BETA Correct the RefCountedInsideLambda Check to complain about capturing and using `this` without a backing strong reference

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Potential Security Holes
[Is this code covered by automated tests?]: This is a series of systematic changes to existing changes, so yes and no.
[Has the fix been verified in Nightly?]: sec-approval? is currently pending on the nightly versions of these patches.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Series of systematic changes which elimates potential use-after-free errors.
[String changes made/needed]: None
Attachment #8834614 - Flags: approval-mozilla-beta?
Attachment #8834615 - Flags: approval-mozilla-beta?
Attachment #8834616 - Flags: approval-mozilla-beta?
Attachment #8834617 - Flags: approval-mozilla-beta?
Attachment #8834618 - Flags: approval-mozilla-beta?
Attachment #8837289 - Flags: approval-mozilla-beta?
Comment on attachment 8836217 [details] [diff] [review]
Part 1: ESR45 Correct the RefCountedInsideLambda Check to complain about capturing and using `this` without a backing strong reference

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high bugh
User impact if declined: Potential Security Holes 
Fix Landed on Version: Currently awaiting sec-approval and uplifts to beta, aurora, and nightly.
Risk to taking this patch (and alternatives if risky): Low risk - small number of changes which are mechanical and low risk.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8836217 - Flags: approval-mozilla-esr45?
Attachment #8836218 - Flags: approval-mozilla-esr45?
OMG, so many flags.

sec-approval+ for trunk.

You only need to request sec-approval? on a single trunk patch in the future. :-)

I'll approve aurora and beta as well, leaving ESR45 to release management but we should take it there.
Attachment #8833509 - Flags: sec-approval?
Attachment #8833509 - Flags: sec-approval+
Attachment #8833509 - Flags: approval-mozilla-aurora?
Attachment #8833509 - Flags: approval-mozilla-aurora+
Attachment #8833510 - Flags: sec-approval?
Attachment #8833510 - Flags: sec-approval+
Attachment #8833510 - Flags: approval-mozilla-aurora?
Attachment #8833510 - Flags: approval-mozilla-aurora+
Attachment #8833511 - Flags: sec-approval? → sec-approval+
Attachment #8833537 - Flags: sec-approval?
Attachment #8833537 - Flags: sec-approval+
Attachment #8833537 - Flags: approval-mozilla-aurora?
Attachment #8833537 - Flags: approval-mozilla-aurora+
Attachment #8834559 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8834562 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8834614 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8834615 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8834616 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8834617 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8834618 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8837289 - Flags: sec-approval?
Attachment #8837289 - Flags: sec-approval+
Attachment #8837289 - Flags: approval-mozilla-beta?
Attachment #8837289 - Flags: approval-mozilla-beta+
Attachment #8837289 - Flags: approval-mozilla-aurora?
Attachment #8837289 - Flags: approval-mozilla-aurora+
(In reply to Al Billings [:abillings] from comment #46)
> OMG, so many flags.

Sorry :S - I couldn't remember if I needed to request it for each patch, so I just went through and requested all (19?) flags.

I imagine that it was a pain to clear all of them. It was a pain to request them so :P

> sec-approval+ for trunk.

Thanks
Once this lands, it should fix bug 1339933 as a side-effect.
Flags: needinfo?(michael)
I've managed to get everything green again - it turned out that there were 3 main problems:
 1) There was a small amount of windows & OS X specific code which failed the static analysis. I have written a patch for each.
 2) CamerasChild is not safe to addref everywhere, and the callsites which I modified blocked their calling scope anyway for the response to return, so I would run into problems due to AddRefing CamerasChild on the wrong thread when I didn't need to.
 3) There was a lifecycle problem with the decoder which meant that firefox would hang on shutdown if I took a (another unnecessary) reference in the Init() function.

These should be fixed in the 4 patches which I am attaching.

MozReview-Commit-ID: 8b5KK7sL6wb
Attachment #8838692 - Flags: review?(jwwang)
MozReview-Commit-ID: B4dYo4ETzkL
Attachment #8838694 - Flags: review?(aklotz)
MozReview-Commit-ID: BHWJMHNgdRu
Attachment #8838695 - Flags: review?(schien)
Comment on attachment 8838696 [details] [diff] [review]
Part 9: Avoid MediaDecoder shutdown hang due to unnecessary strong reference capture

Review of attachment 8838696 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2807,2 @@
>    mOnMediaNotSeekable = mReader->OnMediaNotSeekable().Connect(
> +    OwnerThread(), this, &MediaDecoderStateMachine::SetMediaNotSeekable);

i dont see how that could make a difference, as the two conde are in effect identical:

https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaEventSource.h#433

this continues to the same MediaEventSource::ConnectInternal, which store this into a RefPtr and capture it.
doh, forget my earlier comment, it's RawPtr...
Comment on attachment 8838694 [details] [diff] [review]
Part 7: Stop capturing this by value in windows-only code

Review of attachment 8838694 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/mscom/MainThreadHandoff.cpp
@@ +12,5 @@
>  #include "mozilla/mscom/Utils.h"
>  #include "mozilla/Assertions.h"
>  #include "mozilla/DebugOnly.h"
>  #include "nsThreadUtils.h"
> +#include "nsProxyRelease.h"

nit: please move this up a line to keep the includes in alphabetical order.

::: ipc/mscom/WeakRef.cpp
@@ +11,5 @@
>  #include "mozilla/DebugOnly.h"
>  #include "mozilla/RefPtr.h"
>  #include "nsThreadUtils.h"
>  #include "nsWindowsHelpers.h"
> +#include "nsProxyRelease.h"

nit: please move this up two lines to keep the includes in alphabetical order.
Attachment #8838694 - Flags: review?(aklotz) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #54)
> this continues to the same MediaEventSource::ConnectInternal, which store
> this into a RefPtr and capture it.

It captures it in a RawPtr, not a RefPtr. a RawPtr is a wrapper type which they use to bypass this static analysis. https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/dom/media/MediaEventSource.h#143-154
Comment on attachment 8838696 [details] [diff] [review]
Part 9: Avoid MediaDecoder shutdown hang due to unnecessary strong reference capture

Review of attachment 8838696 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2807,2 @@
>    mOnMediaNotSeekable = mReader->OnMediaNotSeekable().Connect(
> +    OwnerThread(), this, &MediaDecoderStateMachine::SetMediaNotSeekable);

It is different.

https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/dom/media/MediaEventSource.h#144

RawPtr allows capturing a ref-counting pointer without causing SA errors.
Attachment #8838696 - Flags: review?(jwwang) → review+
FWIW, RawPtr sounds very worrisome class to have.
(In reply to Olli Pettay [:smaug] from comment #59)
> FWIW, RawPtr sounds very worrisome class to have.

Perhaps the template for RawPtr should be something like
template<typename T, const char* ReasonThisIsSafeToUseWithProof> class RawPtr...
1/2 :-)
Comment on attachment 8838692 [details] [diff] [review]
Part 6: Avoid AddRefing/Releasing CamerasChild on the wrong thread

Review of attachment 8838692 [details] [diff] [review]:
-----------------------------------------------------------------

Forward CamerasChild.cpp to Randell.
Attachment #8838692 - Flags: review?(rjesup)
Attachment #8838692 - Flags: review?(jwwang)
Attachment #8838692 - Flags: review+
(In reply to Randell Jesup [:jesup] from comment #60)
> (In reply to Olli Pettay [:smaug] from comment #59)
> > FWIW, RawPtr sounds very worrisome class to have.
> 
> Perhaps the template for RawPtr should be something like
> template<typename T, const char* ReasonThisIsSafeToUseWithProof> class
> RawPtr...
> 1/2 :-)

detail::RawPtr is for internal use only and shouldn't be a concern of the client code. MediaEventSource asserts the listener is disconnect before deletion to ensure no dangling pointers are used.
Comment on attachment 8838692 [details] [diff] [review]
Part 6: Avoid AddRefing/Releasing CamerasChild on the wrong thread

Review of attachment 8838692 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't search down through all the fun layers of calls, but my only concern is the code that in the Lambda version went out of it's way to convert the bool returns from SendFoo() to nsresult returns... which is now gone.  I want to ensure that all the failure paths for broken IPC/etc still work correctly without it.  If that's ok, great.  Forwarding review to gcp who wrote this
Attachment #8838692 - Flags: review?(rjesup) → review?(gpascutto)
Comment on attachment 8838695 [details] [diff] [review]
Part 8: Stop capturing this by value in OSX only code

Review of attachment 8838695 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for catching this one. :)
Attachment #8838695 - Flags: review?(schien) → review+
Comment on attachment 8838692 [details] [diff] [review]
Part 6: Avoid AddRefing/Releasing CamerasChild on the wrong thread

Review of attachment 8838692 [details] [diff] [review]:
-----------------------------------------------------------------

It doesn't look like there's anything that checks for the failures of the Send* methods. If the IPC channel is so broken, looks like we're pretty hosed anyway (child might hang).
https://dxr.mozilla.org/mozilla-central/rev/a180b976c165b6cd174d24bbb77941919cdc53cb/dom/media/systemservices/CamerasChild.cpp#262

So I think that losing the return values is OK, assuming this has been properly tested etc etc.
Attachment #8838692 - Flags: review?(gpascutto) → review+
Also appears to have caused some win debug accessibility failures like https://treeherder.mozilla.org/logviewer.html#?job_id=79729982&repo=mozilla-inbound

The job might show as (a) or (a11y) depending on when you push related to the most recent treeherder deployment.
Go ahead and re-land with the affected assertion removed. It is of limited use anyway. r=me
Re-landing with the change suggested by aklotz in https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=57c46960a5f3
Flags: needinfo?(michael)
Comment on attachment 8836217 [details] [diff] [review]
Part 1: ESR45 Correct the RefCountedInsideLambda Check to complain about capturing and using `this` without a backing strong reference

Fix a sec-critical. ESR45+.
Attachment #8836217 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Attachment #8836218 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
has not landed in beta or aurora so far, do we still want this in esr45 when 52 is just about to go out of the door soon ?
Flags: needinfo?(jcristau)
Flags: needinfo?(gchang)
Given that this has already landed and stuck on m-c, I personally think we need this to land before we ship 52. But I'm also not sure where we stand on the branch patches with respect to whether they need rebasing or not. If Julian is on board, I would suggest that Michael rebase the branch patches and land them himself today still. But yeah, I definitely don't think we want this landing on ESR45 in the mean time.
Flags: needinfo?(michael)
Yes, probably not a good idea to leave a sec-crit fixed in trunk but not release for 6 weeks, so we should get some clarity on whether the branch patches here are still good to go and if possible land on aurora and beta today.
Flags: needinfo?(jcristau)
Depends on: 1343069
This fixes the last of the failures I was seeing on beta.

I talked with jwwang on irc about this and made the change and he said that it was safe.

MozReview-Commit-ID: Ll32AzpBXHo
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Depends on: 1343302
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+][adv-esr45.8+]
Group: dom-core-security → core-security-release
Group: core-security-release
Depends on: 1412254
No longer depends on: 1412254
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.