[clang-tidy] Add check to find calls to NSInvocation methods under ARC that don't have proper object argument lifetimes.

[mwyman]

Summary

This check is similar to an ARC Migration check that warned about this incorrect usage under ARC, but most projects are no longer undergoing migration from pre-ARC code. The documentation for NSInvocation is not explicit about these requirements and incorrect usage has been found in many of our projects.

[mwyman]

Check for Objective-C block pointers as well, which also require __unsafe_unretained when retrieved as arguments from NSInvocation under ARC.

[benhamilton]

clang-tools-extra/clang-tidy/objc/NsinvocationArgumentLifetimeCheck.cpp

72–74

Is there a bug filed for this? Seems like an oversight.

117

-[NSInvocation %0] should

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/objc/NsinvocationArgumentLifetimeCheck.cpp

36

Please use static instead of anonymous namespace for variable.

51

Please use static instead of anonymous namespace for functions.

84

Please don't use auto unless type is spelled in same statement or iterator. Please elide braces. Same in other places.

clang-tools-extra/docs/ReleaseNotes.rst

135

Please enclose NSInvocation in double back-ticks.

clang-tools-extra/docs/clang-tidy/checks/objc-nsinvocation-argument-lifetime.rst

7

Please enclose NSInvocation in double back-ticks. Same below.

[mwyman]

Responding to review feedback.

[mwyman]

clang-tools-extra/clang-tidy/objc/NsinvocationArgumentLifetimeCheck.cpp

72–74

Maybe I'm incorrectly reading this, but per the comment on QualifiedTypeLoc it seems like this is intentional at this time: https://github.com/llvm/llvm-project/blob/549e87f3d04bbae91bc7bc38609ce7073e2c8a6d/clang/include/clang/AST/TypeLoc.h#L275

117

Tweaked the format string so it now outputs "NSInvocation '-…'", since the diagnostic formatter inserts single-quotes, which would otherwise make it read "-[NSInvocation 'getArgument:atIndex:']", which seems weirder.

[mwyman]

Trying to fix Harbormaster build.

[mwyman]

Missed CHECK-FIXES for block argument.

[aaron.ballman]

LGTM aside from the file names concern.

clang-tools-extra/clang-tidy/objc/CMakeLists.txt

8

I think the files should be renamed to NSInvocationArgumentLifetimeCheck based on typical naming conventions for NS identifiers.

[mwyman]

Renamed NsinvocationArgumentLifetimeCheck -> NSInvocationArgumentLifetimeCheck to match other ObjC checker names.

[stephanemoore]

clang-tools-extra/clang-tidy/objc/NSInvocationArgumentLifetimeCheck.cpp

49 ↗

(On Diff #256070)

I'm not sure but I guess it's okay to assume that the enumeration ordering will remain stable?

clang-tools-extra/test/clang-tidy/checkers/objc-nsinvocation-argument-lifetime.m

65–70

How about a test case where we take the address of something on an ivar?

For example, I think the code below should not produce a diagnostic finding, right?

struct Example {
  __unsafe_unretained id Field;
};

@interface TestClass : NSObject {
  struct Example ExampleIvar;
}

@end

@implementation TestClass

- (void)processInvocation:(NSInvocation *)Invocation {
  [Invocation getArgument:&(self->ExampleIvar.Field) atIndex:2];
}

@end

65–70

Maybe worth adding an ivar case that doesn't produce a diagnostic?

[mwyman]

Updating per Stephane's comments. To deal with struct fields needed to match MemberRefExprs.

However, in doing so I discovered that the match conditions I thought were catching ObjcIvarRefExpr or MemberRefExpr were sometimes incorrectly matching on the self reference, which is itself a DeclRefExpr to an object with strong lifetime semantics. This meant correct usages would sometimes be caught, because the ObjcIvarRefExpr wouldn't match but the DeclRefExpr (to self) did.

That self (or other object) that is dereferenced (implicity or explicitly) has a parent ImplicitCastExpr in the AST, so I've dealt with this situation by excluding that case from matching the declRefExpr.

[mwyman]

Add matching MemberRefExpr case, not just the non-matching case there was before.

[mwyman]

clang-tools-extra/clang-tidy/objc/NSInvocationArgumentLifetimeCheck.cpp

49 ↗

(On Diff #256070)

I don't know why anyone would change the ordering, but FWIW this is the same approach used by the ARC migrator code.

clang-tools-extra/test/clang-tidy/checkers/objc-nsinvocation-argument-lifetime.m

65–70

This required checking MemberRefExprs too. Added both a matching and non-matching case here.

65–70

Added a non-matching ivar case, and in doing so discovered it was often actually matching the "self" reference, which is an ImplicitParamDecl, itself a type of VarDecl, which matched the declRefExpr(), leading to even cases that shouldn't have produced diagnostics to produce them.

Have fixed this by excluding the self case (by ensuring the declRefExpr's parent is not an implicitCastExpr, which only appears in the AST that I've seen around the self—or other dereferenced object—reference).

[mwyman]

Adding non-object types for argument to getArgument:atIndex: to verify they don't cause problems.

Done while investigating Harbormaster failures.

[mwyman]

Add check for isScalarType before getting the type, which seems to be triggering an assert. Maybe this is a Mac vs Linux thing?

[stephanemoore]

clang-tools-extra/test/clang-tidy/checkers/objc-nsinvocation-argument-lifetime.m

65–70

It might be good to also have a case using the address of a field of a struct local variable.

[mwyman]

Added struct member reference from local variable, per review comment.

clang-tools-extra/clang-tidy/objc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyObjCModule
   AvoidNSErrorInitCheck.cpp
   DeallocInCategoryCheck.cpp
   ForbiddenSubclassingCheck.cpp
   MissingHashCheck.cpp
+  NsinvocationArgumentLifetimeCheck.cpp
   ObjCTidyModule.cpp
   PropertyDeclarationCheck.cpp
   SuperSelfCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tools-extra/clang-tidy/objc/NsinvocationArgumentLifetimeCheck.h

+//===--- NsinvocationArgumentLifetimeCheck.h - clang-tidy -------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_NSINVOCATIONARGUMENTLIFETIMECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_NSINVOCATIONARGUMENTLIFETIMECHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/Basic/LangStandard.h"
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+/// Finds calls to NSInvocation methods under ARC that don't have proper
+/// argument object lifetimes.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/objc-nsinvocation-argument-lifetime.html
+class NsinvocationArgumentLifetimeCheck : public ClangTidyCheck {
+public:
+  NsinvocationArgumentLifetimeCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.ObjC && LangOpts.ObjCAutoRefCount;
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_NSINVOCATIONARGUMENTLIFETIMECHECK_H

clang-tools-extra/clang-tidy/objc/NsinvocationArgumentLifetimeCheck.cpp

+//===--- NsinvocationArgumentLifetimeCheck.cpp - clang-tidy ---------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "NsinvocationArgumentLifetimeCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Attrs.inc"
+#include "clang/AST/ComputeDependence.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/ExprObjC.h"
+#include "clang/AST/Type.h"
+#include "clang/AST/TypeLoc.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace objc {
+namespace {
+
+constexpr StringRef WeakText = "__weak";
+constexpr StringRef StrongText = "__strong";
+constexpr StringRef UnsafeUnretainedText = "__unsafe_unretained";
+
+/// Matches ObjCIvarRefExpr or DeclRefExpr that reference Objective-C object
+/// variables whose object lifetimes are not __unsafe_unretained.
+AST_POLYMORPHIC_MATCHER(isObjCManagedLifetime,
+                        AST_POLYMORPHIC_SUPPORTED_TYPES(ObjCIvarRefExpr,
+                                                        DeclRefExpr)) {
+  QualType QT = Node.getType();
+  return QT->getScalarTypeKind() == Type::STK_ObjCObjectPointer &&
+         QT.getQualifiers().getObjCLifetime() > Qualifiers::OCL_ExplicitNone;
+}
+
+llvm::Optional<FixItHint> replacementForOwnershipString(StringRef Text,
+                                                        CharSourceRange Range,
+                                                        StringRef Ownership) {
+  size_t Index = Text.find(Ownership);
+  if (Index == StringRef::npos)
+    return llvm::None;
+
+  SourceLocation Begin = Range.getBegin().getLocWithOffset(Index);
+  SourceLocation End = Begin.getLocWithOffset(Ownership.size());
+  return FixItHint::CreateReplacement(SourceRange(Begin, End),
+                                      UnsafeUnretainedText);
+}
+
+llvm::Optional<FixItHint> fixItForVarDecl(const VarDecl *VD,
+                                          const SourceManager &SM,
+                                          const LangOptions &LangOpts) {
+  assert(VD && "VarDecl parameter must not be null");
+  // Don't provide fix-its for any parameter variables at this time.
+  if (isa<ParmVarDecl>(VD))
+    return llvm::None;
+
+  // Currently there is no way to directly get the source range for the
+  // __weak/__strong ObjC lifetime qualifiers, so it's necessary to string
+  // search in the source code.
+  CharSourceRange Range = Lexer::makeFileCharRange(
+      CharSourceRange::getTokenRange(VD->getSourceRange()), SM, LangOpts);
+  if (Range.isInvalid()) {
+    // An invalid range likely means inside a macro, in which case don't supply
+    // a fix-it.
+    return llvm::None;
+  }
+
+  StringRef VarDeclText = Lexer::getSourceText(Range, SM, LangOpts);
+  if (auto Hint = replacementForOwnershipString(VarDeclText, Range, WeakText)) {
+    return Hint;
+  }
+
+  if (auto Hint =
+          replacementForOwnershipString(VarDeclText, Range, StrongText)) {
+    return Hint;
+  }
+
+  return FixItHint::CreateInsertion(Range.getBegin(), "__unsafe_unretained ");
+}
+
+} // namespace
+
+void NsinvocationArgumentLifetimeCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      objcMessageExpr(
+          hasReceiverType(asString("NSInvocation *")),
+          anyOf(hasSelector("getArgument:atIndex:"),
+                hasSelector("getReturnValue:")),
+          hasArgument(
+              0, anyOf(hasDescendant(objcIvarRefExpr(isObjCManagedLifetime())),
+                       hasDescendant(declRefExpr(to(varDecl().bind("var")),
+                                                 isObjCManagedLifetime())))))
+          .bind("call"),
+      this);
+}
+
+void NsinvocationArgumentLifetimeCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MatchedExpr = Result.Nodes.getNodeAs<ObjCMessageExpr>("call");
+
+  auto Diag = diag(MatchedExpr->getArg(0)->getBeginLoc(),
+                   "NSInvocation's %0 should only pass pointers to "
+                   "objects with ownership __unsafe_unretained")
+              << MatchedExpr->getSelector();
+
+  // Only provide fix-it hints for references to local variables; fixes for
+  // instance variable references don't have as clear an automated fix.
+  const auto *VD = Result.Nodes.getNodeAs<VarDecl>("var");
+  if (!VD)
+    return;
+
+  if (auto Hint = fixItForVarDecl(VD, *Result.SourceManager,
+                                  Result.Context->getLangOpts())) {
+    Diag << *Hint;
+  }
+}
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp

 //===--- ObjCTidyModule.cpp - clang-tidy --------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidNSErrorInitCheck.h"
 #include "DeallocInCategoryCheck.h"
 #include "ForbiddenSubclassingCheck.h"
 #include "MissingHashCheck.h"
+#include "NsinvocationArgumentLifetimeCheck.h"
 #include "PropertyDeclarationCheck.h"
 #include "SuperSelfCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace objc {
 
 class ObjCModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidNSErrorInitCheck>(
         "objc-avoid-nserror-init");
     CheckFactories.registerCheck<DeallocInCategoryCheck>(
         "objc-dealloc-in-category");
     CheckFactories.registerCheck<ForbiddenSubclassingCheck>(
         "objc-forbidden-subclassing");
     CheckFactories.registerCheck<MissingHashCheck>(
         "objc-missing-hash");
+    CheckFactories.registerCheck<NsinvocationArgumentLifetimeCheck>(
+        "objc-nsinvocation-argument-lifetime");
     CheckFactories.registerCheck<PropertyDeclarationCheck>(
         "objc-property-declaration");
     CheckFactories.registerCheck<SuperSelfCheck>(
         "objc-super-self");
   }
 };
 
 // Register the ObjCTidyModule using this statically initialized variable.

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`objc-dealloc-in-category
 
   Finds implementations of -dealloc in Objective-C categories.
 
 - New :doc:`misc-no-recursion
   <clang-tidy/checks/misc-no-recursion>` check.
 
   Finds recursive functions and diagnoses them.
 
+- New :doc:`objc-nsinvocation-argument-lifetime
+  <clang-tidy/checks/objc-nsinvocation-argument-lifetime>` check.
+
+  Finds calls to NSInvocation methods under ARC that don't have proper
+  argument object lifetimes.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-con36-c
   <clang-tidy/checks/cert-con36-c>` to
   :doc:`bugprone-spuriously-wake-up-functions
   <clang-tidy/checks/bugprone-spuriously-wake-up-functions>` was added.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `modernize-use-uncaught-exceptions <modernize-use-uncaught-exceptions.html>`_, "Yes"
    `modernize-use-using <modernize-use-using.html>`_, "Yes"
    `mpi-buffer-deref <mpi-buffer-deref.html>`_, "Yes"
    `mpi-type-mismatch <mpi-type-mismatch.html>`_, "Yes"
    `objc-avoid-nserror-init <objc-avoid-nserror-init.html>`_,
    `objc-dealloc-in-category <objc-dealloc-in-category.html>`_,
    `objc-forbidden-subclassing <objc-forbidden-subclassing.html>`_,
    `objc-missing-hash <objc-missing-hash.html>`_,
+   `objc-nsinvocation-argument-lifetime <objc-nsinvocation-argument-lifetime.html>`_, "Yes"
    `objc-property-declaration <objc-property-declaration.html>`_, "Yes"
    `objc-super-self <objc-super-self.html>`_, "Yes"
    `openmp-exception-escape <openmp-exception-escape.html>`_,
    `openmp-use-default-none <openmp-use-default-none.html>`_,
    `performance-faster-string-find <performance-faster-string-find.html>`_, "Yes"
    `performance-for-range-copy <performance-for-range-copy.html>`_, "Yes"
    `performance-implicit-conversion-in-loop <performance-implicit-conversion-in-loop.html>`_,
    `performance-inefficient-algorithm <performance-inefficient-algorithm.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/objc-nsinvocation-argument-lifetime.rst

+.. title:: clang-tidy - objc-nsinvocation-argument-lifetime
+
+objc-nsinvocation-argument-lifetime
+===================================
+
+Finds calls to NSInvocation methods under ARC that don't have proper
+argument object lifetimes. When passing Objective-C objects as parameters
+to the NSInvocation methods ``getArgument:atIndex:`` and ``getReturnValue:``,
+the values are copied by value into the argument pointer, which leads to
+to incorrect releasing behavior if the object pointers are not declared
+``__unsafe_unretained``.
+
+For code:
+
+.. code-block:: objc
+
+    id arg;
+    [invocation getArgument:&arg atIndex:2];
+
+    __strong id returnValue;
+    [invocation getReturnValue:&returnValue];
+
+The fix will be:
+
+.. code-block:: objc
+
+    __unsafe_unretained id arg;
+    [invocation getArgument:&arg atIndex:2];
+
+    __unsafe_unretained id returnValue;
+    [invocation getReturnValue:&returnValue];
+
+The check will warn on being passed instance variable references that have
+lifetimes other than ``__unsafe_unretained``, but does not propose a fix:
+
+.. code-block:: objc
+
+    // "id _returnValue" is declaration of instance variable of class.
+    [invocation getReturnValue:&self->_returnValue];

clang-tools-extra/test/clang-tidy/checkers/objc-nsinvocation-argument-lifetime.m

+// RUN: %check_clang_tidy %s objc-nsinvocation-argument-lifetime %t -- -fobjc-arc
+
+__attribute__((objc_root_class))
+@interface NSObject
+@end
+
+@interface NSInvocation : NSObject
+- (void)getArgument:(void *)Arg atIndex:(int)Index;
+- (void)getReturnValue:(void *)ReturnValue;
+@end
+
+@interface OtherClass : NSObject
+- (void)getArgument:(void *)Arg atIndex:(int)Index;
+@end
+
+void foo(NSInvocation *Invocation) {
+  __unsafe_unretained id Arg2;
+  id Arg3;
+  // CHECK-FIXES: __unsafe_unretained id Arg3;
+  NSObject __strong *Arg4;
+  // CHECK-FIXES: NSObject __unsafe_unretained *Arg4;
+  __weak id Arg5;
+  // CHECK-FIXES: __unsafe_unretained id Arg5;
+  id ReturnValue;
+  // CHECK-FIXES: __unsafe_unretained id ReturnValue;
+
+  [Invocation getArgument:&Arg2 atIndex:2];
+
+  [Invocation getArgument:&Arg3 atIndex:3];
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation's 'getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+
+  [Invocation getArgument:&Arg4 atIndex:4];
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation's 'getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+
+  [Invocation getArgument:&Arg5 atIndex:5];
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation's 'getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+
+  [Invocation getReturnValue:&ReturnValue];
+  // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: NSInvocation's 'getReturnValue:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+
+  [Invocation getArgument:(void*)0 atIndex:6];
+}
+
+void bar(OtherClass *OC) {
+  id Arg;
+  [OC getArgument:&Arg atIndex:2];
+}
+
+@interface TestClass : NSObject {
+  id Argument;
+}
+@end
+
+@implementation TestClass
+
+- (void)processInvocation:(NSInvocation *)Invocation {
+  [Invocation getArgument:&Argument atIndex:2];
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation's 'getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+  [Invocation getArgument:&self->Argument atIndex:2];
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation's 'getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+}
+
+@end