[PATCH] D77722: [analyzer] Do not report NSError null dereference for _Nonnull params

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko created this revision.
vsavchenko added reviewers: NoQ, dcoughlin.
Herald added subscribers: cfe-commits, ASDenysPetrov, martong, Charusso, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, JDevlieghere, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: clang.

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 tracks non-null types already.  It could
be easily implemented as a part of NSError checker, but it would look like
a simple "shut up" plug as opposed to a more generic solution.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77722

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


Index: clang/test/Analysis/CheckNSError.m
===================================================================
--- clang/test/Analysis/CheckNSError.m
+++ clang/test/Analysis/CheckNSError.m
@@ -1,4 +1,4 @@
-// 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 -analyzer-checker=core,nullability,osx.cocoa.NSError,osx.coreFoundation.CFError -analyzer-store=region -verify -Wno-objc-root-class %s
 
 
 typedef signed char BOOL;
@@ -18,6 +18,7 @@
 @interface A
 - (void)myMethodWhichMayFail:(NSError **)error;
 - (BOOL)myMethodWhichMayFail2:(NSError **)error;
+- (BOOL)myMethodWhichMayFail3:(NSError **_Nonnull)error;
 @end
 
 @implementation A
@@ -29,6 +30,11 @@
   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 {};
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -81,7 +81,7 @@
     : 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 @@
   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,41 @@
   }
 }
 
+// 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.
+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.
+  auto Region = dyn_cast_or_null<TypedValueRegion>(Location.getAsRegion());
+  if (!Region)
+    return;
+
+  auto State = Context.getState();
+
+  auto StoredVal = State->getSVal(Region).getAs<loc::MemRegionVal>();
+  if (!StoredVal)
+    return;
+
+  auto 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 (auto 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D77722.255982.patch
Type: text/x-patch
Size: 3781 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200408/f47c8ed2/attachment.bin>