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 GADGET +#undef FIXABLE_GADGET +#undef WARNING_GADGET +#undef GADGET \ No newline at end of file Index: clang/lib/Analysis/UnsafeBufferUsage.cpp =================================================================== --- clang/lib/Analysis/UnsafeBufferUsage.cpp +++ clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -9,7 +9,6 @@ #include "clang/Analysis/Analyses/UnsafeBufferUsage.h" #include "clang/AST/RecursiveASTVisitor.h" #include "llvm/ADT/SmallVector.h" -#include using namespace llvm; using namespace clang; @@ -145,9 +144,9 @@ /// rigid AST structure that constitutes an operation on a pointer-type object. /// Discovery of a gadget in the code corresponds to claiming that we understand /// what this part of code is doing well enough to potentially improve it. -/// Gadgets can be unsafe (immediately deserving a warning) or safe (not -/// deserving a warning per se, but affecting our decision-making process -/// nonetheless). +/// Gadgets can be warning (immediately deserving a warning) or fixable (not +/// always deserving a warning per se, but requires our attention to identify +/// it warrants a fixit). class Gadget { public: enum class Kind { @@ -156,20 +155,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,20 +182,13 @@ Kind getKind() const { return K; } - virtual bool isSafe() const = 0; - virtual const Stmt *getBaseStmt() const = 0; + virtual bool isWarningGadget() const = 0; /// Returns the list of pointer-type variables on which this gadget performs /// its operation. Typically there's only one variable. This isn't a list /// 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,43 +196,53 @@ Kind K; }; -using GadgetList = std::vector>; -/// Unsafe gadgets correspond to unsafe code patterns that warrants +/// Warning 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; } + virtual const Stmt *getBaseStmt() const = 0; + }; -/// Safe gadgets correspond to code patterns that aren't unsafe but need to be -/// properly recognized in order to emit correct warnings and fixes over unsafe -/// 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 +/// Fixable gadgets correspond to code patterns that aren't always unsafe but need to be +/// properly recognized in order to emit correct fixes. For example, if a raw pointer-type +/// variable is replaced by a safe C++ container, every use of such variable maust 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 +270,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 +303,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 +336,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 +367,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 +503,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 +522,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)) { \ + FixableGadgets.push_back(std::make_unique(Result)); \ + return; \ + } +#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def" +#undef FIXABLE_GADGET +#define WARNING_GADGET(x) \ if (Result.Nodes.getNodeAs(#x)) { \ - Gadgets.push_back(std::make_unique(Result)); \ + WarningGadgets.push_back(std::make_unique(Result)); \ return; \ } #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def" -#undef GADGET +#undef WARNING_GADGET } }; @@ -537,10 +547,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 +580,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 +635,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 the variable has no warning gadgets, skip it entirely. + if (VDWarningGadgets.empty()) continue; Optional Fixes = None; @@ -637,7 +659,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 +677,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 WarningGadget *G : VDWarningGadgets) { + Handler.handleUnsafeOperation(G->getBaseStmt()); } } }