[StaticAnalyzer] Fix false negative on NilArgChecker when creating li…

…teral object Fix false negative on NilArgChecker when creating literal object, such as @[nullableObject]. Patch By tripleCC! Differential Revision: https://reviews.llvm.org/D152269
llvm · Jun 8, 2023 · fa6b7dd · fa6b7dd
1 parent 15f15ab
commit fa6b7dd
Show file tree

Hide file tree

Showing 2 changed files with 98 additions and 47 deletions.
diff --git a/clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp b/clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
@@ -95,56 +95,64 @@ static FoundationClass findKnownClass(const ObjCInterfaceDecl *ID,
 //===----------------------------------------------------------------------===//
 
 namespace {
-  class NilArgChecker : public Checker<check::PreObjCMessage,
-                                       check::PostStmt<ObjCDictionaryLiteral>,
-                                       check::PostStmt<ObjCArrayLiteral> > {
-    mutable std::unique_ptr<APIMisuse> BT;
-
-    mutable llvm::SmallDenseMap<Selector, unsigned, 16> StringSelectors;
-    mutable Selector ArrayWithObjectSel;
-    mutable Selector AddObjectSel;
-    mutable Selector InsertObjectAtIndexSel;
-    mutable Selector ReplaceObjectAtIndexWithObjectSel;
-    mutable Selector SetObjectAtIndexedSubscriptSel;
-    mutable Selector ArrayByAddingObjectSel;
-    mutable Selector DictionaryWithObjectForKeySel;
-    mutable Selector SetObjectForKeySel;
-    mutable Selector SetObjectForKeyedSubscriptSel;
-    mutable Selector RemoveObjectForKeySel;
-
-    void warnIfNilExpr(const Expr *E,
-                       const char *Msg,
-                       CheckerContext &C) const;
-
-    void warnIfNilArg(CheckerContext &C,
-                      const ObjCMethodCall &msg, unsigned Arg,
-                      FoundationClass Class,
-                      bool CanBeSubscript = false) const;
-
-    void generateBugReport(ExplodedNode *N,
-                           StringRef Msg,
-                           SourceRange Range,
-                           const Expr *Expr,
-                           CheckerContext &C) const;
-
-  public:
-    void checkPreObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const;
-    void checkPostStmt(const ObjCDictionaryLiteral *DL,
-                       CheckerContext &C) const;
-    void checkPostStmt(const ObjCArrayLiteral *AL,
-                       CheckerContext &C) const;
-  };
+class NilArgChecker : public Checker<check::PreObjCMessage,
+                                     check::PostStmt<ObjCDictionaryLiteral>,
+                                     check::PostStmt<ObjCArrayLiteral>,
+                                     EventDispatcher<ImplicitNullDerefEvent>> {
+  mutable std::unique_ptr<APIMisuse> BT;
+
+  mutable llvm::SmallDenseMap<Selector, unsigned, 16> StringSelectors;
+  mutable Selector ArrayWithObjectSel;
+  mutable Selector AddObjectSel;
+  mutable Selector InsertObjectAtIndexSel;
+  mutable Selector ReplaceObjectAtIndexWithObjectSel;
+  mutable Selector SetObjectAtIndexedSubscriptSel;
+  mutable Selector ArrayByAddingObjectSel;
+  mutable Selector DictionaryWithObjectForKeySel;
+  mutable Selector SetObjectForKeySel;
+  mutable Selector SetObjectForKeyedSubscriptSel;
+  mutable Selector RemoveObjectForKeySel;
+
+  void warnIfNilExpr(const Expr *E, const char *Msg, CheckerContext &C) const;
+
+  void warnIfNilArg(CheckerContext &C, const ObjCMethodCall &msg, unsigned Arg,
+                    FoundationClass Class, bool CanBeSubscript = false) const;
+
+  void generateBugReport(ExplodedNode *N, StringRef Msg, SourceRange Range,
+                         const Expr *Expr, CheckerContext &C) const;
+
+public:
+  void checkPreObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const;
+  void checkPostStmt(const ObjCDictionaryLiteral *DL, CheckerContext &C) const;
+  void checkPostStmt(const ObjCArrayLiteral *AL, CheckerContext &C) const;
+};
 } // end anonymous namespace
 
 void NilArgChecker::warnIfNilExpr(const Expr *E,
                                   const char *Msg,
                                   CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-  if (State->isNull(C.getSVal(E)).isConstrainedTrue()) {
+  auto Location = C.getSVal(E).getAs<Loc>();
+  if (!Location)
+    return;
+
+  auto [NonNull, Null] = C.getState()->assume(*Location);
 
+  // If it's known to be null.
+  if (!NonNull && Null) {
     if (ExplodedNode *N = C.generateErrorNode()) {
       generateBugReport(N, Msg, E->getSourceRange(), E, C);
+      return;
+    }
+  }
+
+  // If it might be null, assume that it cannot after this operation.
+  if (Null) {
+    // One needs to make sure the pointer is non-null to be used here.
+    if (ExplodedNode *N = C.generateSink(Null, C.getPredecessor())) {
+      dispatchEvent({*Location, /*IsLoad=*/false, N, &C.getBugReporter(),
+                     /*IsDirectDereference=*/false});
     }
+    C.addTransition(NonNull);
   }
 }
 

diff --git a/clang/test/Analysis/NSContainers.m b/clang/test/Analysis/NSContainers.m
@@ -1,4 +1,7 @@
-// RUN: %clang_analyze_cc1  -Wno-objc-literal-conversion -analyzer-checker=core,osx.cocoa.NonNilReturnValue,osx.cocoa.NilArg,osx.cocoa.Loops,debug.ExprInspection -verify -Wno-objc-root-class %s
+// RUN: %clang_analyze_cc1 -Wno-objc-literal-conversion -Wno-objc-root-class -fobjc-arc \
+// RUN:   -analyzer-checker=core,osx.cocoa,nullability \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-checker=debug.ExprInspection -verify %s
 
 void clang_analyzer_eval(int);
 
@@ -31,13 +34,13 @@ - (id)mutableCopy;
 @end
 
 typedef struct {
-  unsigned long state;
-  id *itemsPtr;
-  unsigned long *mutationsPtr;
-  unsigned long extra[5];
+    unsigned long state;
+    id __unsafe_unretained _Nullable * _Nullable itemsPtr;
+    unsigned long * _Nullable mutationsPtr;
+    unsigned long extra[5];
 } NSFastEnumerationState;
 @protocol NSFastEnumeration
-- (NSUInteger)countByEnumeratingWithState:(NSFastEnumerationState *)state objects:(id [])buffer count:(NSUInteger)len;
+- (NSUInteger)countByEnumeratingWithState:(NSFastEnumerationState *)state objects:(id __unsafe_unretained _Nullable [_Nonnull])buffer count:(NSUInteger)len;
 @end
 
 @interface NSArray : NSObject <NSCopying, NSMutableCopying, NSSecureCoding, NSFastEnumeration>
@@ -323,3 +326,43 @@ void testInlinedDefensiveCheck(NSMutableDictionary *dict, id obj) {
   // that 'obj' can be nil in this context.
   dict[obj] = getStringFromString(obj); // no-warning
 }
+
+Foo * getMightBeNullFoo();
+Foo * _Nonnull getNonnullFoo();
+Foo * _Nullable getNullableFoo();
+
+void testCreateDictionaryLiteralWithNullableArg() {
+  Foo *p1 = getMightBeNullFoo();
+  Foo *p2 = getNonnullFoo();
+  Foo *p3 = getNullableFoo();
+
+  clang_analyzer_eval(p1 == nil); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(p2 == nil); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(p3 == nil); // expected-warning {{UNKNOWN}}
+
+  (void)@{@"abc" : p1}; // no-warning
+  (void)@{@"abc" : p2}; // no-warning
+  (void)@{@"abc" : p3}; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null}}
+
+  clang_analyzer_eval(p1 == nil); // expected-warning {{FALSE}}
+  clang_analyzer_eval(p2 == nil); // expected-warning {{FALSE}}
+  clang_analyzer_eval(p3 == nil); // expected-warning {{FALSE}}
+}
+
+void testCreateArrayLiteralWithNullableArg() {
+  Foo *p1 = getMightBeNullFoo();
+  Foo *p2 = getNonnullFoo();
+  Foo *p3 = getNullableFoo();
+
+  clang_analyzer_eval(p1 == nil); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(p2 == nil); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(p3 == nil); // expected-warning {{UNKNOWN}}
+
+  (void)@[p1]; // no-warning
+  (void)@[p2]; // no-warning
+  (void)@[p3]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null}}
+
+  clang_analyzer_eval(p1 == nil); // expected-warning {{FALSE}}
+  clang_analyzer_eval(p2 == nil); // expected-warning {{FALSE}}
+  clang_analyzer_eval(p3 == nil); // expected-warning {{FALSE}}
+}