[analyzer] CStringChecker.cpp - Code refactoring on bug report.

[MTC]

Summary

When improving the modeling evalMemset(), some scenes need to emit report of NotNullTerm. In this case,
there are three places in CStringChecker.cpp that have the same code about NotNullTerm report. In
addition, CStringChecker.cpp already has emitOverlapBug(), so I think it needs code refactoring.

Thanks in advance for the code review!

[NoQ]

Sorry, one moment, i'm seeing a few regressions after the previous refactoring but i didn't look at them closely yet to provide a reproducer. I'll get back to this.

[MTC]

In D44557#1042357, @NoQ wrote:

Sorry, one moment, i'm seeing a few regressions after the previous refactoring but i didn't look at them closely yet to provide a reproducer. I'll get back to this.

Thank you for the code review, @NoQ !

The regression is maybe caused by https://reviews.llvm.org/D44075, my patch! CheckBufferAccess() may not guarantee checkNonNull() will be called. If that's true, please revert https://reviews.llvm.org/rC326782 and I'll fix it.

[NoQ]

In D44557#1042792, @MTC wrote:

In D44557#1042357, @NoQ wrote:

Sorry, one moment, i'm seeing a few regressions after the previous refactoring but i didn't look at them closely yet to provide a reproducer. I'll get back to this.

Thank you for the code review, @NoQ !

The regression is maybe caused by https://reviews.llvm.org/D44075, my patch! CheckBufferAccess() may not guarantee checkNonNull() will be called. If that's true, please revert https://reviews.llvm.org/rC326782 and I'll fix it.

Thank you; i reverted the change in rC328067, but it'd still be great to at least add a reproducer to our tests :) I'll have a look at this one soon.

[george.karpenkov]

LGTM, thanks!

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

ProgramStateRef CheckOverlap(CheckerContext &C,
                                    const Expr *Size,
                                    const Expr *First,
                                    const Expr *Second) const;
   void emitOverlapBug(CheckerContext &C,
                       ProgramStateRef state,
                       const Stmt *First,
                       const Stmt *Second) const;
 
+  void emitNullArgBug(CheckerContext &C, ProgramStateRef State, const Stmt *S,
+                      StringRef WarningMsg) const;
+  void emitOutOfBoundsBug(CheckerContext &C, ProgramStateRef State,
+                          const Stmt *S, StringRef WarningMsg) const;
+  void emitNotCStringBug(CheckerContext &C, ProgramStateRef State,
+                         const Stmt *S, StringRef WarningMsg) const;
+  void emitAdditionOverflowBug(CheckerContext &C, ProgramStateRef State) const;
+
   ProgramStateRef checkAdditionOverflow(CheckerContext &C,
                                             ProgramStateRef state,
                                             NonLoc left,
                                             NonLoc right) const;
 
   // Return true if the destination buffer of the copy function may be in bound.
   // Expects SVal of Size to be positive and unsigned.
   // Expects SVal of FirstBuf to be a FieldRegion.
ProgramStateRef CStringChecker::checkNonNull(CheckerContext &C,
   // If a previous check has failed, propagate the failure.
   if (!state)
     return nullptr;
 
   ProgramStateRef stateNull, stateNonNull;
   std::tie(stateNull, stateNonNull) = assumeZero(C, state, l, S->getType());
 
   if (stateNull && !stateNonNull) {
-    if (!Filter.CheckCStringNullArg)
-      return nullptr;
-
-    ExplodedNode *N = C.generateErrorNode(stateNull);
-    if (!N)
-      return nullptr;
-
-    if (!BT_Null)
-      BT_Null.reset(new BuiltinBug(
-          Filter.CheckNameCStringNullArg, categories::UnixAPI,
-          "Null pointer argument in call to byte string function"));
-
+    if (Filter.CheckCStringNullArg) {
       SmallString<80> buf;
       llvm::raw_svector_ostream os(buf);
       assert(CurrentFunctionDescription);
       os << "Null pointer argument in call to " << CurrentFunctionDescription;
 
-    // Generate a report for this bug.
-    BuiltinBug *BT = static_cast<BuiltinBug*>(BT_Null.get());
-    auto report = llvm::make_unique<BugReport>(*BT, os.str(), N);
-
-    report->addRange(S->getSourceRange());
-    bugreporter::trackNullOrUndefValue(N, S, *report);
-    C.emitReport(std::move(report));
+      emitNullArgBug(C, stateNull, S, os.str());
+    }
     return nullptr;
   }
 
   // From here on, assume that the value is non-null.
   assert(stateNonNull);
   return stateNonNull;
 }
 
ProgramStateRef CStringChecker::CheckLocation(CheckerContext &C,
   DefinedOrUnknownSVal Size = Extent.castAs<DefinedOrUnknownSVal>();
 
   // Get the index of the accessed element.
   DefinedOrUnknownSVal Idx = ER->getIndex().castAs<DefinedOrUnknownSVal>();
 
   ProgramStateRef StInBound = state->assumeInBound(Idx, Size, true);
   ProgramStateRef StOutBound = state->assumeInBound(Idx, Size, false);
   if (StOutBound && !StInBound) {
-    ExplodedNode *N = C.generateErrorNode(StOutBound);
-    if (!N)
-      return nullptr;
-
-    CheckName Name;
     // These checks are either enabled by the CString out-of-bounds checker
     // explicitly or the "basic" CStringNullArg checker support that Malloc
     // checker enables.
     assert(Filter.CheckCStringOutOfBounds || Filter.CheckCStringNullArg);
-    if (Filter.CheckCStringOutOfBounds)
-      Name = Filter.CheckNameCStringOutOfBounds;
-    else
-      Name = Filter.CheckNameCStringNullArg;
 
-    if (!BT_Bounds) {
-      BT_Bounds.reset(new BuiltinBug(
-          Name, "Out-of-bound array access",
-          "Byte string function accesses out-of-bound array element"));
-    }
-    BuiltinBug *BT = static_cast<BuiltinBug*>(BT_Bounds.get());
-
-    // Generate a report for this bug.
-    std::unique_ptr<BugReport> report;
+    // Emit a bug report.
     if (warningMsg) {
-      report = llvm::make_unique<BugReport>(*BT, warningMsg, N);
+      emitOutOfBoundsBug(C, StOutBound, S, warningMsg);
     } else {
       assert(CurrentFunctionDescription);
       assert(CurrentFunctionDescription[0] != '\0');
 
       SmallString<80> buf;
       llvm::raw_svector_ostream os(buf);
       os << toUppercase(CurrentFunctionDescription[0])
          << &CurrentFunctionDescription[1]
          << " accesses out-of-bound array element";
-      report = llvm::make_unique<BugReport>(*BT, os.str(), N);
+      emitOutOfBoundsBug(C, StOutBound, S, os.str());
     }
-
-    // FIXME: It would be nice to eventually make this diagnostic more clear,
-    // e.g., by referencing the original declaration or by saying *why* this
-    // reference is outside the range.
-
-    report->addRange(S->getSourceRange());
-    C.emitReport(std::move(report));
     return nullptr;
   }
 
   // Array bound check succeeded.  From this point forward the array bound
   // should always succeed.
   return StInBound;
 }
 
void CStringChecker::emitOverlapBug(CheckerContext &C, ProgramStateRef state,
   auto report = llvm::make_unique<BugReport>(
       *BT_Overlap, "Arguments must not be overlapping buffers", N);
   report->addRange(First->getSourceRange());
   report->addRange(Second->getSourceRange());
 
   C.emitReport(std::move(report));
 }
 
+void CStringChecker::emitNullArgBug(CheckerContext &C, ProgramStateRef State,
+                                    const Stmt *S, StringRef WarningMsg) const {
+  if (ExplodedNode *N = C.generateErrorNode(State)) {
+    if (!BT_Null)
+      BT_Null.reset(new BuiltinBug(
+          Filter.CheckNameCStringNullArg, categories::UnixAPI,
+          "Null pointer argument in call to byte string function"));
+
+    BuiltinBug *BT = static_cast<BuiltinBug *>(BT_Null.get());
+    auto Report = llvm::make_unique<BugReport>(*BT, WarningMsg, N);
+    bugreporter::trackNullOrUndefValue(N, S, *Report);
+    C.emitReport(std::move(Report));
+  }
+}
+
+void CStringChecker::emitOutOfBoundsBug(CheckerContext &C,
+                                        ProgramStateRef State, const Stmt *S,
+                                        StringRef WarningMsg) const {
+  if (ExplodedNode *N = C.generateErrorNode(State)) {
+    if (!BT_Bounds)
+      BT_Bounds.reset(new BuiltinBug(
+          Filter.CheckCStringOutOfBounds ? Filter.CheckNameCStringOutOfBounds
+                                         : Filter.CheckNameCStringNullArg,
+          "Out-of-bound array access",
+          "Byte string function accesses out-of-bound array element"));
+
+    BuiltinBug *BT = static_cast<BuiltinBug *>(BT_Bounds.get());
+
+    // FIXME: It would be nice to eventually make this diagnostic more clear,
+    // e.g., by referencing the original declaration or by saying *why* this
+    // reference is outside the range.
+    auto Report = llvm::make_unique<BugReport>(*BT, WarningMsg, N);
+    Report->addRange(S->getSourceRange());
+    C.emitReport(std::move(Report));
+  }
+}
+
+void CStringChecker::emitNotCStringBug(CheckerContext &C, ProgramStateRef State,
+                                       const Stmt *S,
+                                       StringRef WarningMsg) const {
+  if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
+    if (!BT_NotCString)
+      BT_NotCString.reset(new BuiltinBug(
+          Filter.CheckNameCStringNotNullTerm, categories::UnixAPI,
+          "Argument is not a null-terminated string."));
+
+    auto Report = llvm::make_unique<BugReport>(*BT_NotCString, WarningMsg, N);
+
+    Report->addRange(S->getSourceRange());
+    C.emitReport(std::move(Report));
+  }
+}
+
+void CStringChecker::emitAdditionOverflowBug(CheckerContext &C,
+                                             ProgramStateRef State) const {
+  if (ExplodedNode *N = C.generateErrorNode(State)) {
+    if (!BT_NotCString)
+      BT_NotCString.reset(
+          new BuiltinBug(Filter.CheckNameCStringOutOfBounds, "API",
+                         "Sum of expressions causes overflow."));
+
+    // This isn't a great error message, but this should never occur in real
+    // code anyway -- you'd have to create a buffer longer than a size_t can
+    // represent, which is sort of a contradiction.
+    const char *WarningMsg =
+        "This expression will create a string whose length is too big to "
+        "be represented as a size_t";
+
+    auto Report = llvm::make_unique<BugReport>(*BT_NotCString, WarningMsg, N);
+    C.emitReport(std::move(Report));
+  }
+}
+
 ProgramStateRef CStringChecker::checkAdditionOverflow(CheckerContext &C,
                                                      ProgramStateRef state,
                                                      NonLoc left,
                                                      NonLoc right) const {
   // If out-of-bounds checking is turned off, skip the rest.
   if (!Filter.CheckCStringOutOfBounds)
     return state;
 
SVal willOverflow = svalBuilder.evalBinOpNN(state, BO_GT, left,
                                                 *maxMinusRightNL, cmpTy);
 
     ProgramStateRef stateOverflow, stateOkay;
     std::tie(stateOverflow, stateOkay) =
       state->assume(willOverflow.castAs<DefinedOrUnknownSVal>());
 
     if (stateOverflow && !stateOkay) {
       // We have an overflow. Emit a bug report.
-      ExplodedNode *N = C.generateErrorNode(stateOverflow);
-      if (!N)
-        return nullptr;
-
-      if (!BT_AdditionOverflow)
-        BT_AdditionOverflow.reset(
-            new BuiltinBug(Filter.CheckNameCStringOutOfBounds, "API",
-                           "Sum of expressions causes overflow"));
-
-      // This isn't a great error message, but this should never occur in real
-      // code anyway -- you'd have to create a buffer longer than a size_t can
-      // represent, which is sort of a contradiction.
-      const char *warning =
-        "This expression will create a string whose length is too big to "
-        "be represented as a size_t";
-
-      // Generate a report for this bug.
-      C.emitReport(
-          llvm::make_unique<BugReport>(*BT_AdditionOverflow, warning, N));
-
+      emitAdditionOverflowBug(C, stateOverflow);
       return nullptr;
     }
 
     // From now on, assume an overflow didn't occur.
     assert(stateOkay);
     state = stateOkay;
   }
 
SVal CStringChecker::getCStringLength(CheckerContext &C, ProgramStateRef &state,
                                       const Expr *Ex, SVal Buf,
                                       bool hypothetical) const {
   const MemRegion *MR = Buf.getAsRegion();
   if (!MR) {
     // If we can't get a region, see if it's something we /know/ isn't a
     // C string. In the context of locations, the only time we can issue such
     // a warning is for labels.
     if (Optional<loc::GotoLabel> Label = Buf.getAs<loc::GotoLabel>()) {
-      if (!Filter.CheckCStringNotNullTerm)
-        return UndefinedVal();
-
-      if (ExplodedNode *N = C.generateNonFatalErrorNode(state)) {
-        if (!BT_NotCString)
-          BT_NotCString.reset(new BuiltinBug(
-              Filter.CheckNameCStringNotNullTerm, categories::UnixAPI,
-              "Argument is not a null-terminated string."));
-
+      if (Filter.CheckCStringNotNullTerm) {
         SmallString<120> buf;
         llvm::raw_svector_ostream os(buf);
         assert(CurrentFunctionDescription);
         os << "Argument to " << CurrentFunctionDescription
            << " is the address of the label '" << Label->getLabel()->getName()
            << "', which is not a null-terminated string";
 
-        // Generate a report for this bug.
-        auto report = llvm::make_unique<BugReport>(*BT_NotCString, os.str(), N);
-
-        report->addRange(Ex->getSourceRange());
-        C.emitReport(std::move(report));
+        emitNotCStringBug(C, state, Ex, os.str());
       }
       return UndefinedVal();
-
     }
 
     // If it's not a region and not a label, give up.
     return UnknownVal();
   }
 
   // If we have a region, strip casts from it and see if we can figure out
   // its length. For anything we can't figure out, just return UnknownVal.
SVal CStringChecker::getCStringLength(CheckerContext &C, ProgramStateRef &state,
   case MemRegion::ElementRegionKind:
     // FIXME: How can we handle this? It's not good enough to subtract the
     // offset from the base string length; consider "123\x00567" and &a[5].
     return UnknownVal();
   default:
     // Other regions (mostly non-data) can't have a reliable C string length.
     // In this case, an error is emitted and UndefinedVal is returned.
     // The caller should always be prepared to handle this case.
-    if (!Filter.CheckCStringNotNullTerm)
-      return UndefinedVal();
-
-    if (ExplodedNode *N = C.generateNonFatalErrorNode(state)) {
-      if (!BT_NotCString)
-        BT_NotCString.reset(new BuiltinBug(
-            Filter.CheckNameCStringNotNullTerm, categories::UnixAPI,
-            "Argument is not a null-terminated string."));
-
+    if (Filter.CheckCStringNotNullTerm) {
       SmallString<120> buf;
       llvm::raw_svector_ostream os(buf);
 
       assert(CurrentFunctionDescription);
       os << "Argument to " << CurrentFunctionDescription << " is ";
 
       if (SummarizeRegion(os, C.getASTContext(), MR))
         os << ", which is not a null-terminated string";
       else
         os << "not a null-terminated string";
 
-      // Generate a report for this bug.
-      auto report = llvm::make_unique<BugReport>(*BT_NotCString, os.str(), N);
-
-      report->addRange(Ex->getSourceRange());
-      C.emitReport(std::move(report));
+      emitNotCStringBug(C, state, Ex, os.str());
     }
-
     return UndefinedVal();
   }
 }
 
 const StringLiteral *CStringChecker::getCStringLiteral(CheckerContext &C,
   ProgramStateRef &state, const Expr *expr, SVal val) const {
 
   // Get the memory region pointed to by the val.