[clang] 09a1f09 - [analyzer] Do not report NSError null dereference for _Nonnull params.

[Artem Dergachev via cfe-commits]

Author: Valeriy Savchenko
Date: 2020-04-20T12:33:01+03:00
New Revision: 09a1f090509eb5971ade7089330008c6e4024d30

URL: https://github.com/llvm/llvm-project/commit/09a1f090509eb5971ade7089330008c6e4024d30
DIFF: https://github.com/llvm/llvm-project/commit/09a1f090509eb5971ade7089330008c6e4024d30.diff

LOG: [analyzer] Do not report NSError null dereference for _Nonnull params.

We want to trust user type annotations and stop assuming pointers declared
as _Nonnull still can be null. This functionality is implemented as part
of NullabilityChecker as it already tracks non-null types.

Patch by Valeriy Savchenko!

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

Added: 
    

Modified: 
    clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
    clang/test/Analysis/CheckNSError.m

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 0b8d100992a2..565d0eeef704 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -81,7 +81,7 @@ class NullabilityChecker
     : public Checker<check::Bind, check::PreCall, check::PreStmt<ReturnStmt>,
                      check::PostCall, check::PostStmt<ExplicitCastExpr>,
                      check::PostObjCMessage, check::DeadSymbols,
-                     check::Event<ImplicitNullDerefEvent>> {
+                     check::Location, check::Event<ImplicitNullDerefEvent>> {
   mutable std::unique_ptr<BugType> BT;
 
 public:
@@ -101,6 +101,8 @@ class NullabilityChecker
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
   void checkEvent(ImplicitNullDerefEvent Event) const;
+  void checkLocation(SVal Location, bool IsLoad, const Stmt *S,
+                     CheckerContext &C) const;
 
   void printState(raw_ostream &Out, ProgramStateRef State, const char *NL,
                   const char *Sep) const override;
@@ -503,6 +505,52 @@ void NullabilityChecker::checkEvent(ImplicitNullDerefEvent Event) const {
   }
 }
 
+// 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.
+//
+// We do so even if the value is known to have been assigned to null.
+// The user should be warned on assigning the null value to a non-null pointer
+// as opposed to warning on the later dereference of this pointer.
+//
+// \code
+//   int * _Nonnull var = 0; // we want to warn the user here...
+//   // . . .
+//   *var = 42;              // ...and not here
+// \endcode
+void NullabilityChecker::checkLocation(SVal Location, bool IsLoad,
+                                       const Stmt *S,
+                                       CheckerContext &Context) const {
+  // We should care only about loads.
+  // The main idea is to add a constraint whenever we're loading a value from
+  // an annotated pointer type.
+  if (!IsLoad)
+    return;
+
+  // Annotations that we want to consider make sense only for types.
+  const auto *Region =
+      dyn_cast_or_null<TypedValueRegion>(Location.getAsRegion());
+  if (!Region)
+    return;
+
+  ProgramStateRef State = Context.getState();
+
+  auto StoredVal = State->getSVal(Region).getAs<loc::MemRegionVal>();
+  if (!StoredVal)
+    return;
+
+  Nullability NullabilityOfTheLoadedValue =
+      getNullabilityAnnotation(Region->getValueType());
+
+  if (NullabilityOfTheLoadedValue == Nullability::Nonnull) {
+    // It doesn't matter what we think about this particular pointer, it should
+    // be considered non-null as annotated by the developer.
+    if (ProgramStateRef NewState = State->assume(*StoredVal, true)) {
+      Context.addTransition(NewState);
+    }
+  }
+}
+
 /// Find the outermost subexpression of E that is not an implicit cast.
 /// This looks through the implicit casts to _Nonnull that ARC adds to
 /// return expressions of ObjC types when the return type of the function or

diff  --git a/clang/test/Analysis/CheckNSError.m b/clang/test/Analysis/CheckNSError.m
index 6de98e85efe3..10890a8627a1 100644
--- a/clang/test/Analysis/CheckNSError.m
+++ b/clang/test/Analysis/CheckNSError.m
@@ -1,5 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,osx.cocoa.NSError,osx.coreFoundation.CFError -analyzer-store=region -verify -Wno-objc-root-class %s
-
+// RUN: %clang_analyze_cc1 -verify -Wno-objc-root-class %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=nullability \
+// RUN:   -analyzer-checker=osx.cocoa.NSError \
+// RUN:   -analyzer-checker=osx.coreFoundation.CFError
 
 typedef signed char BOOL;
 typedef int NSInteger;
@@ -18,6 +21,7 @@ + (id)errorWithDomain:(NSString *)domain code:(NSInteger)code userInfo:(NSDictio
 @interface A
 - (void)myMethodWhichMayFail:(NSError **)error;
 - (BOOL)myMethodWhichMayFail2:(NSError **)error;
+- (BOOL)myMethodWhichMayFail3:(NSError **_Nonnull)error;
 @end
 
 @implementation A
@@ -29,6 +33,11 @@ - (BOOL)myMethodWhichMayFail2:(NSError **)error {  // no-warning
   if (error) *error = [NSError errorWithDomain:@"domain" code:1 userInfo:0]; // no-warning
   return 0;
 }
+
+- (BOOL)myMethodWhichMayFail3:(NSError **_Nonnull)error {         // no-warning
+  *error = [NSError errorWithDomain:@"domain" code:1 userInfo:0]; // no-warning
+  return 0;
+}
 @end
 
 struct __CFError {};