[analyzer] Add std::string checker

[steakhal]

Summary

This patch adds a checker checking std::string operations.
At first, it only checks the std::string single const char * constructor for nullness.
If It might be null, it will constrain it to non-null and place a note tag there.

[steakhal]

clang/lib/StaticAnalyzer/Checkers/StringChecker.cpp

96

Hmm, I probably need to check if the pointer is interesting and emit this not only if so.

clang/test/Analysis/diagnostics/explicit-suppression.cpp

22

The mock header changed, thus the line numbers changed.

[steakhal]

Remove an accidentally added file. now the diff should be clean.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StringChecker.cpp

38–40

Constructors have identifiers. They totally work in other checkers. You still need to check for the right overload manually but in general they should work.

If you want caching, it probably makes sense to implement it inside CallDescription so that other checkers could take advantage of it as well.

84–89

You can drop the SValBuilder shenanigans and assume on the original value because if (x != nullptr) is entirely equivalent to if (x).

96

I'm not sure this note is necessary. Null dereference checker doesn't make this note. You also made it non-prunable which will cause any nested stack frame that has such note to be brought into the report (never pruned) which may result in hundreds of uninteresting path notes per report.

Such note could be useful in case the pointer value is "interesting", eg. it's tracked because it's used in the condition later down the path. If this is the aim, you should check for this explicitly and only then add a note. In this case a similar note can be added to other null dereference related checkers.

104

The modern-day tradition is to keep these as members and initialize inline:

class StringChecker {
  BugType BT_NullCStringParam { this, "...", "..." };
  ...
};

106

This is basically a null dereference bug. Let's use the same bug category, probably "logic error".

clang/test/Analysis/Inputs/system-header-simulator-cxx.h

567

I think this is supposed to be a template parameter. It probably doesn't matter but the closer we have our tests to the real-world situation the better.

[NoQ]

Generally I think this is a great start! We might indeed be able to enable it by default from the start. What further plans do you have for this checker?

clang/lib/StaticAnalyzer/Checkers/StringChecker.cpp

110

Another great thing to do here is trackExpressionValue(Call.getArgExpr(0)). This would basically take care of all the notes.

[steakhal]

Thanks for the review @NoQ!

clang/lib/StaticAnalyzer/Checkers/StringChecker.cpp

38–40

Okay, I'm gonna investigate this more thoroughly. I'm not exactly sure why did I choose to do it this way, but at first, I tried using CallDescriptions for sure. Let me come back to this.

About the caching, that is an orthogonal feature, so even if I implement it in the map, it will be done in a follow-up revision.

84–89

Oh yeah!

96

Such note could be useful in case the pointer value is "interesting", eg. it's tracked because it's used in the condition later down the path.

Yeah, I tried to highlight exactly the same in the comment of the next line. I'm gonna check for interestingness in the next update.

104

Thanks.

106

You are right.

110

Oh sure, I'm not even sure why I stuck with the markinteresting() here. It's simply a mistake.

clang/test/Analysis/Inputs/system-header-simulator-cxx.h

567

I did not want to introduce the std::allocator<> stuff, hence chose this private class instead.
Do you insist?

I already had to make tradeoffs. I had to drop constexpr and noexcept and the last deleted overload as well since this header is included in older standard version tests.

clang/test/Analysis/std-string.cpp

14–23

To be fair, even if the checker would accidentally select a wrong overload, it would still not emit diagnostic.
So, the no-warning here is somewhat misleading.
I've tested though, and the checker won't select any of these.

[steakhal]

• Address review comments.
• Add more tests.
• Rebase on top D111535.

[steakhal]

In D111247#3047135, @NoQ wrote:

Generally I think this is a great start! We might indeed be able to enable it by default from the start. What further plans do you have for this checker?

Right now I don't plan to add more to this checker.

clang/lib/StaticAnalyzer/Checkers/StringChecker.cpp

38–40

In general, the CallDescription cannot cache the resolved decl, since that would require to match for the given overload. But we can only match for a qualified name and some parameter counts. Matching for the types of operands is generally done at a subsequent step.
So, I'm sticking to caching here for now.

The extension adding support for matching constructors is in D111535.

104

Wait, I thought we do this to lazily initialize the BugType to prevent unnecessary allocations.

[steakhal]

Upload not only the diff to the last revision but actually the complete change :D

[steakhal]

Addressed review comments.
Added a lot of tests.

[martong]

I like this very much, nice work!

clang/lib/StaticAnalyzer/Checkers/StringChecker.cpp

49–64

Argh, I wish CallDescription was able to handle signatures! (StdLibraryFunctionsChecker would receive a big refactor then).

69

Having an early return on if (!isCharToStringCtor(....) could reduce the nesting below.

71–72

How could this happen? Do you have a test case for that?

clang/test/Analysis/std-string.cpp

48–55

Ahhh, this is great!

[steakhal]

• rotated if statement
• getAs<Loc>() -> castAs<Loc>()

clang/lib/StaticAnalyzer/Checkers/StringChecker.cpp

49–64

Yes. Me too.

69

Yea, I'll rotate the if statement.

71–72

Hm, seems reasonable.

[steakhal]

I think I addressed all comments I wanted.
I think it's pretty solid, and I want to move forward with this one.
@martong

[martong]

In D111247#3074637, @steakhal wrote:

I think I addressed all comments I wanted.
I think it's pretty solid, and I want to move forward with this one.
@martong

Solid indeed! Let's move forward, thanks!

[steakhal]

In D111247#3075311, @martong wrote:

In D111247#3074637, @steakhal wrote:

I think I addressed all comments I wanted.
I think it's pretty solid, and I want to move forward with this one.
@martong

Solid indeed! Let's move forward, thanks!

Thanks!
I'm gonna commit this tomorrow if there is no objection.

clang/docs/analyzer/checkers.rst

  }
 
 .. _cplusplus-SelfAssignment:
 
 cplusplus.SelfAssignment (C++)
 """"""""""""""""""""""""""""""
 Checks C++ copy and move assignment operators for self assignment.
 
+.. _cplusplus-StringChecker:
+
+cplusplus.StringChecker (C++)
+"""""""""""""""""""""""""""""
+Checks std::string operations.
+
+.. code-block:: cpp
+
+ #include <string>
+
+ void f(const char *p) {
+   if (!p) {
+     std::string msg(p); // warn: p is NULL
+   }
+ }
+
 .. _deadcode-checkers:
 
 deadcode
 ^^^^^^^^
 
 Dead Code Checkers.
 
 .. _deadcode-DeadStores:

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

CmdLineOption<Boolean,
                   "ModelSmartPtrDereference",
                   "Enable modeling for SmartPtr null dereferences",
                   "false",
                   InAlpha,
                   Hide>,
   ]>,
   Hidden;
 
+def StringChecker: Checker<"StringChecker">,
+  HelpText<"Checks C++ std::string bugs">,
+  Documentation<HasDocumentation>;
+
 def MoveChecker: Checker<"Move">,
   HelpText<"Find use-after-move bugs in C++">,
   CheckerOptions<[
     CmdLineOption<String,
                   "WarnOn",
                   "In non-aggressive mode, only warn on use-after-move of "
                   "local variables (or local rvalue references) and of STL "
                   "objects. The former is possible because local variables (or "

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   RunLoopAutoreleaseLeakChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrChecker.cpp
   SmartPtrModeling.cpp
   StackAddrEscapeChecker.cpp
   StdLibraryFunctionsChecker.cpp
   STLAlgorithmModeling.cpp
   StreamChecker.cpp
+  StringChecker.cpp
   Taint.cpp
   TaintTesterChecker.cpp
   TestAfterDivZeroChecker.cpp
   TraversalChecker.cpp
   TrustNonnullChecker.cpp
   UndefBranchChecker.cpp
   UndefCapturedBlockVarChecker.cpp
   UndefResultChecker.cpp

clang/lib/StaticAnalyzer/Checkers/StringChecker.cpp

+//=== StringChecker.cpp -------------------------------------------*- C++ -*--//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements the modeling of the std::basic_string type.
+// This involves checking preconditions of the operations and applying the
+// effects of the operations, e.g. their post-conditions.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class StringChecker : public Checker<check::PreCall> {
+  BugType BT_Null{this, "Dereference of null pointer", categories::LogicError};
+  mutable const FunctionDecl *StringConstCharPtrCtor = nullptr;
+  mutable CanQualType SizeTypeTy;
+  const CallDescription TwoParamStdStringCtor = {
+      {"std", "basic_string", "basic_string"}, 2, 2};
+
+  bool isCharToStringCtor(const CallEvent &Call, const ASTContext &ACtx) const;
+
+public:
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+};
+
+bool StringChecker::isCharToStringCtor(const CallEvent &Call,
+                                       const ASTContext &ACtx) const {
+  if (!Call.isCalled(TwoParamStdStringCtor))
+    return false;
+  const auto *FD = dyn_cast<FunctionDecl>(Call.getDecl());
+  assert(FD);
+
+  // See if we already cached it.
+  if (StringConstCharPtrCtor && StringConstCharPtrCtor == FD)
+    return true;
+
+  // Verify that the parameters have the expected types:
+  // - arg 1: `const CharT *`
+  // - arg 2: some allocator - which is definately not `size_t`.
+  const QualType Arg1Ty = Call.getArgExpr(0)->getType().getCanonicalType();
+  const QualType Arg2Ty = Call.getArgExpr(1)->getType().getCanonicalType();
+
+  if (!Arg1Ty->isPointerType())
+    return false;
+
+  // It makes sure that we don't select the `string(const char* p, size_t len)`
+  // overload accidentally.
+  if (Arg2Ty.getCanonicalType() == ACtx.getSizeType())
+    return false;
+
+  StringConstCharPtrCtor = FD; // Cache the decl of the right overload.
+  return true;
+}
+
+void StringChecker::checkPreCall(const CallEvent &Call,
+                                 CheckerContext &C) const {
+  if (!isCharToStringCtor(Call, C.getASTContext()))
+    return;
+  const Loc Param = Call.getArgSVal(0).castAs<Loc>();
+
+  // We managed to constrain the parameter to non-null.
+  ProgramStateRef NotNull, Null;
+  std::tie(NotNull, Null) = C.getState()->assume(Param);
+
+  if (NotNull) {
+    const auto Callback = [Param](PathSensitiveBugReport &BR) -> std::string {
+      return BR.isInteresting(Param) ? "Assuming the pointer is not null." : "";
+    };
+
+    // Emit note only if this operation constrained the pointer to be null.
+    C.addTransition(NotNull, Null ? C.getNoteTag(Callback) : nullptr);
+    return;
+  }
+
+  // We found a path on which the parameter is NULL.
+  if (ExplodedNode *N = C.generateErrorNode(C.getState())) {
+    auto R = std::make_unique<PathSensitiveBugReport>(
+        BT_Null, "The parameter must not be null", N);
+    bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R);
+    C.emitReport(std::move(R));
+  }
+}
+
+} // end anonymous namespace
+
+void ento::registerStringChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<StringChecker>();
+}
+
+bool ento::shouldRegisterStringChecker(const CheckerManager &) { return true; }

clang/test/Analysis/Inputs/system-header-simulator-cxx.h

public:
     const_iterator cend() const { return const_iterator(); }
 
     T& front() { return *begin(); }
     const T& front() const { return *begin(); }
   };
 
   template <typename CharT>
   class basic_string {
+    class Allocator {};
+
   public:
-    basic_string();
-    basic_string(const CharT *s);
+    basic_string() : basic_string(Allocator()) {}
+    explicit basic_string(const Allocator &alloc);
+    basic_string(size_type count, CharT ch,
+                 const Allocator &alloc = Allocator());
+    basic_string(const basic_string &other,
+                 size_type pos,
+                 const Allocator &alloc = Allocator());
+    basic_string(const basic_string &other,
+                 size_type pos, size_type count,
+                 const Allocator &alloc = Allocator());
+    basic_string(const CharT *s, size_type count,
+                 const Allocator &alloc = Allocator());
+    basic_string(const CharT *s,
+                 const Allocator &alloc = Allocator());
+    template <class InputIt>
+    basic_string(InputIt first, InputIt last,
+                 const Allocator &alloc = Allocator());
+    basic_string(const basic_string &other);
+    basic_string(const basic_string &other,
+                 const Allocator &alloc);
+    basic_string(basic_string &&other);
+    basic_string(basic_string &&other,
+                 const Allocator &alloc);
+    basic_string(std::initializer_list<CharT> ilist,
+                 const Allocator &alloc = Allocator());
+    template <class T>
+    basic_string(const T &t, size_type pos, size_type n,
+                 const Allocator &alloc = Allocator());
+    // basic_string(std::nullptr_t) = delete;
 
     ~basic_string();
     void clear();
 
     basic_string &operator=(const basic_string &str);
     basic_string &operator+=(const basic_string &str);
 
     const CharT *c_str() const;
     const CharT *data() const;
     CharT *data();
 
+    const char *begin() const;
+    const char *end() const;
+
     basic_string &append(size_type count, CharT ch);
     basic_string &assign(size_type count, CharT ch);
     basic_string &erase(size_type index, size_type count);
     basic_string &insert(size_type index, size_type count, CharT ch);
     basic_string &replace(size_type pos, size_type count, const basic_string &str);
     void pop_back();
     void push_back(CharT ch);
     void reserve(size_type new_cap);

clang/test/Analysis/diagnostics/explicit-suppression.cpp

class C {
   // The virtual function is to make C not trivially copy assignable so that we call the
   // variant of std::copy() that does not defer to memmove().
   virtual int f();
 };
 
 void testCopyNull(C *I, C *E) {
   std::copy(I, E, (C *)0);
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:709 {{Called C++ object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:741 {{Called C++ object pointer is null}}
 #endif
 }

clang/test/Analysis/std-string.cpp

+// RUN: %clang_analyze_cc1 -std=c++14 %s -verify                  \
+// RUN:   -analyzer-checker=core,unix.Malloc,debug.ExprInspection \
+// RUN:   -analyzer-checker=cplusplus.StringChecker               \
+// RUN:   -analyzer-config eagerly-assume=false                   \
+// RUN:   -analyzer-output=text
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void clang_analyzer_eval(bool);
+void clang_analyzer_warnIfReached();
+
+void free(void *ptr);
+
+void irrelevant_std_string_ctors(const char *p) {
+  std::string x1;                             // no-warning
+  std::string x2(2, 'x');                     // no-warning
+  std::string x3(x1, /*pos=*/2);              // no-warning
+  std::string x4(x1, /*pos=*/2, /*count=*/2); // no-warning
+  std::string x5(p, /*count=*/(size_t)2);     // no-warning
+  // skip std::string(const char*)
+  std::string x6(x1.begin(), x1.end()); // no-warning
+  std::string x7(x1);                   // no-warning
+  std::string x8(std::move(x1));        // no-warning
+  std::string x9({'a', 'b', '\0'});     // no-warning
+}
+
+void null_cstring_parameter(const char *p) {
+  clang_analyzer_eval(p == 0); // expected-warning {{UNKNOWN}} expected-note {{UNKNOWN}}
+  if (!p) {
+    // expected-note@-1 2 {{Assuming 'p' is null}}
+    // expected-note@-2 2 {{Taking true branch}}
+    clang_analyzer_eval(p == 0); // expected-warning {{TRUE}} expected-note {{TRUE}}
+    std::string x(p);
+    // expected-warning@-1 {{The parameter must not be null}}
+    // expected-note@-2    {{The parameter must not be null}}
+    clang_analyzer_warnIfReached(); // no-warning
+  }
+}
+
+void null_constant_parameter() {
+  std::string x((char *)0);
+  // expected-warning@-1 {{The parameter must not be null}}
+  // expected-note@-2    {{The parameter must not be null}}
+}
+
+void ctor_notetag_on_constraining_symbol(const char *p) {
+  clang_analyzer_eval(p == 0); // expected-warning {{UNKNOWN}} expected-note {{UNKNOWN}}
+  std::string x(p);            // expected-note {{Assuming the pointer is not null}}
+  clang_analyzer_eval(p == 0); // expected-warning {{FALSE}} expected-note {{FALSE}}
+
+  free((void *)p); // expected-note {{Memory is released}}
+  free((void *)p);
+  // expected-warning@-1 {{Attempt to free released memory}}
+  // expected-note@-2    {{Attempt to free released memory}}
+}
+
+void ctor_no_notetag_symbol_already_constrained(const char *p) {
+  // expected-note@+2 + {{Assuming 'p' is non-null}}
+  // expected-note@+1 + {{Taking false branch}}
+  if (!p)
+    return;
+
+  clang_analyzer_eval(p == 0); // expected-warning {{FALSE}} expected-note {{FALSE}}
+  std::string x(p);            // no-note: 'p' is already constrained to be non-null.
+  clang_analyzer_eval(p == 0); // expected-warning {{FALSE}} expected-note {{FALSE}}
+
+  free((void *)p); // expected-note {{Memory is released}}
+  free((void *)p);
+  // expected-warning@-1 {{Attempt to free released memory}}
+  // expected-note@-2    {{Attempt to free released memory}}
+}
+
+void ctor_no_notetag_if_not_interesting(const char *p1, const char *p2) {
+  std::string s1(p1); // expected-note {{Assuming the pointer is not null}}
+  std::string s2(p2); // no-note: s2 is not interesting
+
+  free((void *)p1); // expected-note {{Memory is released}}
+  free((void *)p1);
+  // expected-warning@-1 {{Attempt to free released memory}}
+  // expected-note@-2    {{Attempt to free released memory}}
+}