1535530 - It's still possible to fool the MOZ_CAN_RUN_SCRIPT analysis to accept non-stack RefPtrs

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Consider code that looks like this:

  for (RefPtr<Foo>& cur : mSomeArray) {
    cur->DoStuff();
  }

This compiles if DoStuff is MOZ_CAN_RUN_SCRIPT. But it should not. Even though "cur" does have stack lifetime, the actual object involved does not!

What we're enforcing right now is that we have a declRefExpr, that the corresponding varDecl hasAutomaticStorageDuration(), and that the declRefExpr hasType(isSmartPtrToRefCounted()).

I don't know whether it's hasType() that is stripping off the reference bit or isSmartPtrToRefCounted() or something else, but the net result is that our test passes. A regression test for TestCanRunScript.cpp for when we fix this:

struct DisallowMemberArgsViaReferenceAlias {
  RefPtr<RefCountedBase> mRefCounted;
  MOZ_CAN_RUN_SCRIPT void foo() {
    RefPtr<RefCountedBase>& bogus = mRefCounted;
    bogus->method_test(); // expected-error {{arguments must all be strong refs or parent parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument)}}
  }
  MOZ_CAN_RUN_SCRIPT void bar() {
    RefPtr<RefCountedBase>& bogus = mRefCounted;
    test2(bogus);
  }
};

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:sylvestre, could you have a look please?

Comment 2

[Mirko Brodesser (:mbrodesser-Igalia)]

With the increased usage of MOZ_CAN_RUN_SCRIPT (https://searchfox.org/mozilla-central/search?q=MOZ_CAN_RUN_SCRIPT&case=true&regexp=true&path=), it's perhaps worth to reconsider the priority of this issue.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Funny how revisiting things after a bit makes them more obvious.... Or maybe I refactored this enough since filing that it's clear what to do. Anyway, fix coming up.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1535530. Fix can-run-script analysis to not mishandle on-stack refs to RefPtrs.

The key here is to test the type of the variable declaration for being a
smartptr type, instead of testing the type of the variable use.

Comment 5

[Mirko Brodesser (:mbrodesser-Igalia)]

Thanks for the fix.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/afbf73fb4073
Fix can-run-script analysis to not mishandle on-stack refs to RefPtrs. r=andi,masayuki

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/afbf73fb4073