[Analyzer] Improve invalid dereference bug reporting in DereferenceChecker.

[balazske]

Summary

Report undefined pointer dereference in similar way as null pointer dereference.

[balazske]

Not sure if this is a good solution. There are few tests that test the undefined reference case. But the added code should work the same way as in the null pointer case?

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

135

Add "results in a " here?

[balazske]

Change of the existing error messages and bug category is not necessary but it would be improvement because more uniform error messages. (And not the "logic error" is the best category for this case, if this value is used at all. Other checkers have bad values too.)

[gamesh411]

I would add one more test for the undefined case. Like a local array variable that is uninitialized. That could mirror some of the null-dereference cases.

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

135

IMHO I would add the "results in a " part here, as this is part of the process of creating the diagnostic anyway.

[Szelethus]

We definitely need more tests. The idea overall however is amazing, thanks!

clang/test/Analysis/misc-ps-region-store.m

1160

Is this the entire message? Because traditionally, analyzer warnings start with an upper case and don't terminate.

[balazske]

NFC changes, added some tests.

[NoQ]

Thanks, excellent!

Let's not change warning text without good reasons. Too few of us speak English well enough to check the articles. The original text seems concise and on-point.

[balazske]

Preserve original error messages.

[balazske]

Do the null pointer and invalid pointer dereference belong to the same checker, that is called NullDereference?

clang/test/Analysis/misc-ps-region-store.m

1160

Full message is:
Access to field 'tail' results in a dereference of an undefined pointer value (loaded from variable 'items') [core.NullDereference]

[whisperity]

In D84520#2206077, @balazske wrote:

Do the null pointer and invalid pointer dereference belong to the same checker, that is called NullDereference?

I think both SA and Tidy auto-appends the checker's name into the emitted diagnostic. However, @baloghadamsoftware did implement multiple checkers in the same translation unit, so maybe he knows where to override this.

(I can tell you where to override it in the context of Tidy, and it's horribly tricky and ugly...)

[NoQ]

In D84520#2206077, @balazske wrote:

Do the null pointer and invalid pointer dereference belong to the same checker, that is called NullDereference?

Yup. And that's bad.

Note that the only reason to have checker names is to allow users to enable/disable the checkers. Given that enabling/disabling core checkers was never supported to begin with, this wasn't much of an issue. Now that we're moving into the direction of allowing users to silence core checkers without disabling their modeling benefits, this becomes much more of a problem and there's a number of checker name inconsistencies that we'll have to revisit. Another famous inconsistency is having a popular case of null dereference, "calling a C++ method on a null pointer", is in fact checked by the core.CallAndMessage checker rather than by core.NullDereference.

[NoQ]

Also thanks, let's land!

[balazske]

Rebase (bug category is LogicError).

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

 using namespace clang;
 using namespace ento;
 
 namespace {
 class DereferenceChecker
     : public Checker< check::Location,
                       check::Bind,
                       EventDispatcher<ImplicitNullDerefEvent> > {
+  enum DerefKind { NullPointer, UndefinedPointerValue };
+
   BugType BT_Null{this, "Dereference of null pointer", categories::LogicError};
   BugType BT_Undef{this, "Dereference of undefined pointer value",
                    categories::LogicError};
 
-  void reportBug(ProgramStateRef State, const Stmt *S, CheckerContext &C) const;
+  void reportBug(DerefKind K, ProgramStateRef State, const Stmt *S,
+                 CheckerContext &C) const;
 
 public:
   void checkLocation(SVal location, bool isLoad, const Stmt* S,
                      CheckerContext &C) const;
   void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const;
 
   static void AddDerefSource(raw_ostream &os,
                              SmallVectorImpl<SourceRange> &Ranges,

 }
 
 static bool isDeclRefExprToReference(const Expr *E) {
   if (const auto *DRE = dyn_cast<DeclRefExpr>(E))
     return DRE->getDecl()->getType()->isReferenceType();
   return false;
 }
 
-void DereferenceChecker::reportBug(ProgramStateRef State, const Stmt *S,
-                                   CheckerContext &C) const {
+void DereferenceChecker::reportBug(DerefKind K, ProgramStateRef State,
+                                   const Stmt *S, CheckerContext &C) const {
+  const BugType *BT = nullptr;
+  llvm::StringRef DerefStr1;
+  llvm::StringRef DerefStr2;
+  switch (K) {
+  case DerefKind::NullPointer:
+    BT = &BT_Null;
+    DerefStr1 = " results in a null pointer dereference";
+    DerefStr2 = " results in a dereference of a null pointer";
+    break;
+  case DerefKind::UndefinedPointerValue:
+    BT = &BT_Undef;
+    DerefStr1 = " results in an undefined pointer dereference";
+    DerefStr2 = " results in a dereference of an undefined pointer value";
+    break;
+  };
+
   // Generate an error node.
   ExplodedNode *N = C.generateErrorNode(State);
   if (!N)
     return;
 
   SmallString<100> buf;
   llvm::raw_svector_ostream os(buf);
 
   SmallVector<SourceRange, 2> Ranges;
 
   switch (S->getStmtClass()) {
   case Stmt::ArraySubscriptExprClass: {
     os << "Array access";
     const ArraySubscriptExpr *AE = cast<ArraySubscriptExpr>(S);
     AddDerefSource(os, Ranges, AE->getBase()->IgnoreParenCasts(),
                    State.get(), N->getLocationContext());
-    os << " results in a null pointer dereference";
+    os << DerefStr1;
     break;
   }
   case Stmt::OMPArraySectionExprClass: {
     os << "Array access";
     const OMPArraySectionExpr *AE = cast<OMPArraySectionExpr>(S);
     AddDerefSource(os, Ranges, AE->getBase()->IgnoreParenCasts(),
                    State.get(), N->getLocationContext());
-    os << " results in a null pointer dereference";
+    os << DerefStr1;
     break;
   }
   case Stmt::UnaryOperatorClass: {
-    os << "Dereference of null pointer";
+    os << BT->getDescription();
     const UnaryOperator *U = cast<UnaryOperator>(S);
     AddDerefSource(os, Ranges, U->getSubExpr()->IgnoreParens(),
                    State.get(), N->getLocationContext(), true);
     break;
   }
   case Stmt::MemberExprClass: {
     const MemberExpr *M = cast<MemberExpr>(S);
     if (M->isArrow() || isDeclRefExprToReference(M->getBase())) {
-      os << "Access to field '" << M->getMemberNameInfo()
-         << "' results in a dereference of a null pointer";
+      os << "Access to field '" << M->getMemberNameInfo() << "'" << DerefStr2;
       AddDerefSource(os, Ranges, M->getBase()->IgnoreParenCasts(),
                      State.get(), N->getLocationContext(), true);
     }
     break;
   }
   case Stmt::ObjCIvarRefExprClass: {
     const ObjCIvarRefExpr *IV = cast<ObjCIvarRefExpr>(S);
-    os << "Access to instance variable '" << *IV->getDecl()
-       << "' results in a dereference of a null pointer";
+    os << "Access to instance variable '" << *IV->getDecl() << "'" << DerefStr2;
     AddDerefSource(os, Ranges, IV->getBase()->IgnoreParenCasts(),
                    State.get(), N->getLocationContext(), true);
     break;
   }
   default:
     break;
   }
 
   auto report = std::make_unique<PathSensitiveBugReport>(
-      BT_Null, buf.empty() ? BT_Null.getDescription() : StringRef(buf), N);
+      *BT, buf.empty() ? BT->getDescription() : StringRef(buf), N);
 
   bugreporter::trackExpressionValue(N, bugreporter::getDerefExpr(S), *report);
 
   for (SmallVectorImpl<SourceRange>::iterator
        I = Ranges.begin(), E = Ranges.end(); I!=E; ++I)
     report->addRange(*I);
 
   C.emitReport(std::move(report));
 }
 
 void DereferenceChecker::checkLocation(SVal l, bool isLoad, const Stmt* S,
                                        CheckerContext &C) const {
   // Check for dereference of an undefined value.
   if (l.isUndef()) {
-    if (ExplodedNode *N = C.generateErrorNode()) {
-      auto report = std::make_unique<PathSensitiveBugReport>(
-          BT_Undef, BT_Undef.getDescription(), N);
-      bugreporter::trackExpressionValue(N, bugreporter::getDerefExpr(S), *report);
-      C.emitReport(std::move(report));
-    }
+    const Expr *DerefExpr = getDereferenceExpr(S);
+    if (!suppressReport(DerefExpr))
+      reportBug(DerefKind::UndefinedPointerValue, C.getState(), DerefExpr, C);
     return;
   }
 
   DefinedOrUnknownSVal location = l.castAs<DefinedOrUnknownSVal>();
 
   // Check for null dereferences.
   if (!location.getAs<Loc>())
     return;
 
   ProgramStateRef state = C.getState();
 
   ProgramStateRef notNullState, nullState;
   std::tie(notNullState, nullState) = state->assume(location);
 
   if (nullState) {
     if (!notNullState) {
       // We know that 'location' can only be null.  This is what
       // we call an "explicit" null dereference.
       const Expr *expr = getDereferenceExpr(S);
       if (!suppressReport(expr)) {
-        reportBug(nullState, expr, C);
+        reportBug(DerefKind::NullPointer, nullState, expr, 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, C.getPredecessor())) {
void DereferenceChecker::checkBind(SVal L, SVal V, const Stmt *S,
 
   ProgramStateRef StNonNull, StNull;
   std::tie(StNonNull, StNull) = State->assume(V.castAs<DefinedOrUnknownSVal>());
 
   if (StNull) {
     if (!StNonNull) {
       const Expr *expr = getDereferenceExpr(S, /*IsBind=*/true);
       if (!suppressReport(expr)) {
-        reportBug(StNull, expr, C);
+        reportBug(DerefKind::NullPointer, StNull, expr, C);
         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, C.getPredecessor())) {
       ImplicitNullDerefEvent event = {V, /*isLoad=*/true, N,

clang/test/Analysis/invalid-deref.c

+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+typedef unsigned uintptr_t;
+
+void f1() {
+  int *p;
+  *p = 0; // expected-warning{{Dereference of undefined pointer value}}
+}
+
+struct foo_struct {
+  int x;
+};
+
+int f2() {
+  struct foo_struct *p;
+
+  return p->x++; // expected-warning{{Access to field 'x' results in a dereference of an undefined pointer value (loaded from variable 'p')}}
+}
+
+int f3() {
+  char *x;
+  int i = 2;
+
+  return x[i + 1]; // expected-warning{{Array access (from variable 'x') results in an undefined pointer dereference}}
+}
+
+int f3_b() {
+  char *x;
+  int i = 2;
+
+  return x[i + 1]++; // expected-warning{{Array access (from variable 'x') results in an undefined pointer dereference}}
+}

clang/test/Analysis/misc-ps-region-store.m

 struct list_pr8141
 {
   struct list_pr8141 *tail;
 };
 
 struct list_pr8141 *
 pr8141 (void) {
   struct list_pr8141 *items;
-  for (;; items = ({ do { } while (0); items->tail; })) // expected-warning{{Dereference of undefined pointer value}}
+  for (;; items = ({ do { } while (0); items->tail; })) // expected-warning{{dereference of an undefined pointer value}}
     {
     }
 }
 
 // Don't crash when building the CFG.
 void do_not_crash(int x) {
   while (x - ({do {} while (0); x; })) {
   }