From 4d6fb5789fca8857c5161a621892b69a23a53d25 Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Fri, 29 Mar 2019 23:11:10 +0000 Subject: [PATCH] Revert "[analyzer] Introduce a simplified API for adding custom path notes." This reverts commit r357323. ASan leaks found by a buildbot :) Differential Revision: https://reviews.llvm.org/D58367 llvm-svn: 357332 --- clang/include/clang/Analysis/ProgramPoint.h | 5 +- .../StaticAnalyzer/Core/BugReporter/BugReporter.h | 54 ---------------------- .../Core/BugReporter/BugReporterVisitors.h | 12 ----- .../Core/PathSensitive/CheckerContext.h | 18 -------- .../StaticAnalyzer/Core/PathSensitive/ExprEngine.h | 5 -- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp | 47 ++++++++++++++----- clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 1 - .../StaticAnalyzer/Core/BugReporterVisitors.cpp | 24 ---------- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 4 +- clang/test/Analysis/mig.mm | 8 ---- 10 files changed, 40 insertions(+), 138 deletions(-) diff --git a/clang/include/clang/Analysis/ProgramPoint.h b/clang/include/clang/Analysis/ProgramPoint.h index 5b554c1..623f5dc 100644 --- a/clang/include/clang/Analysis/ProgramPoint.h +++ b/clang/include/clang/Analysis/ProgramPoint.h @@ -42,11 +42,12 @@ public: virtual ~ProgramPointTag(); virtual StringRef getTagDescription() const = 0; +protected: /// Used to implement 'isKind' in subclasses. - const void *getTagKind() const { return TagKind; } + const void *getTagKind() { return TagKind; } private: - const void *const TagKind; + const void *TagKind; }; class SimpleProgramPointTag : public ProgramPointTag { diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h index b7deb50..a53efbb 100644 --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h @@ -592,60 +592,6 @@ public: NodeMapClosure& getNodeResolver() { return NMC; } }; - -/// The tag upon which the TagVisitor reacts. Add these in order to display -/// additional PathDiagnosticEventPieces along the path. -class NoteTag : public ProgramPointTag { -public: - using Callback = - std::function; - -private: - static int Kind; - - const Callback Cb; - - NoteTag(Callback &&Cb) : ProgramPointTag(&Kind), Cb(std::move(Cb)) {} - -public: - static bool classof(const ProgramPointTag *T) { - return T->getTagKind() == &Kind; - } - - Optional generateMessage(BugReporterContext &BRC, - BugReport &R) const { - std::string Msg = Cb(BRC, R); - if (Msg.empty()) - return None; - - return std::move(Msg); - } - - StringRef getTagDescription() const override { - // TODO: Remember a few examples of generated messages - // and display them in the ExplodedGraph dump by - // returning them from this function. - return "Note Tag"; - } - - // Manage memory for NoteTag objects. - class Factory { - llvm::BumpPtrAllocator &Alloc; - - public: - Factory(llvm::BumpPtrAllocator &Alloc) : Alloc(Alloc) {} - - const NoteTag *makeNoteTag(Callback &&Cb) { - // We cannot use make_unique because we cannot access the private - // constructor from inside it. - NoteTag *Tag = Alloc.Allocate(); - return new (Tag) NoteTag(std::move(Cb)); - } - }; - - friend class TagVisitor; -}; - } // namespace ento } // namespace clang diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h index e624d0f..03b6560 100644 --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h @@ -14,7 +14,6 @@ #ifndef LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTERVISITORS_H #define LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTERVISITORS_H -#include "clang/Analysis/ProgramPoint.h" #include "clang/Basic/LLVM.h" #include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h" @@ -329,17 +328,6 @@ public: BugReport &BR) override; }; - -/// The visitor detects NoteTags and displays the event notes they contain. -class TagVisitor : public BugReporterVisitor { -public: - void Profile(llvm::FoldingSetNodeID &ID) const override; - - std::shared_ptr VisitNode(const ExplodedNode *N, - BugReporterContext &BRC, - BugReport &R) override; -}; - namespace bugreporter { /// Attempts to add visitors to track expression value back to its point of diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h index 6a49aeb..5710ee7 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h @@ -219,24 +219,6 @@ public: Eng.getBugReporter().emitReport(std::move(R)); } - - /// Produce a program point tag that displays an additional path note - /// to the user. This is a lightweight alternative to the - /// BugReporterVisitor mechanism: instead of visiting the bug report - /// node-by-node to restore the sequence of events that led to discovering - /// a bug, you can add notes as you add your transitions. - const NoteTag *getNoteTag(NoteTag::Callback &&Cb) { - return Eng.getNoteTags().makeNoteTag(std::move(Cb)); - } - - /// A shorthand version of getNoteTag that doesn't require you to accept - /// the BugReporterContext arguments when you don't need it. - const NoteTag *getNoteTag(std::function &&Cb) { - return getNoteTag( - [Cb](BugReporterContext &, BugReport &BR) { return Cb(BR); }); - } - - /// Returns the word that should be used to refer to the declaration /// in the report. StringRef getDeclDescription(const Decl *D); diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h index 185774b..605e1c7 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -22,7 +22,6 @@ #include "clang/Analysis/ProgramPoint.h" #include "clang/Basic/LLVM.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" -#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h" #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h" #include "clang/StaticAnalyzer/Core/PathSensitive/FunctionSummary.h" @@ -156,8 +155,6 @@ private: /// The flag, which specifies the mode of inlining for the engine. InliningModes HowToInline; - NoteTag::Factory NoteTags; - public: ExprEngine(cross_tu::CrossTranslationUnitContext &CTU, AnalysisManager &mgr, SetOfConstDecls *VisitedCalleesIn, @@ -399,8 +396,6 @@ public: SymbolManager &getSymbolManager() { return SymMgr; } MemRegionManager &getRegionManager() { return MRMgr; } - NoteTag::Factory &getNoteTags() { return NoteTags; } - // Functions for external checking of whether we have unfinished work bool wasBlocksExhausted() const { return Engine.wasBlocksExhausted(); } diff --git a/clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp index b5e78b5..685fd88 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp @@ -80,10 +80,43 @@ public: checkReturnAux(RS, C); } + class Visitor : public BugReporterVisitor { + public: + void Profile(llvm::FoldingSetNodeID &ID) const { + static int X = 0; + ID.AddPointer(&X); + } + + std::shared_ptr VisitNode(const ExplodedNode *N, + BugReporterContext &BRC, BugReport &R); + }; }; } // end anonymous namespace -REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, bool) +// FIXME: It's a 'const ParmVarDecl *' but there's no ready-made GDM traits +// specialization for this sort of types. +REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, const void *) + +std::shared_ptr +MIGChecker::Visitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC, + BugReport &R) { + const auto *NewPVD = static_cast( + N->getState()->get()); + const auto *OldPVD = static_cast( + N->getFirstPred()->getState()->get()); + if (OldPVD == NewPVD) + return nullptr; + + assert(NewPVD && "What is deallocated cannot be un-deallocated!"); + SmallString<64> Str; + llvm::raw_svector_ostream OS(Str); + OS << "Value passed through parameter '" << NewPVD->getName() + << "' is deallocated"; + + PathDiagnosticLocation Loc = + PathDiagnosticLocation::create(N->getLocation(), BRC.getSourceManager()); + return std::make_shared(Loc, OS.str()); +} static const ParmVarDecl *getOriginParam(SVal V, CheckerContext &C) { SymbolRef Sym = V.getAsSymbol(); @@ -162,16 +195,7 @@ void MIGChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const { if (!PVD) return; - const NoteTag *T = C.getNoteTag([this, PVD](BugReport &BR) -> std::string { - if (&BR.getBugType() != &BT) - return ""; - SmallString<64> Str; - llvm::raw_svector_ostream OS(Str); - OS << "Value passed through parameter '" << PVD->getName() - << "\' is deallocated"; - return OS.str(); - }); - C.addTransition(C.getState()->set(true), T); + C.addTransition(C.getState()->set(PVD)); } // Returns true if V can potentially represent a "successful" kern_return_t. @@ -236,6 +260,7 @@ void MIGChecker::checkReturnAux(const ReturnStmt *RS, CheckerContext &C) const { R->addRange(RS->getSourceRange()); bugreporter::trackExpressionValue(N, RS->getRetValue(), *R, false); + R->addVisitor(llvm::make_unique()); C.emitReport(std::move(R)); } diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp index b9b579b..1a6314d 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -2612,7 +2612,6 @@ std::pair> findValidReport( R->addVisitor(llvm::make_unique()); R->addVisitor(llvm::make_unique()); R->addVisitor(llvm::make_unique()); - R->addVisitor(llvm::make_unique()); BugReporterContext BRC(Reporter, ErrorGraph.BackMap); diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 1576c09..cd03d16 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -2472,30 +2472,6 @@ FalsePositiveRefutationBRVisitor::VisitNode(const ExplodedNode *N, return nullptr; } -int NoteTag::Kind = 0; - -void TagVisitor::Profile(llvm::FoldingSetNodeID &ID) const { - static int Tag = 0; - ID.AddPointer(&Tag); -} - -std::shared_ptr -TagVisitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC, - BugReport &R) { - ProgramPoint PP = N->getLocation(); - const NoteTag *T = dyn_cast_or_null(PP.getTag()); - if (!T) - return nullptr; - - if (Optional Msg = T->generateMessage(BRC, R)) { - PathDiagnosticLocation Loc = - PathDiagnosticLocation::create(PP, BRC.getSourceManager()); - return std::make_shared(Loc, *Msg); - } - - return nullptr; -} - void FalsePositiveRefutationBRVisitor::Profile( llvm::FoldingSetNodeID &ID) const { static int Tag = 0; diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index e36820fa..fb86cea 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -201,9 +201,7 @@ ExprEngine::ExprEngine(cross_tu::CrossTranslationUnitContext &CTU, svalBuilder(StateMgr.getSValBuilder()), ObjCNoRet(mgr.getASTContext()), BR(mgr, *this), - VisitedCallees(VisitedCalleesIn), - HowToInline(HowToInlineIn), - NoteTags(G.getAllocator()) { + VisitedCallees(VisitedCalleesIn), HowToInline(HowToInlineIn) { unsigned TrimInterval = mgr.options.GraphTrimInterval; if (TrimInterval != 0) { // Enable eager node reclamation when constructing the ExplodedGraph. diff --git a/clang/test/Analysis/mig.mm b/clang/test/Analysis/mig.mm index f7b3789..59442d3 100644 --- a/clang/test/Analysis/mig.mm +++ b/clang/test/Analysis/mig.mm @@ -91,14 +91,6 @@ kern_return_t release_twice(mach_port_name_t port, vm_address_t addr1, vm_addres // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}} } -MIG_SERVER_ROUTINE -kern_return_t no_unrelated_notes(mach_port_name_t port, vm_address_t address, vm_size_t size) { - vm_deallocate(port, address, size); // no-note - 1 / 0; // expected-warning{{Division by zero}} - // expected-note@-1{{Division by zero}} - return KERN_SUCCESS; -} - // Make sure we find the bug when the object is destroyed within an // automatic destructor. MIG_SERVER_ROUTINE -- 2.7.4