Index: clang/lib/StaticAnalyzer/Checkers/StringChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/StringChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/StringChecker.cpp @@ -22,9 +22,11 @@ namespace { class StringChecker : public Checker { - mutable std::unique_ptr BT_NullCStringParam; + 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; @@ -34,25 +36,15 @@ bool StringChecker::isCharToStringCtor(const CallEvent &Call, const ASTContext &ACtx) const { - // FIXME: Currently CallDescription does not support matching - // non-identifier functions. Thus, operator member functions and - // constructors are cannot be matched by them. - const auto *FD = dyn_cast_or_null(Call.getDecl()); + if (!Call.isCalled(TwoParamStdStringCtor)) + return false; + const auto *FD = dyn_cast(Call.getDecl()); + assert(FD); // See if we already cached it. if (StringConstCharPtrCtor && StringConstCharPtrCtor == FD) return true; - if (!FD || FD->getNumParams() != 2) - return false; - - const auto DeclName = FD->getDeclName(); - constexpr auto CXXConstructorName = - clang::DeclarationName::CXXConstructorName; - if (DeclName.getNameKind() != CXXConstructorName || - DeclName.getAsString() != "basic_string") - return false; - // Verify that the parameters have the expected types: // - arg 1: `const CharT *` // - arg 2: some allocator - which is definately not `size_t`. @@ -78,35 +70,26 @@ if (!Param.hasValue()) return; - ProgramStateRef State = C.getState(); - SValBuilder &SVB = C.getSValBuilder(); - const Loc Null = SVB.makeNullWithType(Call.getArgExpr(0)->getType()); - - const auto Assumption = - SVB.evalBinOpLL(State, BO_NE, *Param, Null, SVB.getConditionType()) - .getAs(); - if (!Assumption.hasValue()) - return; + // We managed to constrain the parameter to non-null. + ProgramStateRef NotNull, Null; + std::tie(NotNull, Null) = C.getState()->assume(*Param); - ProgramStateRef NewState = State->assume(*Assumption, true); + if (NotNull) { + const auto Callback = [Param](PathSensitiveBugReport &BR) -> std::string { + return BR.isInteresting(*Param) ? "Assuming the pointer is not null." + : ""; + }; - // We managed to constrain the parameter to non-null. - if (NewState) { - const NoteTag *Msg = C.getNoteTag("Assuming the pointer is not null."); - C.addTransition(NewState, Msg); + // 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(State)) { - if (!BT_NullCStringParam) - BT_NullCStringParam = std::make_unique( - this, "Constructing std::string from a null pointer", - "Unterminated std::string"); - + if (ExplodedNode *N = C.generateErrorNode(C.getState())) { auto R = std::make_unique( - *BT_NullCStringParam, "The parameter must not be null", N); - R->markInteresting(*Param); + BT_Null, "The parameter must not be null", N); + bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R); C.emitReport(std::move(R)); } } Index: clang/test/Analysis/std-string.cpp =================================================================== --- clang/test/Analysis/std-string.cpp +++ clang/test/Analysis/std-string.cpp @@ -1,14 +1,16 @@ -// RUN: %clang_analyze_cc1 -std=c++14 %s -verify \ -// RUN: -analyzer-checker=core,debug.ExprInspection \ -// RUN: -analyzer-checker=cplusplus.StringChecker \ -// RUN: -analyzer-config eagerly-assume=false \ +// 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(int); +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 @@ -41,11 +43,39 @@ // expected-note@-2 {{The parameter must not be null}} } -void ctor_postcondition_applied(const char *p) { +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; - // This call marks 'p' interesting, thus the std::string constructor - // emits a note about constraining 'p' to non-null. 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}} }