1323455 - check that |new T(...)| for refcounted T is always stored into a RefPtr<T>

Status: NEW

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

Description

[Nathan Froyd [:froydnj]]

Over in bug 1307961, we are talking about making MOZ_COUNT_CTOR/DTOR and refcounting incompatible: you can have one or the other, but not both.  The only hole this opens up is something like:

  // T is refcounted
  T* foo = new T(...)
  // ...forget to use |foo| and leak it

This leak can be caught by Valgrind, ASan, et al, and it could be caught by our current infrastructure if we permitted MOZ_COUNT_CTOR/DTOR to happily coexist with refcounting, but it wouldn't be caught by our infrastructure in the new world.

Static analysis to the rescue!  We can check that for all |new T(...)|, we have something like:

  RefPtr<T> x(new T(...));

or:

  RefPtr<T> y = new T(...);

or:

  RefPtr<T> z = MakeAndAddRef<T>(...);

and probably one or two other cases I have forgotten about.  Alternatively, we could just check that we always allocate such objects via MakeAndAddRef, but that may be untenable.  We probably also have to permit:

  Foo(new T(...), aOtherArg, ...);

because of historical reasons.  But it'd be a step in the right direction.

Comment 1

[(no longer active)]

Michael, do you feel like implementing this?

Comment 2

[Andi [:andi]]

If Michael is busy i can take it after Christmas.

Comment 3

[(no longer active)]

(In reply to Andi-Bogdan Postelnicu from comment #2)
> If Michael is busy i can take it after Christmas.

I think that's a good idea.  It would be a fun analysis to write.  Please go for it!  :-)

Comment 4

[Tristan Bourvon]

Attachment: Bug 1323455 - add checker to detect creation of refcounted class outside refptr

This checker detects assignment of refcounted objects into raw pointers for
variable/field initialization and assigment.
This includes an autofix suggestion to replace the raw pointer declaration with
a RefPtr.

Review commit: https://reviewboard.mozilla.org/r/152176/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152176/

Comment 5

[Tristan Bourvon]

Hopefully this commit covers most cases of the problem described. If you feel like there are corner cases which have completely been ignored, don't hesitate to point them to me.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8880810 [details]
Bug 1323455 - add checker to detect creation of refcounted class outside refptr

https://reviewboard.mozilla.org/r/152176/#review157438

::: build/clang-plugin/CustomMatchers.h:42
(Diff revision 1)
>  }
>  
> +/// This matcher will match the assignment operator
> +AST_MATCHER(BinaryOperator, binaryAssignmentOperator) {
> +  BinaryOperatorKind OpCode = Node.getOpcode();
> +  return OpCode == BO_Assign;

nit: Node.getOpcode() == BO_Assign;

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Tristan, do we have any occurrence of this issue in the current code base?

Comment 8

[Tristan Bourvon]

Comment on attachment 8880810 [details]
Bug 1323455 - add checker to detect creation of refcounted class outside refptr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152176/diff/1-2/

Comment 9

[Tristan Bourvon]

Comment on attachment 8880810 [details]
Bug 1323455 - add checker to detect creation of refcounted class outside refptr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152176/diff/2-3/

Comment 10

[Tristan Bourvon]

(In reply to Sylvestre Ledru [:sylvestre] from comment #7)
> Tristan, do we have any occurrence of this issue in the current code base?

Last time I ran the checker on the codebase probably close to 1000.

Comment 11

[Andi [:andi]]

I thinks this one suits best for auto-fix.

Comment 12

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

Comment on attachment 8880810 [details]
Bug 1323455 - add checker to detect creation of refcounted class outside refptr

https://reviewboard.mozilla.org/r/152176/#review158686

::: build/clang-plugin/CustomMatchers.h:40
(Diff revision 3)
>    return hasCustomAnnotation(&Node,
>                                           "moz_no_addref_release_on_return");
>  }
>  
> +/// This matcher will match the assignment operator
> +AST_MATCHER(BinaryOperator, binaryAssignmentOperator) {

Please use hasOperatorName("=")

::: build/clang-plugin/RefCountedToRawAssignmentChecker.cpp:9
(Diff revision 3)
> +
> +#include "RefCountedToRawAssignmentChecker.h"
> +#include "CustomMatchers.h"
> +
> +void RefCountedToRawAssignmentChecker::registerMatchers(
> +    MatchFinder *AstMatcher) {

I think I would prefer for our matcher to look for new expressions which do not have ancestor constructor invocations for RefPtr<T> or nsCOMPtr<T>. For example:

new T(); // Would error
nsCOMPtr<A>(static_cast<A*>(new B())); // Should be allowed
f(new T()); // Would error

nsCOMPtr<A> x = new B(); // Should be allowed (as there is an implicit constructor
nsCOMPtr<A> x(new B()); // Should be allowed (again implicit constructor)

class Foo {
  Foo() : mFoo(new B()) {} // Should be allowed
  RefPtr<B> mFoo;
};

I _think_ that in all of these cases clang inserts an implicit constructor invocation, but we should test this.

The unfortunate thing about this suggestion would be that it would be harder to generate the quickfix suggestion. Perhaps we could perform an additional optional match to find the declaration init or assignment, just for the fixit.

::: build/clang-plugin/tests/TestRefCountedToRawAssignment.cpp:27
(Diff revision 3)
> +  RC1* ptr2(new RC1); // expected-error {{assigning refcounted class RC1 to raw pointer ptr2}} \
> +                         expected-note {{consider making ptr2 a RefPtr}}
> +
> +  RC1* ptr3; // expected-note {{consider making ptr3 a RefPtr}}
> +  ptr3 = new RC1; // expected-error {{assigning refcounted class RC1 to raw pointer ptr3}}
> +}

Can we add another test:

class A { DECLARE_REFCOUNTING };
class B : public A {};

A* a = new B(); // Should error

---

Also, I would like to prevent something like this:

foo(new A());

Where `foo` takes a T* parameter.

Comment 13

[Tristan Bourvon]

Comment on attachment 8880810 [details]
Bug 1323455 - add checker to detect creation of refcounted class outside refptr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152176/diff/3-4/

Comment 14

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

Comment on attachment 8880810 [details]
Bug 1323455 - add checker to detect creation of refcounted class outside refptr

https://reviewboard.mozilla.org/r/152176/#review159332

r=me with the nits I mentioned fixed :) Looks awesome!

::: build/clang-plugin/CustomMatchers.h:156
(Diff revisions 3 - 4)
>      return Name == "AddRef" || Name == "Release";
>    }
>    return false;
>  }
>  
>  /// This matcher will select classes which are refcounted.

nit: Can you move this comment to your newly added matcher, and clarify that this will match classes which are refcounted and define their mRefCnt member?

::: build/clang-plugin/RefCountedToRawAssignmentChecker.cpp:10
(Diff revisions 3 - 4)
>  #include "RefCountedToRawAssignmentChecker.h"
>  #include "CustomMatchers.h"
>  
>  void RefCountedToRawAssignmentChecker::registerMatchers(
>      MatchFinder *AstMatcher) {
> +  AstMatcher->addMatcher(

I love the documentation on your matcher! Makes it very clear :)

::: build/clang-plugin/RefCountedToRawAssignmentChecker.cpp:25
(Diff revisions 3 - 4)
> -          binaryAssignmentOperator(),
> -          // The expression on the LHS refers to a var/field declaration,
> -          hasLHS(anyOf(declRefExpr(to(varDecl(
> -                           // which is of pointer type.
> -                           hasType(isAnyPointer()), decl().bind("decl")))),
> -                       memberExpr(hasDeclaration(fieldDecl(
> +              // which is a (possibly implicit) constructor expression,
> +              cxxConstructExpr(
> +                  // which is a ref counted pointer.
> +                  hasType(isRefPtr())))),
> +
> +          // and which optionnally has a corresponding declaration.

nit: optionally

::: build/clang-plugin/RefCountedToRawAssignmentChecker.cpp:94
(Diff revisions 3 - 4)
> +      }
> +
> +      unsigned ArgNum = 0;
> +      for (auto Arg : Call->arguments()) {
> +        if (IgnoreTrivials(Arg) == NewExpression) {
> +          Declaration = FuncDecl->getParamDecl(ArgNum);

Please check that `FuncDecl->getNumParams() > ArgNum` before calling `getParamDecl`. It is possible for the parameter list and the arguments list to have different lengths.

::: build/clang-plugin/tests/TestRefCountedToRawAssignment.cpp:32
(Diff revisions 3 - 4)
> +void f(RC1 *test) { // expected-note {{consider making test a RefPtr}}
> +
> +}
> +
>  void TestFunc() {
> -  RC1* ptr = new RC1; // expected-error {{assigning refcounted class RC1 to raw pointer ptr}} \
> +  new RC1; // expected-error {{assigning refcounted class RC1 to raw pointer}}

This error doesn't feel appropriate anymore. How about rewording it as:

`new RC1` must be immediately constructed into a smart pointer

(perhaps some more word doctoring could make it more clear).

::: build/clang-plugin/tests/TestRefCountedToRawAssignment.cpp:47
(Diff revisions 3 - 4)
> +  RC1* ptr4 = new RC2; // expected-error {{assigning refcounted class RC2 to raw pointer}} \
> +                          expected-note {{consider making ptr4 a RefPtr}}
> +
> +  f(new RC1); // expected-error {{assigning refcounted class RC1 to raw pointer}}
> +
> +  RefPtr<RC1> refptr(static_cast<RC1*>(new RC2)); // expected-error {{Unused "kungFuDeathGrip" 'RefPtr<RC1>' objects constructed from temporary values are prohibited}} \

nit: Please explicitly pass this refptr object to something to silence this error. It makes this file less clear.

::: build/clang-plugin/Utils.h:332
(Diff revision 4)
>      } else {
>        break;
>      }
>    }
>  
>    return s;

It could be nice to add an `assert(!IsTrivial(s))` as a validation that our `IsTrivial` and `IgnoreTrivials` implementations match.

Comment 15

[Tristan Bourvon]

Comment on attachment 8880810 [details]
Bug 1323455 - add checker to detect creation of refcounted class outside refptr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152176/diff/4-5/

Comment 16

[Tristan Bourvon]

Comment on attachment 8880810 [details]
Bug 1323455 - add checker to detect creation of refcounted class outside refptr

https://reviewboard.mozilla.org/r/152176/#review159332

Thanks for all the reviews Michael :)

Comment 17

[Tristan Bourvon]

Attachment: Bug 1323455 - add handling of assignment expressions; restrict to 1st party code

Review commit: https://reviewboard.mozilla.org/r/155724/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155724/

Comment 18

[Tristan Bourvon]

Comment on attachment 8880810 [details]
Bug 1323455 - add checker to detect creation of refcounted class outside refptr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152176/diff/5-6/

Comment 19

[Tristan Bourvon]

Comment on attachment 8880810 [details]
Bug 1323455 - add checker to detect creation of refcounted class outside refptr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152176/diff/6-7/

Comment 20

[Tristan Bourvon]

Comment on attachment 8884846 [details]
Bug 1323455 - add handling of assignment expressions; restrict to 1st party code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155724/diff/1-2/

Comment 21

[Tristan Bourvon]

Attachment: Bug 1323455 - refactor the checker to handle new exprs in return stmts

The matcher will now check that every call to a function returning a raw pointer
to a ref counted object uses the return value to construct a ref ptr.
This also fixes a small bug where isRefPtr() would not match specializations of
RefPtr<T>.

Review commit: https://reviewboard.mozilla.org/r/156498/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156498/

Comment 22

[Tristan Bourvon]

Comment on attachment 8880810 [details]
Bug 1323455 - add checker to detect creation of refcounted class outside refptr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152176/diff/7-8/

Comment 23

[Tristan Bourvon]

Comment on attachment 8884846 [details]
Bug 1323455 - add handling of assignment expressions; restrict to 1st party code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155724/diff/2-3/

Comment 24

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

Comment on attachment 8884846 [details]
Bug 1323455 - add handling of assignment expressions; restrict to 1st party code

https://reviewboard.mozilla.org/r/155724/#review161744

lgtm other than my comment below.

::: build/clang-plugin/CustomMatchers.h:186
(Diff revision 3)
> +AST_POLYMORPHIC_MATCHER(isFirstParty, AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, Stmt)) {
> +  return !inThirdPartyPath(&Node, &Finder->getASTContext()) &&
> +         !ASTIsInSystemHeader(Finder->getASTContext(), Node);
> +}

How is this going to support `Decl`? `inThirdPartyPath(Decl*)` doesn't accept an ASTContext as a second argument. You might need to also add that overload I think.

Comment 25

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

Comment on attachment 8885643 [details]
Bug 1323455 - refactor the checker to handle new exprs in return stmts

https://reviewboard.mozilla.org/r/156498/#review161746

I don't think we want to allow this. We should not allow functions to create refcounted objects and directly return them, and should instead require them to addref them themselves, and return an already_AddRefed.

Are there any places which we couldn't fix like that?

::: build/clang-plugin/tests/TestRefCountedToRawAssignment.cpp:32
(Diff revision 1)
> +RC1* f_ret() {
> +  return new RC1; // expected-note {{created using `new RC1` here}}
> +}

I'm fairly strongly of the opinion that we just shouldn't do this. How often does code like this appear in tree? Can we change all of it to return `already_AddRefed<RC1>` instead (just wrap the `new RC1` call with `do_AddRef`)?

Comment 26

[Tristan Bourvon]

Comment on attachment 8880810 [details]
Bug 1323455 - add checker to detect creation of refcounted class outside refptr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152176/diff/8-9/

Comment 27

[Tristan Bourvon]

Comment on attachment 8884846 [details]
Bug 1323455 - add handling of assignment expressions; restrict to 1st party code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155724/diff/3-4/

Comment 28

[Tristan Bourvon]

Comment on attachment 8885643 [details]
Bug 1323455 - refactor the checker to handle new exprs in return stmts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156498/diff/1-2/

Comment 29

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

Comment on attachment 8885643 [details]
Bug 1323455 - refactor the checker to handle new exprs in return stmts

https://reviewboard.mozilla.org/r/156498/#review162934

::: build/clang-plugin/RefCountedToRawAssignmentChecker.cpp:47
(Diff revision 2)
> +              // which is a variable declaration,
> +              varDecl(
> +                  // which has a ref counted pointer type.
> +                  hasType(isSmartPtrToRefCounted())))),
> +
> +          unless(hasAncestor(

You have a comment explaining each of the other cases. This one is pretty straightforward, but it would be nice to have a comment here too for consistency.

::: build/clang-plugin/RefCountedToRawAssignmentChecker.cpp:67
(Diff revision 2)
> -                      // This is a bit basic but probably sufficient until we
> +                  // This is a bit basic but probably sufficient until we
> -                      // have more powerful helpers to be able to have a full
> +                  // have more powerful helpers to be able to have a full
> -                      // coverage of this kind of cases.
> +                  // coverage of this kind of cases.
> -                      anyOf(declRefExpr(to(decl().bind("decl"))),
> +                  anyOf(declRefExpr(to(decl().bind("decl"))),
> -                            memberExpr(hasDeclaration(decl().bind("decl"))))))),
> +                        memberExpr(hasDeclaration(decl().bind("decl"))))))),
> +          hasNonTrivialParent(returnStmt(

A comment here could be nice too ^.^

::: build/clang-plugin/Utils.h:322
(Diff revision 2)
> +  }
> +
> +  return name;
> +}
> +
> +inline bool typeIsRefPtr(QualType Q) {

Could we consider having an annotation on the RefPtr type rather than doing a name string comparison to `RefPtr` and `nsCOMPtr`? It might be more clear to have a MOZ_IS_REFPTR and MOZ_IS_ALREADY_ADDREFED annotation, rather than looking at type names.

Unfortunately right now this test would stop working if, for example, we did a refactoring which renamed `already_AddRefed` to, say, `AlreadyAddRefed`, and forgot to update this analysis.

It's better when its clear that the type is "special" to the analysis in the code itself.

::: build/clang-plugin/Utils.h:342
(Diff revision 2)
> +
> +  if (!name) {
> +    return false;
> +  }
> +
> +  if (*name == "RefPtr" || *name == "nsCOMPtr" || *name == "OwningNonNull") {

Just return the condition here, rather than doing the `if (...) return true; return false;` thing.

::: build/clang-plugin/Utils.h:355
(Diff revision 2)
> +
> +  if (!name) {
> +    return false;
> +  }
> +
> +  if (*name == "already_AddRefed") {

Just `return  *name == "already_AddRefed"` (although I hope we'll stop doing name comparisons ^.^)

Comment 30

[Tristan Bourvon]

Comment on attachment 8880810 [details]
Bug 1323455 - add checker to detect creation of refcounted class outside refptr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152176/diff/9-10/

Comment 31

[Tristan Bourvon]

Comment on attachment 8884846 [details]
Bug 1323455 - add handling of assignment expressions; restrict to 1st party code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155724/diff/4-5/

Comment 32

[Tristan Bourvon]

Comment on attachment 8885643 [details]
Bug 1323455 - refactor the checker to handle new exprs in return stmts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156498/diff/2-3/

Comment 33

[Tristan Bourvon]

Comment on attachment 8885643 [details]
Bug 1323455 - refactor the checker to handle new exprs in return stmts

https://reviewboard.mozilla.org/r/156498/#review162934

Thanks for the review. I just added what you asked, plus I moved the refptr matchers from commit 3 to commit 1, and also added a condition in the matcher that the |new ...| is not in a call expression, so that it doesn't match stuff like do_AddRef(new X);.

Comment 34

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

Comment on attachment 8885643 [details]
Bug 1323455 - refactor the checker to handle new exprs in return stmts

https://reviewboard.mozilla.org/r/156498/#review165854

Sorry about the delay in the review.

::: build/clang-plugin/RefCountedToRawAssignmentChecker.cpp:42
(Diff revision 3)
>                              memberExpr(member(hasType(isSmartPtrToRefCounted()))))
>                    )))),
>  
> -          // and which optionally has a corresponding declaration.
> +          // and which doesn't have an ancestor,
> +          unless(hasAncestor(
> +              // which is a call expression (passing raw pointers is fine),

I don't think I agree with this - We only want to allow passing a new-ed raw pointer to some functions. Many functions will expect that a strong reference to the argument is being held.

Is there any chance we could special-case do_AddRef as an acceptable call expression? I don't want to allow you to call an arbitrary method with a new expression as an argument.

::: mfbt/AlreadyAddRefed.h:40
(Diff revision 3)
> -struct MOZ_MUST_USE_TYPE MOZ_NON_AUTOABLE already_AddRefed
> +struct MOZ_MUST_USE_TYPE MOZ_NON_AUTOABLE MOZ_IS_ALREADY_ADDREFED
> +already_AddRefed

I don't know what the style we should use here is - this looks kinda ugly with the already_AddRefed on a new line.

::: mfbt/Attributes.h:478
(Diff revision 3)
>   *   are disallowed by default unless they are marked as MOZ_IMPLICIT. This
>   *   attribute must be used for constructors which intend to provide implicit
>   *   conversions.
>   * MOZ_IS_REFPTR: Applies to class declarations of ref pointer to mark them as
>   *   such for use with static-analysis.
> + * MOZ_IS_SMARTPTR_TO_REFCOUNTED: Applies to class declarations of smart

This documentation should be added in the same patch as the annotation is added.

Comment 35

[Tristan Bourvon]

Comment on attachment 8880810 [details]
Bug 1323455 - add checker to detect creation of refcounted class outside refptr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152176/diff/10-11/

Comment 36

[Tristan Bourvon]

Comment on attachment 8884846 [details]
Bug 1323455 - add handling of assignment expressions; restrict to 1st party code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155724/diff/5-6/

Comment 37

[Tristan Bourvon]

Comment on attachment 8885643 [details]
Bug 1323455 - refactor the checker to handle new exprs in return stmts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156498/diff/3-4/

Comment 38

[Tristan Bourvon]

Comment on attachment 8885643 [details]
Bug 1323455 - refactor the checker to handle new exprs in return stmts

https://reviewboard.mozilla.org/r/156498/#review165854

> I don't think I agree with this - We only want to allow passing a new-ed raw pointer to some functions. Many functions will expect that a strong reference to the argument is being held.
> 
> Is there any chance we could special-case do_AddRef as an acceptable call expression? I don't want to allow you to call an arbitrary method with a new expression as an argument.

Please take a look at the first commit as it fixes this and some other points.

Comment 39

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

Comment on attachment 8885643 [details]
Bug 1323455 - refactor the checker to handle new exprs in return stmts

Awesome, so only annotated methods are considered now in part 1 :-)

I haven't read through all 3 patches again, but I think we're good now - thank you for making this analysis.

Comment 40

[(no longer active)]

Is this ready to land?  :-)

Comment 41

[Tristan Bourvon]

Not really, because all the issues it detects have to be fixed by hand before we can land it.
After our discussion where we established that it would be better to move to smart pointers most of the code we touch using this checker, we've had other discussions with the module owners about the way we should handle that.
Most were welcoming the changes, but warned us that there are many traps regarding thread safety and some special threads, as well as cycle risks.
This is why this checker is on hold as the work required to integrate it usefully is very big.

I'll let Andi add his thoughts on that, as he should probably be taking the final decision on the checker.

Comment 42

[Andi [:andi]]

I think we should first come to a consensus at what modules are affected by this change and after we decide this we should move further. We've started an email thread, to which i will add Ehsan and Michael in order to be more visible. 
As Tristan said the general opinion was that we should move as much as we can to smart pointers but there are many traps that should be handled with care and as a second node there are some portions that should not be migrated in order to to excessive use cpu cycles.

Comment 43

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Tristan, it looks like you are the assignee on this bug, but under a different email. Are you still working on this bug? If not, would you please unassign yourself?

Comment 44

[Tristan Bourvon]

Sylvestre, is this still interesting to you?

Comment 45

[Sylvestre Ledru [:Sylvestre]]

Yeah and no :)
We need to find an owner for that.

Comment 46

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

FWIW, deriving from SafeRefCounted enforces creation via MakeSafeRefPtr (see https://searchfox.org/mozilla-central/rev/1a47a74bd5ba89f2474aa27c40bd478e853f3276/dom/indexedDB/SafeRefPtr.h).