Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def =================================================================== --- clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def +++ clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def @@ -10,20 +10,20 @@ #define GADGET(name) #endif -#ifndef UNSAFE_GADGET -#define UNSAFE_GADGET(name) GADGET(name) +#ifndef WARNING_GADGET +#define WARNING_GADGET(name) GADGET(name) #endif -#ifndef SAFE_GADGET -#define SAFE_GADGET(name) GADGET(name) +#ifndef FIXABLE_GADGET +#define FIXABLE_GADGET(name) GADGET(name) #endif -UNSAFE_GADGET(Increment) -UNSAFE_GADGET(Decrement) -UNSAFE_GADGET(ArraySubscript) -UNSAFE_GADGET(UnsafeBufferUsageAttr) -UNSAFE_GADGET(PointerArithmetic) +WARNING_GADGET(Increment) +WARNING_GADGET(Decrement) +WARNING_GADGET(ArraySubscript) +WARNING_GADGET(UnsafeBufferUsageAttr) +WARNING_GADGET(PointerArithmetic) -#undef SAFE_GADGET -#undef UNSAFE_GADGET +#undef FIXABLE_GADGET +#undef WARNING_GADGET #undef GADGET Index: clang/lib/Analysis/UnsafeBufferUsage.cpp =================================================================== --- clang/lib/Analysis/UnsafeBufferUsage.cpp +++ clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -156,20 +156,20 @@ #undef GADGETS }; - /// Determine if a kind is a safe kind. Slower than calling isSafe(). - static bool isSafeKind(Kind K) { + /// Determine if a kind is a safe kind. Slower than calling isWarningGadget(). + static bool isWarningKind(Kind K) { switch (K) { -#define UNSAFE_GADGET(x) \ +#define WARNING_GADGET(x) \ case Kind::x: #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def" -#undef UNSAFE_GADGET - return false; +#undef WARNING_GADGET + return true; -#define SAFE_GADGET(x) \ +#define FIXABLE_GADGET(x) \ case Kind::x: #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def" -#undef SAFE_GADGET - return true; +#undef FIXABLE_GADGET + return false; } llvm_unreachable("Invalid gadget kind!"); } @@ -183,7 +183,7 @@ Kind getKind() const { return K; } - virtual bool isSafe() const = 0; + virtual bool isWarningGadget() const = 0; virtual const Stmt *getBaseStmt() const = 0; /// Returns the list of pointer-type variables on which this gadget performs @@ -191,12 +191,6 @@ /// of all DeclRefExprs in the gadget's AST! virtual DeclUseList getClaimedVarUseSites() const = 0; - /// Returns a fixit that would fix the current gadget according to - /// the current strategy. Returns None if the fix cannot be produced; - /// returns an empty list if no fixes are necessary. - virtual Optional getFixits(const Strategy &) const { - return None; - } virtual ~Gadget() {} @@ -204,18 +198,17 @@ Kind K; }; -using GadgetList = std::vector>; /// Unsafe gadgets correspond to unsafe code patterns that warrants /// an immediate warning. -class UnsafeGadget : public Gadget { +class WarningGadget : public Gadget { public: - UnsafeGadget(Kind K) : Gadget(K) { + WarningGadget(Kind K) : Gadget(K) { assert(classof(this) && "Invalid unsafe gadget kind!"); } - static bool classof(const Gadget *G) { return !isSafeKind(G->getKind()); } - bool isSafe() const override { return false; } + static bool classof(const Gadget *G) { return isWarningKind(G->getKind()); } + bool isWarningGadget() const override { return true; } }; /// Safe gadgets correspond to code patterns that aren't unsafe but need to be @@ -223,24 +216,34 @@ /// gadgets. For example, if a raw pointer-type variable is replaced by /// a safe C++ container, every use of such variable may need to be /// carefully considered and possibly updated. -class SafeGadget : public Gadget { +class FixableGadget : public Gadget { public: - SafeGadget(Kind K) : Gadget(K) { + FixableGadget(Kind K) : Gadget(K) { assert(classof(this) && "Invalid safe gadget kind!"); } - static bool classof(const Gadget *G) { return isSafeKind(G->getKind()); } - bool isSafe() const override { return true; } + static bool classof(const Gadget *G) { return !isWarningKind(G->getKind()); } + bool isWarningGadget() const override { return false; } + + /// Returns a fixit that would fix the current gadget according to + /// the current strategy. Returns None if the fix cannot be produced; + /// returns an empty list if no fixes are necessary. + virtual Optional getFixits(const Strategy &) const { + return None; + } + }; +using FixableGadgetList = std::vector>; +using WarningGadgetList = std::vector>; /// An increment of a pointer-type value is unsafe as it may run the pointer /// out of bounds. -class IncrementGadget : public UnsafeGadget { +class IncrementGadget : public WarningGadget { const UnaryOperator *Op; public: IncrementGadget(const MatchFinder::MatchResult &Result) - : UnsafeGadget(Kind::Increment), + : WarningGadget(Kind::Increment), Op(Result.Nodes.getNodeAs("op")) {} static bool classof(const Gadget *G) { @@ -268,12 +271,12 @@ /// A decrement of a pointer-type value is unsafe as it may run the pointer /// out of bounds. -class DecrementGadget : public UnsafeGadget { +class DecrementGadget : public WarningGadget { const UnaryOperator *Op; public: DecrementGadget(const MatchFinder::MatchResult &Result) - : UnsafeGadget(Kind::Decrement), + : WarningGadget(Kind::Decrement), Op(Result.Nodes.getNodeAs("op")) {} static bool classof(const Gadget *G) { @@ -301,12 +304,12 @@ /// Array subscript expressions on raw pointers as if they're arrays. Unsafe as /// it doesn't have any bounds checks for the array. -class ArraySubscriptGadget : public UnsafeGadget { +class ArraySubscriptGadget : public WarningGadget { const ArraySubscriptExpr *ASE; public: ArraySubscriptGadget(const MatchFinder::MatchResult &Result) - : UnsafeGadget(Kind::ArraySubscript), + : WarningGadget(Kind::ArraySubscript), ASE(Result.Nodes.getNodeAs("arraySubscr")) {} static bool classof(const Gadget *G) { @@ -334,12 +337,12 @@ /// A call of a function or method that performs unchecked buffer operations /// over one of its pointer parameters. -class UnsafeBufferUsageAttrGadget : public UnsafeGadget { +class UnsafeBufferUsageAttrGadget : public WarningGadget { const CallExpr *Op; public: UnsafeBufferUsageAttrGadget(const MatchFinder::MatchResult &Result) - : UnsafeGadget(Kind::UnsafeBufferUsageAttr), + : WarningGadget(Kind::UnsafeBufferUsageAttr), Op(Result.Nodes.getNodeAs("call_expr")) {} static bool classof(const Gadget *G) { @@ -365,13 +368,13 @@ /// \code /// ptr + n | n + ptr | ptr - n | ptr += n | ptr -= n /// \endcode -class PointerArithmeticGadget : public UnsafeGadget { +class PointerArithmeticGadget : public WarningGadget { const BinaryOperator *PA; // pointer arithmetic expression const Expr * Ptr; // the pointer expression in `PA` public: PointerArithmeticGadget(const MatchFinder::MatchResult &Result) - : UnsafeGadget(Kind::PointerArithmetic), + : WarningGadget(Kind::PointerArithmetic), PA(Result.Nodes.getNodeAs("ptrAdd")), Ptr(Result.Nodes.getNodeAs("ptrAddPtr")) {} @@ -501,10 +504,11 @@ } // namespace /// Scan the function and return a list of gadgets found with provided kits. -static std::pair findGadgets(const Decl *D) { +static std::tuple findGadgets(const Decl *D) { struct GadgetFinderCallback : MatchFinder::MatchCallback { - GadgetList Gadgets; + FixableGadgetList FixableGadgets; + WarningGadgetList WarningGadgets; DeclUseTracker Tracker; void run(const MatchFinder::MatchResult &Result) override { @@ -519,13 +523,20 @@ // Figure out which matcher we've found, and call the appropriate // subclass constructor. // FIXME: Can we do this more logarithmically? -#define GADGET(x) \ +#define FIXABLE_GADGET(x) \ if (Result.Nodes.getNodeAs(#x)) { \ - Gadgets.push_back(std::make_unique(Result)); \ + FixableGadgets.push_back(std::make_unique(Result)); \ return; \ } #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def" -#undef GADGET +#undef FIXABLE_GADGET +#define WARNING_GADGET(x) \ + if (Result.Nodes.getNodeAs(#x)) { \ + WarningGadgets.push_back(std::make_unique(Result)); \ + return; \ + } +#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def" +#undef WARNING_GADGET } }; @@ -537,10 +548,15 @@ stmt(forEveryDescendant( stmt(anyOf( // Add Gadget::matcher() for every gadget in the registry. -#define GADGET(x) \ +#define FIXABLE_GADGET(x) \ x ## Gadget::matcher().bind(#x), #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def" -#undef GADGET +#undef FIXABLE_GADGET +#define WARNING_GADGET(x) \ + x ## Gadget::matcher().bind(#x), +#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def" +#undef WARNING_GADGET + // In parallel, match all DeclRefExprs so that to find out // whether there are any uncovered by gadgets. declRefExpr(hasPointerType(), to(varDecl())).bind("any_dre"), @@ -565,45 +581,51 @@ // Gadgets "claim" variables they're responsible for. Once this loop finishes, // the tracker will only track DREs that weren't claimed by any gadgets, // i.e. not understood by the analysis. - for (const auto &G : CB.Gadgets) { + for (const auto &G : CB.FixableGadgets) { for (const auto *DRE : G->getClaimedVarUseSites()) { CB.Tracker.claimUse(DRE); } } - return {std::move(CB.Gadgets), std::move(CB.Tracker)}; + return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets), std::move(CB.Tracker)}; } void clang::checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler) { - std::cout << "REACHED HERE \n"; assert(D && D->getBody()); SmallSet WarnedDecls; - auto [Gadgets, Tracker] = findGadgets(D); - - DenseMap> Map; - int count = 0; + auto [FixableGadgets, WarningGadgets, Tracker] = findGadgets(D); + DenseMap, + std::vector>> Map; // First, let's sort gadgets by variables. If some gadgets cover more than one // variable, they'll appear more than once in the map. - for (const auto &G : Gadgets) { + for (const auto &G : FixableGadgets) { + DeclUseList DREs = G->getClaimedVarUseSites(); + + // Populate the map. + for (const DeclRefExpr *DRE : DREs) { + if (const auto *VD = dyn_cast(DRE->getDecl())) { + Map[VD].first.push_back(G.get()); + } + } + } + + for (const auto &G : WarningGadgets) { DeclUseList DREs = G->getClaimedVarUseSites(); // Populate the map. bool Pushed = false; for (const DeclRefExpr *DRE : DREs) { if (const auto *VD = dyn_cast(DRE->getDecl())) { - Map[VD].push_back(G.get()); + Map[VD].second.push_back(G.get()); Pushed = true; } } - bool b = !Pushed && !G->isSafe(); - std::cout << "count" << count << " " << b <<"\n"; - count++; - if (!Pushed && !G->isSafe()) { + if (!Pushed) { // We won't return to this gadget later. Emit the warning right away. Handler.handleUnsafeOperation(G->getBaseStmt()); continue; @@ -614,10 +636,11 @@ for (const auto &Item : Map) { const VarDecl *VD = Item.first; - const std::vector &VDGadgets = Item.second; + const std::vector &VDFixableGadgets = Item.second.first; + const std::vector &VDWarningGadgets = Item.second.second; // If the variable has no unsafe gadgets, skip it entirely. - if (!any_of(VDGadgets, [](const Gadget *G) { return !G->isSafe(); })) + if (!any_of(VDWarningGadgets, [](const Gadget *G) { return G->isWarningGadget(); })) continue; Optional Fixes = None; @@ -626,6 +649,7 @@ // as known gadgets. // FIXME: Support parameter variables as well. if (!Tracker.hasUnclaimedUses(VD) && VD->isLocalVarDecl()) { + std::cout << " This should not be entered"; // Choose the appropriate strategy. FIXME: We should try different // strategies. S.set(VD, Strategy::Kind::Span); @@ -637,7 +661,7 @@ // to undo the previous fix first, and then if we can't produce the new // fix for both variables, revert to the old one. Fixes = FixItList{}; - for (const Gadget *G : VDGadgets) { + for (const FixableGadget *G : VDFixableGadgets) { Optional F = G->getFixits(S); if (!F) { Fixes = None; @@ -655,10 +679,8 @@ } else { // The strategy has failed. Emit the warning without the fixit. S.set(VD, Strategy::Kind::Wontfix); - for (const Gadget *G : VDGadgets) { - if (!G->isSafe()) { - Handler.handleUnsafeOperation(G->getBaseStmt()); - } + for (const Gadget *G : VDWarningGadgets) { + Handler.handleUnsafeOperation(G->getBaseStmt()); } } }