Index: clang/include/clang/Analysis/ProgramPoint.h =================================================================== --- clang/include/clang/Analysis/ProgramPoint.h +++ clang/include/clang/Analysis/ProgramPoint.h @@ -191,6 +191,10 @@ return ID.ComputeHash(); } + /// If the program point corresponds to a statement, retrieve that statement. + /// Useful for figuring out where to put a warning or a note. + const Stmt *getStmtForDiagnostics() const; + bool operator==(const ProgramPoint & RHS) const { return Data1 == RHS.Data1 && Data2 == RHS.Data2 && Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -300,6 +300,13 @@ "are not null">, Documentation; +def SuppressInvalidationRelatedReportsChecker + : Checker<"SuppressInvalidationRelatedReports">, + HelpText<"Suppress reports where the expression that was a cause of a bug " + "or other expressions related to this expression have been " + "invalidated">, + Documentation; + } // end "apiModeling" //===----------------------------------------------------------------------===// Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h +++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h @@ -16,8 +16,11 @@ #include "clang/Analysis/ProgramPoint.h" #include "clang/Basic/LLVM.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h" #include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/Store.h" #include "llvm/ADT/FoldingSet.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringRef.h" @@ -367,6 +370,56 @@ PathSensitiveBugReport &BR) override; }; +/// Looks for and suppresses reports where the tracked region's last value +/// change was a result of an invalidation, which is prone to false positives. +/// In this example, foo *could* change flags value, but it most likely doesn't. +/// +/// int flag; +/// void foo(); // Function body is unknown. +/// +/// void f() { +/// flag = 0; // flag is initialized to false. +/// int *x = 0; // x is initialized to a nullptr +/// +/// foo(); // Invalidate flag's value. +/// +/// if (flag) // Assume flag is true. +/// *x = 5; // x is dereferenced as a nullptr +/// } +class SuppressInvalidationVisitor final : public BugReporterVisitor { + /// The region whose changes we're analyzing. + const MemRegion *const R; + /// R may not be directly listed in the invalidation set, but its super region + /// might be. In case R *is* in the invalidation set, this field is equal to + /// it. + const MemRegion *SuperRegion; + + /// Track until the last value change only, whether or not its an + /// invalidation. + bool IsSatisfied = false; + +public: + SuppressInvalidationVisitor(const MemRegion *R, const ExplodedNode *N); + + void Profile(llvm::FoldingSetNodeID &ID) const override; + + static const char *getTag(); + + PathDiagnosticPieceRef VisitNode(const ExplodedNode *Succ, + BugReporterContext &BRC, + PathSensitiveBugReport &BR) override; +}; + +} // namespace ento +} // namespace clang + +REGISTER_MAP_FACTORY_WITH_PROGRAMSTATE(HadInvalidation, + const clang::ento::MemRegion *, + clang::ProgramPoint) + +namespace clang { +namespace ento { + /// The bug visitor will walk all the nodes in a path and collect all the /// constraints. When it reaches the root node, will create a refutation /// manager and check if the constraints are satisfiable @@ -388,7 +441,6 @@ PathSensitiveBugReport &BR) override; }; - /// The visitor detects NoteTags and displays the event notes they contain. class TagVisitor : public BugReporterVisitor { public: Index: clang/lib/Analysis/ProgramPoint.cpp =================================================================== --- clang/lib/Analysis/ProgramPoint.cpp +++ clang/lib/Analysis/ProgramPoint.cpp @@ -43,6 +43,24 @@ } } +const Stmt *ProgramPoint::getStmtForDiagnostics() const { + if (auto SP = getAs()) + return SP->getStmt(); + if (auto BE = getAs()) + return BE->getSrc()->getTerminatorStmt(); + if (auto CE = getAs()) + return CE->getCallExpr(); + if (auto CEE = getAs()) + return CEE->getCalleeContext()->getCallSite(); + if (auto PIPP = getAs()) + return PIPP->getInitializer()->getInit(); + if (auto CEB = getAs()) + return CEB->getReturnStmt(); + if (auto FEP = getAs()) + return FEP->getStmt(); + return nullptr; +} + LLVM_DUMP_METHOD void ProgramPoint::dump() const { return printJson(llvm::errs()); } Index: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -102,6 +102,7 @@ StdLibraryFunctionsChecker.cpp STLAlgorithmModeling.cpp StreamChecker.cpp + SuppressInvalidationRelatedReports.cpp Taint.cpp TaintTesterChecker.cpp TestAfterDivZeroChecker.cpp Index: clang/lib/StaticAnalyzer/Checkers/SuppressInvalidationRelatedReports.cpp =================================================================== --- /dev/null +++ clang/lib/StaticAnalyzer/Checkers/SuppressInvalidationRelatedReports.cpp @@ -0,0 +1,69 @@ +//=== FIXME.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 +// +//===----------------------------------------------------------------------===// +// +// TODO +// +//===----------------------------------------------------------------------===// + +#include "clang/AST/ExprCXX.h" +#include "clang/Basic/IdentifierTable.h" +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/Support/ErrorHandling.h" + +using namespace clang; +using namespace ento; + +namespace { + +class SuppressInvalidationRelatedReportsChecker + : public Checker { +public: + ProgramStateRef + checkRegionChanges(ProgramStateRef State, + const InvalidatedSymbols *Invalidated, + ArrayRef ExplicitRegions, + ArrayRef Regions, + const LocationContext *LCtx, const CallEvent *Call) const { + if (!Call) + return State; + + for (const MemRegion *MR : Regions) + State = State->set(MR, Call->getProgramPoint()); + + return State; + } + + void printState(raw_ostream &Out, ProgramStateRef State, const char *NL, + const char *Sep) const override { + for (const HadInvalidation::value_type &I : State->get()) { + I.first->dumpToStream(Out); + Out << " was invalidated on '"; + if (const Stmt *InvalidatingStmt = I.second.getStmtForDiagnostics()) + InvalidatingStmt->getBeginLoc().print( + Out, State->getAnalysisManager().getSourceManager()); + Out << "'" << NL; + } + } +}; + +} // end anonymous namespace + +void ento::registerSuppressInvalidationRelatedReportsChecker( + CheckerManager &mgr) { + mgr.registerChecker(); +} + +bool ento::shouldRegisterSuppressInvalidationRelatedReportsChecker( + const LangOptions &LO) { + return true; +} Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -32,6 +32,7 @@ #include "clang/Basic/LLVM.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" +#include "clang/Basic/TargetBuiltins.h" #include "clang/Lex/Lexer.h" #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" @@ -1595,6 +1596,60 @@ return nullptr; } +//===----------------------------------------------------------------------===// +// Implementation of SuppressInvalidationVisitor. +//===----------------------------------------------------------------------===// + +SuppressInvalidationVisitor::SuppressInvalidationVisitor(const MemRegion *R, + const ExplodedNode *N) + : R(R), SuperRegion(R) { + for (HadInvalidation::value_type &HI : N->getState()->get()) + if (R->isSubRegionOf(HI.first)) + SuperRegion = HI.first; + if (!N->getState()->contains(SuperRegion)) + IsSatisfied = true; +} + +void SuppressInvalidationVisitor::Profile(llvm::FoldingSetNodeID &ID) const { + static int id = 0; + ID.AddPointer(&id); + ID.Add(R); +} + +const char *SuppressInvalidationVisitor::getTag() { + return "InvalidSuppVisitor"; +} + +PathDiagnosticPieceRef +SuppressInvalidationVisitor::VisitNode(const ExplodedNode *Succ, + BugReporterContext &BRC, + PathSensitiveBugReport &BR) { + if (IsSatisfied) + return nullptr; + + ProgramStateRef SuccState = Succ->getState(); + ProgramStateRef PredState = Succ->getFirstPred()->getState(); + + // Look for the last write of R. + if (PredState->getSVal(R) != SuccState->getSVal(R)) { + Succ->getStmtForDiagnostics(); + + // We found the last write, invalidations previous to this point are not + // interesting. + IsSatisfied = true; + + // R may have been invalidated multiple times, is the last invalidation + // also the last write? + if (Succ->getLocation() != *SuccState->get(SuperRegion)) + return nullptr; + const LocationContext *CurLC = Succ->getLocationContext(); + + BR.markInvalid("Suppress IV", CurLC); + return nullptr; + } + return nullptr; +} + //===----------------------------------------------------------------------===// // Implementation of SuppressInlineDefensiveChecksVisitor. //===----------------------------------------------------------------------===// @@ -1811,9 +1866,9 @@ // isn't sufficient, because a new visitor is created for each tracked // expression, hence the BugReport level set. if (BR.addTrackedCondition(N)) { - bugreporter::trackExpressionValue( - N, Condition, BR, bugreporter::TrackingKind::Condition, - /*EnableNullFPSuppression=*/true); + bugreporter::trackExpressionValue(N, Condition, BR, + bugreporter::TrackingKind::Condition, + /*EnableNullFPSuppression=*/true); return constructDebugPieceForTrackedCondition(Condition, N, BRC); } } @@ -1988,6 +2043,9 @@ if (R) { + report.addVisitor( + std::make_unique(R, InputNode)); + // Mark both the variable region and its contents as interesting. SVal V = LVState->getRawSVal(loc::MemRegionVal(R)); report.addVisitor( @@ -2006,7 +2064,7 @@ V.castAs(), false)); // Add visitor, which will suppress inline defensive checks. - if (auto DV = V.getAs()) + if (auto DV = V.getAs()) { if (!DV->isZeroConstant() && EnableNullFPSuppression) { // Note that LVNode may be too late (i.e., too far from the InputNode) // because the lvalue may have been computed before the inlined call @@ -2017,6 +2075,7 @@ std::make_unique( *DV, InputNode)); } + } if (auto KV = V.getAs()) report.addVisitor(std::make_unique( Index: clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp +++ clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp @@ -330,24 +330,7 @@ ->getCallSite(); } // Otherwise, see if the node's program point directly points to a statement. - // FIXME: Refactor into a ProgramPoint method? - ProgramPoint P = getLocation(); - if (auto SP = P.getAs()) - return SP->getStmt(); - if (auto BE = P.getAs()) - return BE->getSrc()->getTerminatorStmt(); - if (auto CE = P.getAs()) - return CE->getCallExpr(); - if (auto CEE = P.getAs()) - return CEE->getCalleeContext()->getCallSite(); - if (auto PIPP = P.getAs()) - return PIPP->getInitializer()->getInit(); - if (auto CEB = P.getAs()) - return CEB->getReturnStmt(); - if (auto FEP = P.getAs()) - return FEP->getStmt(); - - return nullptr; + return getLocation().getStmtForDiagnostics(); } const Stmt *ExplodedNode::getNextStmtForDiagnostics() const { Index: clang/test/Analysis/analyzer-enabled-checkers.c =================================================================== --- clang/test/Analysis/analyzer-enabled-checkers.c +++ clang/test/Analysis/analyzer-enabled-checkers.c @@ -7,6 +7,7 @@ // CHECK: OVERVIEW: Clang Static Analyzer Enabled Checkers List // CHECK-EMPTY: // CHECK-NEXT: apiModeling.StdCLibraryFunctions +// CHECK-NEXT: apiModeling.SuppressInvalidationRelatedReports // CHECK-NEXT: apiModeling.TrustNonnull // CHECK-NEXT: apiModeling.llvm.CastValue // CHECK-NEXT: apiModeling.llvm.ReturnValue Index: clang/test/Analysis/suppress-invalidation-related-reports.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/suppress-invalidation-related-reports.cpp @@ -0,0 +1,102 @@ +// RUN: %clang_analyze_cc1 -verify=expected %s \ +// RUN: -analyzer-checker=core,apiModeling.SuppressInvalidationRelatedReports + +// RUN: %clang_analyze_cc1 -verify=non-suppressed,expected %s \ +// RUN: -analyzer-checker=core + +namespace invalidated_global { +int flag; +void foo(); + +void f() { + flag = 0; // Initialize flag to false. + int *x = 0; // x is initialized to a nullptr. + + foo(); // Invalidate flag's value. + + if (flag) // Assume flag is true. + *x = 5; // non-suppressed-warning{{Dereference of null pointer}} +} +} // namespace invalidated_global + +namespace invalidated_field { +struct S { + int flag; + void invalidate(); +}; + +void f() { + S s; + s.flag = 0; // Initialize s.flag to false. + int *x = 0; // x is initialized to a nullptr. + + s.invalidate(); // Invalidate s.flag's value. + + if (s.flag) // Assume s.flag is true. + *x = 5; // non-suppressed-warning{{Dereference of null pointer}} +} +} // namespace invalidated_field + +namespace invalidated_field_before_bind { +struct S { + int flag; + void invalidate(); + void setFlagToTrue() { + invalidate(); + flag = 1; + } +}; + +void f() { + S s; + s.flag = 0; // Initialize s.flag to false. + int *x = 0; // x is initialized to a nullptr. + + s.setFlagToTrue(); // Invalidate s.flag's, but also set it to true. + + if (s.flag) // Assume s.flag is true. + *x = 5; // expected-warning{{Dereference of null pointer}} +} +} // namespace invalidated_field_before_bind + +namespace invalidated_field_twice { +struct S { + int flag; + void invalidate(); + void setFlagToTrue() { + invalidate(); + flag = 1; + invalidate(); + } +}; + +void f() { + S s; + s.flag = 0; // Initialize s.flag to false. + int *x = 0; // x is initialized to a nullptr. + + s.setFlagToTrue(); // Invalidate s.flag's, but also set it to true. + + if (s.flag) // Assume s.flag is true. + *x = 5; // non-suppressed-warning{{Dereference of null pointer}} +} +} // namespace invalidated_field_twice + +namespace invalidated_field_through_bind { +struct S { + int flag; +}; + +void f() { + S s; + s.flag = 0; // Initialize s.flag to false. + S s2; + s2.flag = 1; // Initialize s2.flag to true + int *x = 0; // x is initialized to a nullptr. + + s = s2; // "Invalidate" s.flag's value through bind. + + if (s.flag) // Assume s.flag is true. + *x = 5; // expected-warning{{Dereference of null pointer}} +} +} // namespace invalidated_field_through_bind