[clang] 77a599a - [analyzer] Fix false negative when using a nullable parameter directly without binding to a variable

[Balazs Benics via cfe-commits]

Author: tripleCC
Date: 2023-07-03T09:28:41+02:00
New Revision: 77a599ae5828df343e2aa6acc634fe9acb152c99

URL: https://github.com/llvm/llvm-project/commit/77a599ae5828df343e2aa6acc634fe9acb152c99
DIFF: https://github.com/llvm/llvm-project/commit/77a599ae5828df343e2aa6acc634fe9acb152c99.diff

LOG: [analyzer] Fix false negative when using a nullable parameter directly without binding to a variable

If a parameter has a nullability annotation, the nullability information
of the parameter should be set as a NullabilityState trait at the
beginning of the function.

Patch By tripleCC!

Differential Revision: https://reviews.llvm.org/D153017

Added: 
    

Modified: 
    clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
    clang/test/Analysis/nullability.mm

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 11d5e77db0c731..635b6e4f6bd71d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -26,12 +26,13 @@
 
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 
+#include "clang/Analysis/AnyCall.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Path.h"
@@ -81,7 +82,8 @@ class NullabilityChecker
     : public Checker<check::Bind, check::PreCall, check::PreStmt<ReturnStmt>,
                      check::PostCall, check::PostStmt<ExplicitCastExpr>,
                      check::PostObjCMessage, check::DeadSymbols, eval::Assume,
-                     check::Location, check::Event<ImplicitNullDerefEvent>> {
+                     check::Location, check::Event<ImplicitNullDerefEvent>,
+                     check::BeginFunction> {
 
 public:
   // If true, the checker will not diagnose nullabilility issues for calls
@@ -102,6 +104,7 @@ class NullabilityChecker
   void checkEvent(ImplicitNullDerefEvent Event) const;
   void checkLocation(SVal Location, bool IsLoad, const Stmt *S,
                      CheckerContext &C) const;
+  void checkBeginFunction(CheckerContext &Ctx) const;
   ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
                              bool Assumption) const;
 
@@ -563,6 +566,37 @@ void NullabilityChecker::checkEvent(ImplicitNullDerefEvent Event) const {
   }
 }
 
+void NullabilityChecker::checkBeginFunction(CheckerContext &C) const {
+  if (!C.inTopFrame())
+    return;
+
+  const LocationContext *LCtx = C.getLocationContext();
+  auto AbstractCall = AnyCall::forDecl(LCtx->getDecl());
+  if (!AbstractCall || AbstractCall->parameters().empty())
+    return;
+
+  ProgramStateRef State = C.getState();
+  for (const ParmVarDecl *Param : AbstractCall->parameters()) {
+    if (!isValidPointerType(Param->getType()))
+      continue;
+
+    Nullability RequiredNullability =
+        getNullabilityAnnotation(Param->getType());
+    if (RequiredNullability != Nullability::Nullable)
+      continue;
+
+    const VarRegion *ParamRegion = State->getRegion(Param, LCtx);
+    const MemRegion *ParamPointeeRegion =
+        State->getSVal(ParamRegion).getAsRegion();
+    if (!ParamPointeeRegion)
+      continue;
+
+    State = State->set<NullabilityMap>(ParamPointeeRegion,
+                                       NullabilityState(RequiredNullability));
+  }
+  C.addTransition(State);
+}
+
 // Whenever we see a load from a typed memory region that's been annotated as
 // 'nonnull', we want to trust the user on that and assume that it is is indeed
 // non-null.

diff  --git a/clang/test/Analysis/nullability.mm b/clang/test/Analysis/nullability.mm
index 44c241e07ee50a..5a702f86564a61 100644
--- a/clang/test/Analysis/nullability.mm
+++ b/clang/test/Analysis/nullability.mm
@@ -145,6 +145,17 @@ void testArgumentTracking(Dummy *_Nonnull nonnull, Dummy *_Nullable nullable) {
   }
 }
 
+void testArgumentTrackingDirectly(Dummy *_Nonnull nonnull, Dummy *_Nullable nullable) {
+  switch(getRandom()) {
+  case 1: testMultiParamChecking(nonnull, nullable, nonnull); break;
+  case 2: testMultiParamChecking(nonnull, nonnull, nonnull); break;
+  case 3: testMultiParamChecking(nonnull, nullable, nullable); break; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 3rd parameter}}
+  case 4: testMultiParamChecking(nullable, nullable, nonnull); // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+  case 5: testMultiParamChecking(nullable, nullable, nullable); // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+  case 6: testMultiParamChecking((Dummy *_Nonnull)0, nullable, nonnull); break;
+  }
+}
+
 Dummy *_Nonnull testNullableReturn(Dummy *_Nullable a) {
   Dummy *p = a;
   return p; // expected-warning {{Nullable pointer is returned from a function that is expected to return a non-null value}}