[analyzer] Fix some checker's output plist not containing the checker name #2

[xazax.hun]

Summary

Unfortunately, currently, the analyzer core sets the checker name after the constructor was already run. So if we set the BugType in the constructor, the output plist will not contain a checker name. Right now the idiomatic solution is to create the BugType lazily. This patch moves the lazy initialization from the checker side to the analyzer core. I also found some bugs, when some checkers were triggered even if they were not enabled or wrong checker name is emitted to the plist.

In the future probably it would be better to alter the signature of the checkers' constructor to set the name in the constructor so it is possible to create the BugType eagerly.

This patch is an alternative approach to https://reviews.llvm.org/D37437.

[a.sidorin]

Hello Gabor,

For me, this patch looks much better than previous. I have some questions inline.

include/clang/StaticAnalyzer/Core/BugReporter/BugType.h

49

Hm, do we really want to use elaborated name here: class CheckName?

59

Looks like a misspell. Should it be "The check names are set after the constructors are run"?

63

Possibly I am missing the context, but could you please explain, why do we modify CheckName in getName() but not in getCheckName()? It seems a bit unclear to me.

lib/StaticAnalyzer/Checkers/ValistChecker.cpp

277

If I understand correctly, if we report uninitialized and then unterminated, the second report will have wrong CheckName because it is never reset.

test/Analysis/malloc.c

1723

Should we enable the checker instead of removing test?

[xazax.hun]

• Address review comments.

include/clang/StaticAnalyzer/Core/BugReporter/BugType.h

63

Good catch, it is just an oversight. I got confused by CheckName vs Name. And since the analyzer called getName before getCheckName the tests passed.

lib/StaticAnalyzer/Checkers/ValistChecker.cpp

277

That is right. Unfortunately, If unterminated check is not turned on but uninitialized is, we can end up with empty check names. I replaced this workaround with a slightly more robust one.

test/Analysis/malloc.c

1723

I tried that once, but this caused more new warnings (and sinks) so more changes would be required to make the tests pass. And this case is already tested in string.c.

[a.sidorin]

lib/StaticAnalyzer/Checkers/ValistChecker.cpp

277

Maybe we should use different BugTypes for Uninitialized and Unterminated?

test/Analysis/malloc.c

1723

Thank you, I got it.

[NoQ]

I'm still not quite sure what's the whole point of having BugType without a checker. We can still easily write anything we want in the "category" and "name" fields anyways, so we can easily produce bugs that are indistinguishable to the user from different checkers, while being able to distinguish them when we, as developers, want to figure out what checker throws them, so what was the whole point this whole time? I might be missing something, but if you have time, maybe remove the whole CheckName-based constructor and just tie every BugType to the checker? The current approach is totally fine though :)

include/clang/StaticAnalyzer/Core/BugReporter/BugType.h

51–61

I suggest:

if (Checker)
  return Checker->getCheckname().getName();
return Check.getName();

Like, what's the point of storing the StringRef if we can retrieve it any time anyway?

I guess Checker does live long enough?

lib/StaticAnalyzer/Checkers/ValistChecker.cpp

277

Yep, this rings my bells too. We shouldn't race on how do we initialize a BugType depending on what bug is encountered first in the translation unit.

[xazax.hun]

• Address some review comments.

include/clang/StaticAnalyzer/Core/BugReporter/BugType.h

51–61

I slightly modified your snippet to preserve the assertion that can be useful to not to reintroduce this bug in the future.

lib/StaticAnalyzer/Checkers/ValistChecker.cpp

277

Maybe I am missing something but right now there is no race on how we initialize the BugType. The result of the initialization can depend on, however, the set of checks that are registered. I agree that this is unfortunate but introducing a new BugType does not solve this problem or at least it is not apparent for me how would it solve this. One option would be to simply not report these issues unless the corresponding check is registered. The other option would be to make this depend on a new check name that needs to be enabled.

[NoQ]

include/clang/StaticAnalyzer/Core/BugReporter/BugType.h

51–61

Yep, totally!

lib/StaticAnalyzer/Checkers/ValistChecker.cpp

277

Aha, that's right, yep, so this BugType's checker name would be different depending on which checks are enabled, not on bug first encountered, sorry. So it'd only have any effect if someone is grepping through bug types and turns these checkers on and off individually at the same time. Still worth a FIXME i guess? Is it hard to make a third bugtype with a hardcoded name that doesn't rely on the checker?

[sylvestre.ledru]

It missed the 6.0 branching. Will you try to get it on this branch?
Thanks

[xazax.hun]

In D41538#969205, @sylvestre.ledru wrote:

It missed the 6.0 branching. Will you try to get it on this branch?
Thanks

Sure! I will try to do so: https://reviews.llvm.org/rC321933

[hans]

In D41538#969207, @xazax.hun wrote:

In D41538#969205, @sylvestre.ledru wrote:

It missed the 6.0 branching. Will you try to get it on this branch?
Thanks

Sure! I will try to do so: https://reviews.llvm.org/rC321933

Merged in r322550. Thanks, Hans.

include/clang/StaticAnalyzer/Core/BugReporter/BugType.h

 class ExplodedNode;
 class ExprEngine;
 
 class BugType {
 private:
   const CheckName Check;
   const std::string Name;
   const std::string Category;
-  bool SuppressonSink;
+  const CheckerBase *Checker;
+  bool SuppressOnSink;
 
   virtual void anchor();
+
 public:
-  BugType(class CheckName check, StringRef name, StringRef cat)
-      : Check(check), Name(name), Category(cat), SuppressonSink(false) {}
-  BugType(const CheckerBase *checker, StringRef name, StringRef cat)
-      : Check(checker->getCheckName()), Name(name), Category(cat),
-        SuppressonSink(false) {}
-  virtual ~BugType() {}
+  BugType(CheckName Check, StringRef Name, StringRef Cat)
+      : Check(Check), Name(Name), Category(Cat), Checker(nullptr),
+        SuppressOnSink(false) {}
+  BugType(const CheckerBase *Checker, StringRef Name, StringRef Cat)
+      : Check(Checker->getCheckName()), Name(Name), Category(Cat),
+        Checker(Checker), SuppressOnSink(false) {}
+  virtual ~BugType() = default;
 
-  // FIXME: Should these be made strings as well?
   StringRef getName() const { return Name; }
   StringRef getCategory() const { return Category; }
-  StringRef getCheckName() const { return Check.getName(); }
+  StringRef getCheckName() const {
+    // FIXME: This is a workaround to ensure that the correct check name is used
+    // The check names are set after the constructors are run.
+    // In case the BugType object is initialized in the checker's ctor
+    // the Check field will be empty. To circumvent this problem we use 
+    // CheckerBase whenever it is possible.
+    StringRef CheckName =
+        Checker ? Checker->getCheckName().getName() : Check.getName();
+    assert(!CheckName.empty() && "Check name is not set properly.");
+    return CheckName;
+  }
 
   /// isSuppressOnSink - Returns true if bug reports associated with this bug
   ///  type should be suppressed if the end node of the report is post-dominated
   ///  by a sink node.
-  bool isSuppressOnSink() const { return SuppressonSink; }
-  void setSuppressOnSink(bool x) { SuppressonSink = x; }
+  bool isSuppressOnSink() const { return SuppressOnSink; }
+  void setSuppressOnSink(bool x) { SuppressOnSink = x; }
 
   virtual void FlushReports(BugReporter& BR);
 };
 
 class BuiltinBug : public BugType {
   const std::string desc;
   void anchor() override;
 public:
   BuiltinBug(class CheckName check, const char *name, const char *description)
       : BugType(check, name, categories::LogicError), desc(description) {}
 
   BuiltinBug(const CheckerBase *checker, const char *name,
              const char *description)
       : BugType(checker, name, categories::LogicError), desc(description) {}
 
   BuiltinBug(const CheckerBase *checker, const char *name)
       : BugType(checker, name, categories::LogicError), desc(name) {}
 
   StringRef getDescription() const { return desc; }
 };
 
-} // end GR namespace
+} // end ento namespace
 
 } // end clang namespace
 #endif

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

void ento::registerNewDeleteLeaksChecker(CheckerManager &mgr) {
   MallocChecker *checker = mgr.registerChecker<MallocChecker>();
   checker->IsOptimistic = mgr.getAnalyzerOptions().getBooleanOption(
       "Optimistic", false, checker);
   checker->ChecksEnabled[MallocChecker::CK_NewDeleteLeaksChecker] = true;
   checker->CheckNames[MallocChecker::CK_NewDeleteLeaksChecker] =
       mgr.getCurrentCheckName();
   // We currently treat NewDeleteLeaks checker as a subchecker of NewDelete
   // checker.
-  if (!checker->ChecksEnabled[MallocChecker::CK_NewDeleteChecker])
+  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();
+  }
 }
 
 #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)

lib/StaticAnalyzer/Checkers/ValistChecker.cpp

private:
   StringRef getVariableNameFromRegion(const MemRegion *Reg) const;
   const ExplodedNode *getStartCallSite(const ExplodedNode *N,
                                        const MemRegion *Reg) const;
 
   void reportUninitializedAccess(const MemRegion *VAList, StringRef Msg,
                                  CheckerContext &C) const;
   void reportLeakedVALists(const RegionVector &LeakedVALists, StringRef Msg1,
                            StringRef Msg2, CheckerContext &C, ExplodedNode *N,
-                           bool ForceReport = false) const;
+                           bool ReportUninit = false) const;
 
   void checkVAListStartCall(const CallEvent &Call, CheckerContext &C,
                             bool IsCopy) const;
   void checkVAListEndCall(const CallEvent &Call, CheckerContext &C) const;
 
   class ValistBugVisitor : public BugReporterVisitorImpl<ValistBugVisitor> {
   public:
     ValistBugVisitor(const MemRegion *Reg, bool IsLeak = false)
if (ExplodedNode *N = C.generateErrorNode()) {
     R->addVisitor(llvm::make_unique<ValistBugVisitor>(VAList));
     C.emitReport(std::move(R));
   }
 }
 
 void ValistChecker::reportLeakedVALists(const RegionVector &LeakedVALists,
                                         StringRef Msg1, StringRef Msg2,
                                         CheckerContext &C, ExplodedNode *N,
-                                        bool ForceReport) const {
+                                        bool ReportUninit) const {
   if (!(ChecksEnabled[CK_Unterminated] ||
-        (ChecksEnabled[CK_Uninitialized] && ForceReport)))
+        (ChecksEnabled[CK_Uninitialized] && ReportUninit)))
     return;
   for (auto Reg : LeakedVALists) {
     if (!BT_leakedvalist) {
-      BT_leakedvalist.reset(new BugType(CheckNames[CK_Unterminated],
-                                        "Leaked va_list",
-                                        categories::MemoryError));
+      // FIXME: maybe creating a new check name for this type of bug is a better
+      // solution.
+      BT_leakedvalist.reset(
+          new BugType(CheckNames[CK_Unterminated].getName().empty()
+                          ? CheckNames[CK_Uninitialized]
+                          : CheckNames[CK_Unterminated],
+                      "Leaked va_list", categories::MemoryError));
       BT_leakedvalist->setSuppressOnSink(true);
     }
 
     const ExplodedNode *StartNode = getStartCallSite(N, Reg);
     PathDiagnosticLocation LocUsedForUniqueing;
 
     if (const Stmt *StartCallStmt = PathDiagnosticLocation::getStmt(StartNode))
       LocUsedForUniqueing = PathDiagnosticLocation::createBegin(
void ValistChecker::checkVAListEndCall(const CallEvent &Call,
   }
   ProgramStateRef State = C.getState();
   State = State->remove<InitializedVALists>(VAList);
   C.addTransition(State);
 }
 
 std::shared_ptr<PathDiagnosticPiece> ValistChecker::ValistBugVisitor::VisitNode(
     const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC,
-    BugReport &BR) {
+    BugReport &) {
   ProgramStateRef State = N->getState();
   ProgramStateRef StatePrev = PrevN->getState();
 
   const Stmt *S = PathDiagnosticLocation::getStmt(N);
   if (!S)
     return nullptr;
 
   StringRef Msg;

test/Analysis/malloc.c

void *smallocWarn(size_t size) {
   if (size == 2) {
     return malloc(1);
   }
   else {
     return malloc(size);
   }
 }
 
-char *dupstrWarn(const char *s) {
-  const int len = strlen(s);
-  char *p = (char*) smallocWarn(len + 1);
-  strcpy(p, s); // expected-warning{{String copy function overflows destination buffer}}
-  return p;
-}
-
 int *radar15580979() {
   int *data = (int *)malloc(32);
   int *p = data ?: (int*)malloc(32); // no warning
   return p;
 }
 
 // Some data structures may hold onto the pointer and free it later.
 void testEscapeThroughSystemCallTakingVoidPointer1(void *queue) {