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

[vsavchenko]

Summary

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.

[vsavchenko]

Hi everyone!
I'm thinking about adding support for __attribute((nonnull))__ as well, but it should be handled a bit differently. That annotation is for parameters and not for types, so the corresponding constraints should be generated only when entering the function (as opposed to each load). I'm not sure that it should be a part of this commit though because it will have a completely standalone solution. Another question about __attribute((nonnull))__ is "Where should we put it?". It is quite reasonable to put it into NullabilityChecker, but NonnullParamChecker seems like a good candidate as well.

[vsavchenko]

Fix formatting issues in the test file

[NoQ]

NonnullParamChecker

Yup, looks like that's actually the one that deals with __attribute__((nonnull)), as opposed to _Nonnull.

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

521

[[ https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | Too much auto ]]!

527

([[ https://reviews.llvm.org/D33672?id=171506#inline-475812 | this auto is good ]])

537

Ok, so we're continuing normally if the value is already known to have been assigned to null. We could sink the analysis instead but presumably it's not our job as another checker must have warned before we get there (let's comment about this maybe).

[vsavchenko]

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

521

But this auto is also fine IMO as you can clearly see the actual type in the RHS. Mb const auto *Region at least?

537

Sure!

[NoQ]

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

521

Yup, this one should be const auto *.

[Szelethus]

In D77722#1969261, @vsavchenko wrote:

Hi everyone!
I'm thinking about adding support for __attribute((nonnull))__ as well, but it should be handled a bit differently. That annotation is for parameters and not for types, so the corresponding constraints should be generated only when entering the function (as opposed to each load). I'm not sure that it should be a part of this commit though because it will have a completely standalone solution. Another question about __attribute((nonnull))__ is "Where should we put it?". It is quite reasonable to put it into NullabilityChecker, but NonnullParamChecker seems like a good candidate as well.

Wait, we don't already assume nonnull attributed parameters as, well, not null? That's crazy.

[vsavchenko]

In D77722#1969391, @Szelethus wrote:

In D77722#1969261, @vsavchenko wrote:

Hi everyone!
I'm thinking about adding support for __attribute((nonnull))__ as well, but it should be handled a bit differently. That annotation is for parameters and not for types, so the corresponding constraints should be generated only when entering the function (as opposed to each load). I'm not sure that it should be a part of this commit though because it will have a completely standalone solution. Another question about __attribute((nonnull))__ is "Where should we put it?". It is quite reasonable to put it into NullabilityChecker, but NonnullParamChecker seems like a good candidate as well.

Wait, we don't already assume nonnull attributed parameters as, well, not null? That's crazy.

Yep, I was shocked by it as well. Maybe for some warnings it is checked in the end and they are truncated before reporting. However, DereferenceChecker definitely dispatches ImplicitNullDerefEvent for params marked with __attribute__((nonnull)).

[vsavchenko]

Fix review remarks

[NoQ]

In D77722#1969391, @Szelethus wrote:

Wait, we don't already assume nonnull attributed parameters as, well, not null? That's crazy.

That's only for the top frame. For nested stack frames we're not only assuming they're not null, we're also emitting a warning when they're null. But, yeah, it's clearly an unfortunate omission.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

536

That's still too much auto.

[vsavchenko]

Get rid of nonessential 'auto' in variable declaration

[vsavchenko]

Remove more auto's

[NoQ]

Thanks!

I'll commit.

[vsavchenko]

In D77722#1969551, @NoQ wrote:

Thanks!

I'll commit.

Awesome! Thanks!

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

enum class ErrorKind : int {
   NullableDereferenced,
   NullablePassedToNonnull
 };
 
 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:
   // If true, the checker will not diagnose nullabilility issues for calls
   // to system headers. This option is motivated by the observation that large
   // projects may have many nullability warnings. These projects may
   // find warnings about nullability annotations that they have explicitly
   // added themselves higher priority to fix than warnings on calls to system
   // libraries.
   DefaultBool NoDiagnoseCallsToSystemHeaders;
 
   void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const;
   void checkPostStmt(const ExplicitCastExpr *CE, CheckerContext &C) const;
   void checkPreStmt(const ReturnStmt *S, CheckerContext &C) const;
   void checkPostObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const;
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
   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;
 
   struct NullabilityChecksFilter {
     DefaultBool CheckNullPassedToNonnull;
     DefaultBool CheckNullReturnedFromNonnull;
     DefaultBool CheckNullableDereferenced;
if (Filter.CheckNullableDereferenced &&
     else {
       reportBug("Nullable pointer is passed to a callee that requires a "
                 "non-null", ErrorKind::NullablePassedToNonnull,
                 Event.SinkNode, Region, BR);
     }
   }
 }
 
+// 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
 /// method is non-null but the express is not.
 static const Expr *lookThroughImplicitCasts(const Expr *E) {
   return E->IgnoreImpCasts();
 }
 

clang/test/Analysis/CheckNSError.m

-// 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;
 typedef struct _NSZone NSZone;
 @class NSInvocation, NSMethodSignature, NSCoder, NSString, NSEnumerator;
 @protocol NSObject  - (BOOL)isEqual:(id)object; @end
 @protocol NSCopying  - (id)copyWithZone:(NSZone *)zone; @end
 @protocol NSCoding  - (void)encodeWithCoder:(NSCoder *)aCoder; @end
 @interface NSObject <NSObject> {} @end
 @class NSDictionary;
 @interface NSError : NSObject <NSCopying, NSCoding> {}
 + (id)errorWithDomain:(NSString *)domain code:(NSInteger)code userInfo:(NSDictionary *)dict;
 @end
 extern NSString * const NSXMLParserErrorDomain ;
 
 @interface A
 - (void)myMethodWhichMayFail:(NSError **)error;
 - (BOOL)myMethodWhichMayFail2:(NSError **)error;
+- (BOOL)myMethodWhichMayFail3:(NSError **_Nonnull)error;
 @end
 
 @implementation A
 - (void)myMethodWhichMayFail:(NSError **)error {   // expected-warning {{Method accepting NSError** should have a non-void return value to indicate whether or not an error occurred}}
   *error = [NSError errorWithDomain:@"domain" code:1 userInfo:0]; // expected-warning {{Potential null dereference}}
 }
 
 - (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 {};
 typedef struct __CFError* CFErrorRef;
 
 void foo(CFErrorRef* error) { // expected-warning {{Function accepting CFErrorRef* should have a non-void return value to indicate whether or not an error occurred}}
   *error = 0;  // expected-warning {{Potential null dereference}}
 }