[analyzer] Fix displayed checker name for InnerPointerChecker

[rnkovacs]

Summary

For InnerPointerChecker to function properly, both the checker itself and MallocChecker's capabilities that handle relevant use-after-free problems need to be turned on. So far, the latter part has been developed under the name of MallocChecker's NewDelete sub-checker, often causing warnings to appear under that name. This patch defines a new CheckKind within MallocChecker for the inner pointer checking functionality, so that the correct name is displayed on warnings.

Tested on clang-tidy.

[NoQ]

I see, so that's how it's done!

I also noticed that checker name was weird in exploded graph dumps, i.e. it was showing regular new/delete stuff as if it was done by InnerPointer checker. I'll check if this is fixed tomorrow.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

1689

It's usually just None, but i guess whoever touched this code before didn't know; it's pretty hard to discover.

[rnkovacs]

In D50211#1186630, @NoQ wrote:

I see, so that's how it's done!

I also noticed that checker name was weird in exploded graph dumps, i.e. it was showing regular new/delete stuff as if it was done by InnerPointer checker. I'll check if this is fixed tomorrow.

This seemed interesting, so I tried out myself on the NewDelete-path-notes.cpp test file. I saw the following (using IP and ND abbreviations for InnerPointer and NewDelete respectively):

Before the patch,

• If only IP was turned on, I got ND's warnings with IP's name everywhere in the exploded graph.
• If only ND was turned on, I got the warnings under ND's tag.
• If both IP and ND were turned on, I got the warnings, and saw ND's name beside any allocation state change in the graph, but the bugs seemed to be tagged with IP's name in the end.

After this patch,

• If only IP is turned on, I get no warning but I see IP's tag in the graph on the last PurgeDeadSymbols node (I guess it's normal?).
• If only ND is turned on, I get the warnings with ND's tag everywhere.
• If both IP and ND are turned on, I get the warnings, and I see no trace of IP, only ND tags everywhere.

It was really messed up before, but I think I see an improvement there.

[NoQ]

Yay thx! Yeah, that's the best behavior i can imagine here.

[rnkovacs]

Replace empty Optionals with Nones.

[rnkovacs]

Committed in r339067, I just messed up the revision-closing line in the commit message.

[NoQ]

Welcome to the club!

[rnkovacs]

In D50211#1190146, @NoQ wrote:

Welcome to the club!

:D
Thanks, makes me feel better.

[Szelethus]

Bad news, this approach doesn't work too well, and here's why: https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Core/CheckerRegistry.cpp#L115

void CheckerRegistry::initializeManager(CheckerManager &checkerMgr,
                                  SmallVectorImpl<CheckerOptInfo> &opts) const {
  // Sort checkers for efficient collection.
  llvm::sort(Checkers, checkerNameLT);

  // Collect checkers enabled by the options.
  CheckerInfoSet enabledCheckers;
  for (auto &i : opts)
    collectCheckers(Checkers, Packages, i, enabledCheckers);

  // Initialize the CheckerManager with all enabled checkers.
  for (const auto *i :enabledCheckers) {
    checkerMgr.setCurrentCheckName(CheckName(i->FullName));
    i->Initialize(checkerMgr);
  }
}

Note that Initialize is a function pointer that points to register##CHECKERNAME, so a single registry function should only register one checker, because setCurrentCheckName is only called once, resulting in MallocChecker's checker object receiving the cpluscplus.InnetPointer name. It's very not-obvious how to fix this gracefully :/ You could call CheckerManager::setCurrentCheckName within the registry function, but you have to supply a full name, which adds unnecessary maintenance cost (for example, if someone moves a checker out of alpha, or puts one back).

I found this bug while trying to fix checker options, and noticed that the Optimistic flag of MallocChecker is acquired as cplusplus.InnerPointer:Optimistic, instead of unix.Malloc:Optimistic. Let's keep this in for now, and I'll try to look for ways to express dependencies while avoiding this issue.

[Szelethus]

I personally find the registration process very error-prone, the current infrastructure around it makes it super easy to mess up in a way that won't be detected for months, so we probably need a better approach altogether. It shouldn't be possible to do any harm within a registry function, especially not in a way that affects other checkers.

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

 // This file defines a check that marks a raw pointer to a C++ container's
 // inner buffer released when the object is destroyed. This information can
 // be used by MallocChecker to detect use-after-free problems.
 //
 //===----------------------------------------------------------------------===//
 
 #include "AllocationState.h"
 #include "ClangSACheckers.h"
+#include "InterCheckerAPI.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 
 using namespace clang;
 using namespace ento;
std::unique_ptr<BugReporterVisitor> getInnerPointerBRVisitor(SymbolRef Sym) {
   return llvm::make_unique<InnerPointerChecker::InnerPointerBRVisitor>(Sym);
 }
 
 } // end namespace allocation_state
 } // end namespace ento
 } // end namespace clang
 
 void ento::registerInnerPointerChecker(CheckerManager &Mgr) {
-  registerNewDeleteChecker(Mgr);
+  registerInnerPointerCheckerAux(Mgr);
   Mgr.registerChecker<InnerPointerChecker>();
 }

lib/StaticAnalyzer/Checkers/InterCheckerAPI.h

 namespace clang {
 class CheckerManager;
 
 namespace ento {
 
 /// Register the checker which evaluates CString API calls.
 void registerCStringCheckerBasic(CheckerManager &Mgr);
 
+/// Register the part of MallocChecker connected to InnerPointerChecker.
+void registerInnerPointerCheckerAux(CheckerManager &Mgr);
+
 }}
 #endif /* INTERCHECKERAPI_H_ */

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

public:
 
   /// In pessimistic mode, the checker assumes that it does not know which
   /// functions might free the memory.
   enum CheckKind {
     CK_MallocChecker,
     CK_NewDeleteChecker,
     CK_NewDeleteLeaksChecker,
     CK_MismatchedDeallocatorChecker,
+    CK_InnerPointerChecker,
     CK_NumCheckKinds
   };
 
   enum class MemoryOperationKind {
     MOK_Allocate,
     MOK_Free,
     MOK_Any
   };

 MallocChecker::getCheckIfTracked(AllocationFamily Family,
                                  bool IsALeakCheck) const {
   switch (Family) {
   case AF_Malloc:
   case AF_Alloca:
   case AF_IfNameIndex: {
     if (ChecksEnabled[CK_MallocChecker])
       return CK_MallocChecker;
-
-    return Optional<MallocChecker::CheckKind>();
+    return None;
   }
   case AF_CXXNew:
-  case AF_CXXNewArray:
-  // FIXME: Add new CheckKind for AF_InnerBuffer.
-  case AF_InnerBuffer: {
+  case AF_CXXNewArray: {
     if (IsALeakCheck) {
       if (ChecksEnabled[CK_NewDeleteLeaksChecker])
         return CK_NewDeleteLeaksChecker;
     }
     else {
       if (ChecksEnabled[CK_NewDeleteChecker])
         return CK_NewDeleteChecker;
     }
-    return Optional<MallocChecker::CheckKind>();
+    return None;
+  }
+  case AF_InnerBuffer: {
+    if (ChecksEnabled[CK_InnerPointerChecker])
+      return CK_InnerPointerChecker;
+    return None;
   }
   case AF_None: {
     llvm_unreachable("no family");
   }
   }
   llvm_unreachable("unhandled family");
 }
 
 Optional<MallocChecker::CheckKind>
 MallocChecker::getCheckIfTracked(CheckerContext &C,
                                  const Stmt *AllocDeallocStmt,
                                  bool IsALeakCheck) const {
   return getCheckIfTracked(getAllocationFamily(C, AllocDeallocStmt),
                            IsALeakCheck);
void MallocChecker::ReportOffsetFree(CheckerContext &C, SVal ArgVal,
   R->addRange(Range);
   C.emitReport(std::move(R));
 }
 
 void MallocChecker::ReportUseAfterFree(CheckerContext &C, SourceRange Range,
                                        SymbolRef Sym) const {
 
   if (!ChecksEnabled[CK_MallocChecker] &&
-      !ChecksEnabled[CK_NewDeleteChecker])
+      !ChecksEnabled[CK_NewDeleteChecker] &&
+      !ChecksEnabled[CK_InnerPointerChecker])
     return;
 
   Optional<MallocChecker::CheckKind> CheckKind = getCheckIfTracked(C, Sym);
   if (!CheckKind.hasValue())
     return;
 
   if (ExplodedNode *N = C.generateErrorNode()) {
     if (!BT_UseFree[*CheckKind])
if (!checker->ChecksEnabled[MallocChecker::CK_NewDeleteChecker]) {
     checker->ChecksEnabled[MallocChecker::CK_NewDeleteChecker] = true;
     // FIXME: This does not set the correct name, but without this workaround
     //        no name will be set at all.
     checker->CheckNames[MallocChecker::CK_NewDeleteChecker] =
         mgr.getCurrentCheckName();
   }
 }
 
+// Intended to be used in InnerPointerChecker to register the part of
+// MallocChecker connected to it.
+void ento::registerInnerPointerCheckerAux(CheckerManager &mgr) {
+    registerCStringCheckerBasic(mgr);
+    MallocChecker *checker = mgr.registerChecker<MallocChecker>();
+    checker->IsOptimistic = mgr.getAnalyzerOptions().getBooleanOption(
+        "Optimistic", false, checker);
+    checker->ChecksEnabled[MallocChecker::CK_InnerPointerChecker] = true;
+    checker->CheckNames[MallocChecker::CK_InnerPointerChecker] =
+        mgr.getCurrentCheckName();
+}
+
 #define REGISTER_CHECKER(name)                                                 \
   void ento::register##name(CheckerManager &mgr) {                             \
     registerCStringCheckerBasic(mgr);                                          \
     MallocChecker *checker = mgr.registerChecker<MallocChecker>();             \
     checker->IsOptimistic = mgr.getAnalyzerOptions().getBooleanOption(         \
         "Optimistic", false, checker);                                         \
     checker->ChecksEnabled[MallocChecker::CK_##name] = true;                   \
     checker->CheckNames[MallocChecker::CK_##name] = mgr.getCurrentCheckName(); \
   }
 
 REGISTER_CHECKER(MallocChecker)
 REGISTER_CHECKER(NewDeleteChecker)
 REGISTER_CHECKER(MismatchedDeallocatorChecker)