[Static Analyzer] Make NonNullParamChecker emit implicit null dereference events.

[xazax.hun]

Summary

For most of the implicit null pointer dereferences (when a pointer is dereferenced and the static analyzer can not prove that the pointer is non null, and can not prove that the pointer is null either) the DereferenceChecker will emit an event. However in some cases (e.g. binding such pointers to references in function parameters) no event will be emitted. This patch addresses this problem by extending NonNullParamChecker to emit an event in such cases. Future checkers (listening to these events) will profit from this change. For example I am working on a checker for nullability qualifiers that depend on this patch.

[zaks.anna]

I believe we saw a regression in diagnostics with this modification. Gabor, do you recall what it was?

[xazax.hun]

The problem here is that, this checker can emit a warning for two cases:
1, Null was bound to a reference which should be reported as a dereference
2, Null was passed to parameter that should be non-null (marked by the attribute, not the qualifier) which should be reported appropriately

Right now exactly the same event will be triggered for both cases, so the checkers that process this event has no information whether it is a dereference or a value passed to a nonnull parameter, so it can not provide appropriate diagnostic for both cases.

One possibility would be to have two separate events, the other is to have an argument to an event that determines its origin. Probably it would be better to have two separate events, since it might be confusing to have dereference in the name of the event in the second case.

What do you think?

[xazax.hun]

Only send implicit dereference events, when the null pointer was bound to a reference.

[zaks.anna]

Please, commit this after committing the nullability checker so that this could have tests. Two tests need to be added:

1. the reference case
2. the attribute nonnull case

Also, I'd suggest adding a field to ImplicitNullDerefEvent instead of creating a new event.

[xazax.hun]

• Updated to latest trunk.
• Added a new field to the event with documentation.
• Rebased the patch on the top of nullability checker.
• Added covered cases to the nullability checker tests.

[zaks.anna]

Thanks!

cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h

 };
 
 /// \brief We dereferenced a location that may be null.
 struct ImplicitNullDerefEvent {
   SVal Location;
   bool IsLoad;
   ExplodedNode *SinkNode;
   BugReporter *BR;
+  // When true, the dereference is in the source code directly. When false, the
+  // dereference might happen later (for example pointer passed to a parameter
+  // that is marked with nonnull attribute.)
+  bool IsDirectDereference;
 };
 
 /// \brief A helper class which wraps a boolean value set to false by default.
 ///
 /// This class should behave exactly like 'bool' except that it doesn't need to
 /// be explicitly initialized.
 struct DefaultBool {
   bool val;

cfe/trunk/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

if (!notNullState) {
       reportBug(nullState, S, C);
       return;
     }
 
     // Otherwise, we have the case where the location could either be
     // null or not-null.  Record the error node as an "implicit" null
     // dereference.
     if (ExplodedNode *N = C.generateSink(nullState)) {
-      ImplicitNullDerefEvent event = { l, isLoad, N, &C.getBugReporter() };
+      ImplicitNullDerefEvent event = {l, isLoad, N, &C.getBugReporter(),
+                                      /*IsDirectDereference=*/false};
       dispatchEvent(event);
     }
   }
 
   // From this point forward, we know that the location is not null.
   C.addTransition(notNullState);
 }
 
if (StNull) {
     if (!StNonNull) {
       reportBug(StNull, S, C, /*isBind=*/true);
       return;
     }
 
     // At this point the value could be either null or non-null.
     // Record this as an "implicit" null dereference.
     if (ExplodedNode *N = C.generateSink(StNull)) {
-      ImplicitNullDerefEvent event = { V, /*isLoad=*/true, N,
-                                       &C.getBugReporter() };
+      ImplicitNullDerefEvent event = {V, /*isLoad=*/true, N,
+                                      &C.getBugReporter(),
+                                      /*IsDirectDereference=*/false};
       dispatchEvent(event);
     }
   }
 
   // Unlike a regular null dereference, initializing a reference with a
   // dereferenced null pointer does not actually cause a runtime exception in
   // Clang's implementation of references.
   //

cfe/trunk/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp

 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 class NonNullParamChecker
-  : public Checker< check::PreCall > {
+  : public Checker< check::PreCall, EventDispatcher<ImplicitNullDerefEvent> > {
   mutable std::unique_ptr<BugType> BTAttrNonNull;
   mutable std::unique_ptr<BugType> BTNullRefArg;
 
 public:
 
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
 
   std::unique_ptr<BugReport>
if (haveAttrNonNull && !DV->getAs<Loc>()) {
         continue;
       }
     }
 
     ConstraintManager &CM = C.getConstraintManager();
     ProgramStateRef stateNotNull, stateNull;
     std::tie(stateNotNull, stateNull) = CM.assumeDual(state, *DV);
 
-    if (stateNull && !stateNotNull) {
+    if (stateNull) {
+      if (!stateNotNull) {
         // Generate an error node.  Check for a null node in case
         // we cache out.
         if (ExplodedNode *errorNode = C.generateSink(stateNull)) {
 
           std::unique_ptr<BugReport> R;
           if (haveAttrNonNull)
             R = genReportNullAttrNonNull(errorNode, ArgE);
           else if (haveRefTypeParam)
             R = genReportReferenceToNullPointer(errorNode, ArgE);
 
           // Highlight the range of the argument that was null.
           R->addRange(Call.getArgSourceRange(idx));
 
           // Emit the bug report.
           C.emitReport(std::move(R));
         }
 
         // Always return.  Either we cached out or we just emitted an error.
         return;
       }
+      if (ExplodedNode *N = C.generateSink(stateNull)) {
+        ImplicitNullDerefEvent event = {
+            V, false, N, &C.getBugReporter(),
+            /*IsDirectDereference=*/haveRefTypeParam};
+        dispatchEvent(event);
+      }
+    }
 
     // If a pointer value passed the check we should assume that it is
     // indeed not null from this point forward.
     assert(stateNotNull);
     state = stateNotNull;
   }
 
   // If we reach here all of the arguments passed the nonnull check.

cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

const NullabilityState *TrackedNullability =
       State->get<NullabilityMap>(Region);
 
   if (!TrackedNullability)
     return;
 
   if (Filter.CheckNullableDereferenced &&
       TrackedNullability->getValue() == Nullability::Nullable) {
     BugReporter &BR = *Event.BR;
+    if (Event.IsDirectDereference)
       reportBug(ErrorKind::NullableDereferenced, Event.SinkNode, Region, BR);
+    else
+      reportBug(ErrorKind::NullablePassedToNonnull, Event.SinkNode, Region, BR);
   }
 }
 
 /// This method check when nullable pointer or null value is returned from a
 /// function that has nonnull return type.
 ///
 /// TODO: when nullability preconditons are violated, it is ok to violate the
 /// nullability postconditons (i.e.: when one of the nonnull parameters are null

cfe/trunk/test/Analysis/nullability.mm

 
 Dummy *_Nullable returnsNullable();
 Dummy *_Nonnull returnsNonnull();
 Dummy *returnsUnspecified();
 int *_Nullable returnsNullableInt();
 
 template <typename T> T *eraseNullab(T *p) { return p; }
 
+void takesAttrNonnull(Dummy *p) __attribute((nonnull(1)));
+
 void testBasicRules() {
   Dummy *p = returnsNullable();
   int *ptr = returnsNullableInt();
   // Make every dereference a different path to avoid sinks after errors.
   switch (getRandom()) {
   case 0: {
     Dummy &r = *p; // expected-warning {{}}
   } break;
   case 1: {
     int b = p->val; // expected-warning {{}}
   } break;
   case 2: {
     int stuff = *ptr; // expected-warning {{}}
   } break;
   case 3:
     takesNonnull(p); // expected-warning {{}}
     break;
   case 4: {
     Dummy d;
     takesNullable(&d);
     Dummy dd(d);
     break;
   }
-  // Here the copy constructor is called, so a reference is initialized with the
-  // value of p. No ImplicitNullDereference event will be dispatched for this
-  // case. A followup patch is expected to fix this in NonNullParamChecker.
-  default: { Dummy d = *p; } break; // No warning.
+  case 5: takesAttrNonnull(p); break; // expected-warning {{Nullable pointer is passed to}}
+  default: { Dummy d = *p; } break; // expected-warning {{Nullable pointer is dereferenced}}
   }
   if (p) {
     takesNonnull(p);
     if (getRandom()) {
       Dummy &r = *p;
     } else {
       int b = p->val;
     }