diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h index aca1ad998822c..5d16dcc824c50 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -42,6 +42,43 @@ class VariableGroupsManager { virtual VarGrpRef getGroupOfParms() const =0; }; +// FixitStrategy is a map from variables to the way we plan to emit fixes for +// these variables. It is figured out gradually by trying different fixes +// for different variables depending on gadgets in which these variables +// participate. +class FixitStrategy { +public: + enum class Kind { + Wontfix, // We don't plan to emit a fixit for this variable. + Span, // We recommend replacing the variable with std::span. + Iterator, // We recommend replacing the variable with std::span::iterator. + Array, // We recommend replacing the variable with std::array. + Vector // We recommend replacing the variable with std::vector. + }; + +private: + using MapTy = llvm::DenseMap; + + MapTy Map; + +public: + FixitStrategy() = default; + FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies. + FixitStrategy &operator=(const FixitStrategy &) = delete; + FixitStrategy(FixitStrategy &&) = default; + FixitStrategy &operator=(FixitStrategy &&) = default; + + void set(const VarDecl *VD, Kind K) { Map[VD] = K; } + + Kind lookup(const VarDecl *VD) const { + auto I = Map.find(VD); + if (I == Map.end()) + return Kind::Wontfix; + + return I->second; + } +}; + /// The interface that lets the caller handle unsafe buffer usage analysis /// results by overriding this class's handle... methods. class UnsafeBufferUsageHandler { @@ -75,9 +112,11 @@ class UnsafeBufferUsageHandler { /// /// `D` is the declaration of the callable under analysis that owns `Variable` /// and all of its group mates. - virtual void handleUnsafeVariableGroup(const VarDecl *Variable, - const VariableGroupsManager &VarGrpMgr, - FixItList &&Fixes, const Decl *D) = 0; + virtual void + handleUnsafeVariableGroup(const VarDecl *Variable, + const VariableGroupsManager &VarGrpMgr, + FixItList &&Fixes, const Decl *D, + const FixitStrategy &VarTargetTypes) = 0; #ifndef NDEBUG public: diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 1c0ebbe4e6834..b423ef1be9425 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -12113,9 +12113,9 @@ def warn_unsafe_buffer_operation : Warning< def note_unsafe_buffer_operation : Note< "used%select{| in pointer arithmetic| in buffer access}0 here">; def note_unsafe_buffer_variable_fixit_group : Note< - "change type of %0 to '%select{std::span|std::array|std::span::iterator}1' to preserve bounds information%select{|, and change %2 to '%select{std::span|std::array|std::span::iterator}1' to propagate bounds information between them}3">; + "change type of %0 to '%select{std::span' to preserve bounds information|std::array' to label it for hardening|std::span::iterator' to preserve bounds information}1%select{|, and change %2 to '%select{std::span|std::array|std::span::iterator}1' to propagate bounds information between them}3">; def note_unsafe_buffer_variable_fixit_together : Note< - "change type of %0 to '%select{std::span|std::array|std::span::iterator}1' to preserve bounds information" + "change type of %0 to '%select{std::span' to preserve bounds information|std::array' to label it for hardening|std::span::iterator' to preserve bounds information}1" "%select{|, and change %2 to safe types to make function %4 bounds-safe}3">; def note_safe_buffer_usage_suggestions_disabled : Note< "pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions">; diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 823cd2a7b9969..be6f77c9ddfdb 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -12,10 +12,14 @@ #include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/StmtVisitor.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Basic/CharInfo.h" +#include "clang/Basic/SourceLocation.h" #include "clang/Lex/Lexer.h" #include "clang/Lex/Preprocessor.h" #include "llvm/ADT/APSInt.h" #include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/Casting.h" #include #include #include @@ -407,9 +411,6 @@ using DeclUseList = SmallVector; // Convenience typedef. using FixItList = SmallVector; - -// Defined below. -class Strategy; } // namespace namespace { @@ -486,7 +487,7 @@ class FixableGadget : public Gadget { /// Returns a fixit that would fix the current gadget according to /// the current strategy. Returns std::nullopt if the fix cannot be produced; /// returns an empty list if no fixes are necessary. - virtual std::optional getFixits(const Strategy &) const { + virtual std::optional getFixits(const FixitStrategy &) const { return std::nullopt; } @@ -737,7 +738,8 @@ class PointerInitGadget : public FixableGadget { return stmt(PtrInitStmt); } - virtual std::optional getFixits(const Strategy &S) const override; + virtual std::optional + getFixits(const FixitStrategy &S) const override; virtual const Stmt *getBaseStmt() const override { // FIXME: This needs to be the entire DeclStmt, assuming that this method @@ -789,7 +791,8 @@ class PointerAssignmentGadget : public FixableGadget { return stmt(isInUnspecifiedUntypedContext(PtrAssignExpr)); } - virtual std::optional getFixits(const Strategy &S) const override; + virtual std::optional + getFixits(const FixitStrategy &S) const override; virtual const Stmt *getBaseStmt() const override { // FIXME: This should be the binary operator, assuming that this method @@ -892,7 +895,8 @@ class ULCArraySubscriptGadget : public FixableGadget { return expr(isInUnspecifiedLvalueContext(Target)); } - virtual std::optional getFixits(const Strategy &S) const override; + virtual std::optional + getFixits(const FixitStrategy &S) const override; virtual const Stmt *getBaseStmt() const override { return Node; } @@ -932,7 +936,8 @@ class UPCStandalonePointerGadget : public FixableGadget { return stmt(isInUnspecifiedPointerContext(target)); } - virtual std::optional getFixits(const Strategy &S) const override; + virtual std::optional + getFixits(const FixitStrategy &S) const override; virtual const Stmt *getBaseStmt() const override { return Node; } @@ -976,7 +981,8 @@ class PointerDereferenceGadget : public FixableGadget { virtual const Stmt *getBaseStmt() const final { return Op; } - virtual std::optional getFixits(const Strategy &S) const override; + virtual std::optional + getFixits(const FixitStrategy &S) const override; }; // Represents expressions of the form `&DRE[any]` in the Unspecified Pointer @@ -1009,7 +1015,8 @@ class UPCAddressofArraySubscriptGadget : public FixableGadget { .bind(UPCAddressofArraySubscriptTag))))); } - virtual std::optional getFixits(const Strategy &) const override; + virtual std::optional + getFixits(const FixitStrategy &) const override; virtual const Stmt *getBaseStmt() const override { return Node; } @@ -1088,46 +1095,6 @@ class DeclUseTracker { }; } // namespace -namespace { -// Strategy is a map from variables to the way we plan to emit fixes for -// these variables. It is figured out gradually by trying different fixes -// for different variables depending on gadgets in which these variables -// participate. -class Strategy { -public: - enum class Kind { - Wontfix, // We don't plan to emit a fixit for this variable. - Span, // We recommend replacing the variable with std::span. - Iterator, // We recommend replacing the variable with std::span::iterator. - Array, // We recommend replacing the variable with std::array. - Vector // We recommend replacing the variable with std::vector. - }; - -private: - using MapTy = llvm::DenseMap; - - MapTy Map; - -public: - Strategy() = default; - Strategy(const Strategy &) = delete; // Let's avoid copies. - Strategy &operator=(const Strategy &) = delete; - Strategy(Strategy &&) = default; - Strategy &operator=(Strategy &&) = default; - - void set(const VarDecl *VD, Kind K) { Map[VD] = K; } - - Kind lookup(const VarDecl *VD) const { - auto I = Map.find(VD); - if (I == Map.end()) - return Kind::Wontfix; - - return I->second; - } -}; -} // namespace - - // Representing a pointer type expression of the form `++Ptr` in an Unspecified // Pointer Context (UPC): class UPCPreIncrementGadget : public FixableGadget { @@ -1159,7 +1126,8 @@ class UPCPreIncrementGadget : public FixableGadget { ).bind(UPCPreIncrementTag))))); } - virtual std::optional getFixits(const Strategy &S) const override; + virtual std::optional + getFixits(const FixitStrategy &S) const override; virtual const Stmt *getBaseStmt() const override { return Node; } @@ -1204,7 +1172,8 @@ class UUCAddAssignGadget : public FixableGadget { // clang-format on } - virtual std::optional getFixits(const Strategy &S) const override; + virtual std::optional + getFixits(const FixitStrategy &S) const override; virtual const Stmt *getBaseStmt() const override { return Node; } @@ -1254,7 +1223,8 @@ class DerefSimplePtrArithFixableGadget : public FixableGadget { // clang-format on } - virtual std::optional getFixits(const Strategy &s) const final; + virtual std::optional + getFixits(const FixitStrategy &s) const final; // TODO remove this method from FixableGadget interface virtual const Stmt *getBaseStmt() const final { return nullptr; } @@ -1464,38 +1434,40 @@ bool clang::internal::anyConflict(const SmallVectorImpl &FixIts, } std::optional -PointerAssignmentGadget::getFixits(const Strategy &S) const { +PointerAssignmentGadget::getFixits(const FixitStrategy &S) const { const auto *LeftVD = cast(PtrLHS->getDecl()); const auto *RightVD = cast(PtrRHS->getDecl()); switch (S.lookup(LeftVD)) { - case Strategy::Kind::Span: - if (S.lookup(RightVD) == Strategy::Kind::Span) - return FixItList{}; - return std::nullopt; - case Strategy::Kind::Wontfix: - return std::nullopt; - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: - llvm_unreachable("unsupported strategies for FixableGadgets"); + case FixitStrategy::Kind::Span: + if (S.lookup(RightVD) == FixitStrategy::Kind::Span) + return FixItList{}; + return std::nullopt; + case FixitStrategy::Kind::Wontfix: + return std::nullopt; + case FixitStrategy::Kind::Iterator: + case FixitStrategy::Kind::Array: + return std::nullopt; + case FixitStrategy::Kind::Vector: + llvm_unreachable("unsupported strategies for FixableGadgets"); } return std::nullopt; } std::optional -PointerInitGadget::getFixits(const Strategy &S) const { +PointerInitGadget::getFixits(const FixitStrategy &S) const { const auto *LeftVD = PtrInitLHS; const auto *RightVD = cast(PtrInitRHS->getDecl()); switch (S.lookup(LeftVD)) { - case Strategy::Kind::Span: - if (S.lookup(RightVD) == Strategy::Kind::Span) - return FixItList{}; - return std::nullopt; - case Strategy::Kind::Wontfix: - return std::nullopt; - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: + case FixitStrategy::Kind::Span: + if (S.lookup(RightVD) == FixitStrategy::Kind::Span) + return FixItList{}; + return std::nullopt; + case FixitStrategy::Kind::Wontfix: + return std::nullopt; + case FixitStrategy::Kind::Iterator: + case FixitStrategy::Kind::Array: + return std::nullopt; + case FixitStrategy::Kind::Vector: llvm_unreachable("unsupported strategies for FixableGadgets"); } return std::nullopt; @@ -1512,12 +1484,12 @@ static bool isNonNegativeIntegerExpr(const Expr *Expr, const VarDecl *VD, } std::optional -ULCArraySubscriptGadget::getFixits(const Strategy &S) const { +ULCArraySubscriptGadget::getFixits(const FixitStrategy &S) const { if (const auto *DRE = dyn_cast(Node->getBase()->IgnoreImpCasts())) if (const auto *VD = dyn_cast(DRE->getDecl())) { switch (S.lookup(VD)) { - case Strategy::Kind::Span: { + case FixitStrategy::Kind::Span: { // If the index has a negative constant value, we give up as no valid // fix-it can be generated: @@ -1528,10 +1500,11 @@ ULCArraySubscriptGadget::getFixits(const Strategy &S) const { // no-op is a good fix-it, otherwise return FixItList{}; } - case Strategy::Kind::Wontfix: - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: + case FixitStrategy::Kind::Array: + return FixItList{}; + case FixitStrategy::Kind::Wontfix: + case FixitStrategy::Kind::Iterator: + case FixitStrategy::Kind::Vector: llvm_unreachable("unsupported strategies for FixableGadgets"); } } @@ -1542,17 +1515,18 @@ static std::optional // forward declaration fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node); std::optional -UPCAddressofArraySubscriptGadget::getFixits(const Strategy &S) const { +UPCAddressofArraySubscriptGadget::getFixits(const FixitStrategy &S) const { auto DREs = getClaimedVarUseSites(); const auto *VD = cast(DREs.front()->getDecl()); switch (S.lookup(VD)) { - case Strategy::Kind::Span: + case FixitStrategy::Kind::Span: return fixUPCAddressofArraySubscriptWithSpan(Node); - case Strategy::Kind::Wontfix: - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: + case FixitStrategy::Kind::Wontfix: + case FixitStrategy::Kind::Iterator: + case FixitStrategy::Kind::Array: + return std::nullopt; + case FixitStrategy::Kind::Vector: llvm_unreachable("unsupported strategies for FixableGadgets"); } return std::nullopt; // something went wrong, no fix-it @@ -1803,10 +1777,10 @@ getSpanTypeText(StringRef EltTyText, } std::optional -DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const { +DerefSimplePtrArithFixableGadget::getFixits(const FixitStrategy &s) const { const VarDecl *VD = dyn_cast(BaseDeclRefExpr->getDecl()); - if (VD && s.lookup(VD) == Strategy::Kind::Span) { + if (VD && s.lookup(VD) == FixitStrategy::Kind::Span) { ASTContext &Ctx = VD->getASTContext(); // std::span can't represent elements before its begin() if (auto ConstVal = Offset->getIntegerConstantExpr(Ctx)) @@ -1866,10 +1840,10 @@ DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const { } std::optional -PointerDereferenceGadget::getFixits(const Strategy &S) const { +PointerDereferenceGadget::getFixits(const FixitStrategy &S) const { const VarDecl *VD = cast(BaseDeclRefExpr->getDecl()); switch (S.lookup(VD)) { - case Strategy::Kind::Span: { + case FixitStrategy::Kind::Span: { ASTContext &Ctx = VD->getASTContext(); SourceManager &SM = Ctx.getSourceManager(); // Required changes: *(ptr); => (ptr[0]); and *ptr; => ptr[0] @@ -1884,11 +1858,12 @@ PointerDereferenceGadget::getFixits(const Strategy &S) const { } break; } - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: - llvm_unreachable("Strategy not implemented yet!"); - case Strategy::Kind::Wontfix: + case FixitStrategy::Kind::Iterator: + case FixitStrategy::Kind::Array: + return std::nullopt; + case FixitStrategy::Kind::Vector: + llvm_unreachable("FixitStrategy not implemented yet!"); + case FixitStrategy::Kind::Wontfix: llvm_unreachable("Invalid strategy!"); } @@ -1897,28 +1872,28 @@ PointerDereferenceGadget::getFixits(const Strategy &S) const { // Generates fix-its replacing an expression of the form UPC(DRE) with // `DRE.data()` -std::optional UPCStandalonePointerGadget::getFixits(const Strategy &S) - const { +std::optional +UPCStandalonePointerGadget::getFixits(const FixitStrategy &S) const { const auto VD = cast(Node->getDecl()); switch (S.lookup(VD)) { - case Strategy::Kind::Span: { - ASTContext &Ctx = VD->getASTContext(); - SourceManager &SM = Ctx.getSourceManager(); - // Inserts the .data() after the DRE - std::optional EndOfOperand = - getPastLoc(Node, SM, Ctx.getLangOpts()); - - if (EndOfOperand) - return FixItList{{FixItHint::CreateInsertion( - *EndOfOperand, ".data()")}}; - // FIXME: Points inside a macro expansion. - break; - } - case Strategy::Kind::Wontfix: - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: - llvm_unreachable("unsupported strategies for FixableGadgets"); + case FixitStrategy::Kind::Span: { + ASTContext &Ctx = VD->getASTContext(); + SourceManager &SM = Ctx.getSourceManager(); + // Inserts the .data() after the DRE + std::optional EndOfOperand = + getPastLoc(Node, SM, Ctx.getLangOpts()); + + if (EndOfOperand) + return FixItList{{FixItHint::CreateInsertion(*EndOfOperand, ".data()")}}; + // FIXME: Points inside a macro expansion. + break; + } + case FixitStrategy::Kind::Wontfix: + case FixitStrategy::Kind::Iterator: + case FixitStrategy::Kind::Array: + return std::nullopt; + case FixitStrategy::Kind::Vector: + llvm_unreachable("unsupported strategies for FixableGadgets"); } return std::nullopt; @@ -1962,14 +1937,14 @@ fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node) { } std::optional -UUCAddAssignGadget::getFixits(const Strategy &S) const { +UUCAddAssignGadget::getFixits(const FixitStrategy &S) const { DeclUseList DREs = getClaimedVarUseSites(); if (DREs.size() != 1) return std::nullopt; // In cases of `Ptr += n` where `Ptr` is not a DRE, we // give up if (const VarDecl *VD = dyn_cast(DREs.front()->getDecl())) { - if (S.lookup(VD) == Strategy::Kind::Span) { + if (S.lookup(VD) == FixitStrategy::Kind::Span) { FixItList Fixes; const Stmt *AddAssignNode = getBaseStmt(); @@ -2003,14 +1978,15 @@ UUCAddAssignGadget::getFixits(const Strategy &S) const { return std::nullopt; // Not in the cases that we can handle for now, give up. } -std::optional UPCPreIncrementGadget::getFixits(const Strategy &S) const { +std::optional +UPCPreIncrementGadget::getFixits(const FixitStrategy &S) const { DeclUseList DREs = getClaimedVarUseSites(); if (DREs.size() != 1) return std::nullopt; // In cases of `++Ptr` where `Ptr` is not a DRE, we // give up if (const VarDecl *VD = dyn_cast(DREs.front()->getDecl())) { - if (S.lookup(VD) == Strategy::Kind::Span) { + if (S.lookup(VD) == FixitStrategy::Kind::Span) { FixItList Fixes; std::stringstream SS; const Stmt *PreIncNode = getBaseStmt(); @@ -2033,7 +2009,6 @@ std::optional UPCPreIncrementGadget::getFixits(const Strategy &S) con return std::nullopt; // Not in the cases that we can handle for now, give up. } - // For a non-null initializer `Init` of `T *` type, this function returns // `FixItHint`s producing a list initializer `{Init, S}` as a part of a fix-it // to output stream. @@ -2261,7 +2236,7 @@ static bool hasConflictingOverload(const FunctionDecl *FD) { // } // static std::optional -createOverloadsForFixedParams(const Strategy &S, const FunctionDecl *FD, +createOverloadsForFixedParams(const FixitStrategy &S, const FunctionDecl *FD, const ASTContext &Ctx, UnsafeBufferUsageHandler &Handler) { // FIXME: need to make this conflict checking better: @@ -2278,9 +2253,9 @@ createOverloadsForFixedParams(const Strategy &S, const FunctionDecl *FD, for (unsigned i = 0; i < NumParms; i++) { const ParmVarDecl *PVD = FD->getParamDecl(i); - if (S.lookup(PVD) == Strategy::Kind::Wontfix) + if (S.lookup(PVD) == FixitStrategy::Kind::Wontfix) continue; - if (S.lookup(PVD) != Strategy::Kind::Span) + if (S.lookup(PVD) != FixitStrategy::Kind::Span) // Not supported, not suppose to happen: return std::nullopt; @@ -2291,7 +2266,8 @@ createOverloadsForFixedParams(const Strategy &S, const FunctionDecl *FD, if (!PteTyText) // something wrong in obtaining the text of the pointee type, give up return std::nullopt; - // FIXME: whether we should create std::span type depends on the Strategy. + // FIXME: whether we should create std::span type depends on the + // FixitStrategy. NewTysTexts[i] = getSpanTypeText(*PteTyText, PteTyQuals); ParmsMask[i] = true; AtLeastOneParmToFix = true; @@ -2495,10 +2471,103 @@ static FixItList fixVariableWithSpan(const VarDecl *VD, return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler); } +static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx, + UnsafeBufferUsageHandler &Handler) { + FixItList FixIts{}; + + // Note: the code below expects the declaration to not use any type sugar like + // typedef. + if (auto CAT = dyn_cast(D->getType())) { + const QualType &ArrayEltT = CAT->getElementType(); + assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!"); + // FIXME: support multi-dimensional arrays + if (isa(ArrayEltT.getCanonicalType())) + return {}; + + const SourceLocation IdentifierLoc = getVarDeclIdentifierLoc(D); + + // Get the spelling of the element type as written in the source file + // (including macros, etc.). + auto MaybeElemTypeTxt = + getRangeText({D->getBeginLoc(), IdentifierLoc}, Ctx.getSourceManager(), + Ctx.getLangOpts()); + if (!MaybeElemTypeTxt) + return {}; + const llvm::StringRef ElemTypeTxt = MaybeElemTypeTxt->trim(); + + // Find the '[' token. + std::optional NextTok = Lexer::findNextToken( + IdentifierLoc, Ctx.getSourceManager(), Ctx.getLangOpts()); + while (NextTok && !NextTok->is(tok::l_square) && + NextTok->getLocation() <= D->getSourceRange().getEnd()) + NextTok = Lexer::findNextToken(NextTok->getLocation(), + Ctx.getSourceManager(), Ctx.getLangOpts()); + if (!NextTok) + return {}; + const SourceLocation LSqBracketLoc = NextTok->getLocation(); + + // Get the spelling of the array size as written in the source file + // (including macros, etc.). + auto MaybeArraySizeTxt = getRangeText( + {LSqBracketLoc.getLocWithOffset(1), D->getTypeSpecEndLoc()}, + Ctx.getSourceManager(), Ctx.getLangOpts()); + if (!MaybeArraySizeTxt) + return {}; + const llvm::StringRef ArraySizeTxt = MaybeArraySizeTxt->trim(); + if (ArraySizeTxt.empty()) { + // FIXME: Support array size getting determined from the initializer. + // Examples: + // int arr1[] = {0, 1, 2}; + // int arr2{3, 4, 5}; + // We might be able to preserve the non-specified size with `auto` and + // `std::to_array`: + // auto arr1 = std::to_array({0, 1, 2}); + return {}; + } + + std::optional IdentText = + getVarDeclIdentifierText(D, Ctx.getSourceManager(), Ctx.getLangOpts()); + + if (!IdentText) { + DEBUG_NOTE_DECL_FAIL(D, " : failed to locate the identifier"); + return {}; + } + + SmallString<32> Replacement; + raw_svector_ostream OS(Replacement); + OS << "std::array<" << ElemTypeTxt << ", " << ArraySizeTxt << "> " + << IdentText->str(); + + FixIts.push_back(FixItHint::CreateReplacement( + SourceRange{D->getBeginLoc(), D->getTypeSpecEndLoc()}, OS.str())); + } + + return FixIts; +} + +static FixItList fixVariableWithArray(const VarDecl *VD, + const DeclUseTracker &Tracker, + const ASTContext &Ctx, + UnsafeBufferUsageHandler &Handler) { + const DeclStmt *DS = Tracker.lookupDecl(VD); + assert(DS && "Fixing non-local variables not implemented yet!"); + if (!DS->isSingleDecl()) { + // FIXME: to support handling multiple `VarDecl`s in a single `DeclStmt` + return {}; + } + // Currently DS is an unused variable but we'll need it when + // non-single decls are implemented, where the pointee type name + // and the '*' are spread around the place. + (void)DS; + + // FIXME: handle cases where DS has multiple declarations + return fixVarDeclWithArray(VD, Ctx, Handler); +} + // TODO: we should be consistent to use `std::nullopt` to represent no-fix due // to any unexpected problem. static FixItList -fixVariable(const VarDecl *VD, Strategy::Kind K, +fixVariable(const VarDecl *VD, FixitStrategy::Kind K, /* The function decl under analysis */ const Decl *D, const DeclUseTracker &Tracker, ASTContext &Ctx, UnsafeBufferUsageHandler &Handler) { @@ -2529,7 +2598,7 @@ fixVariable(const VarDecl *VD, Strategy::Kind K, } switch (K) { - case Strategy::Kind::Span: { + case FixitStrategy::Kind::Span: { if (VD->getType()->isPointerType()) { if (const auto *PVD = dyn_cast(VD)) return fixParamWithSpan(PVD, Ctx, Handler); @@ -2540,11 +2609,18 @@ fixVariable(const VarDecl *VD, Strategy::Kind K, DEBUG_NOTE_DECL_FAIL(VD, " : not a pointer"); return {}; } - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: - llvm_unreachable("Strategy not implemented yet!"); - case Strategy::Kind::Wontfix: + case FixitStrategy::Kind::Array: { + if (VD->isLocalVarDecl() && + isa(VD->getType().getCanonicalType())) + return fixVariableWithArray(VD, Tracker, Ctx, Handler); + + DEBUG_NOTE_DECL_FAIL(VD, " : not a local const-size array"); + return {}; + } + case FixitStrategy::Kind::Iterator: + case FixitStrategy::Kind::Vector: + llvm_unreachable("FixitStrategy not implemented yet!"); + case FixitStrategy::Kind::Wontfix: llvm_unreachable("Invalid strategy!"); } llvm_unreachable("Unknown strategy!"); @@ -2605,7 +2681,8 @@ static void eraseVarsForUnfixableGroupMates( static FixItList createFunctionOverloadsForParms( std::map &FixItsForVariable /* mutable */, const VariableGroupsManager &VarGrpMgr, const FunctionDecl *FD, - const Strategy &S, ASTContext &Ctx, UnsafeBufferUsageHandler &Handler) { + const FixitStrategy &S, ASTContext &Ctx, + UnsafeBufferUsageHandler &Handler) { FixItList FixItsSharedByParms{}; std::optional OverloadFixes = @@ -2625,8 +2702,8 @@ static FixItList createFunctionOverloadsForParms( // Constructs self-contained fix-its for each variable in `FixablesForAllVars`. static std::map -getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S, - ASTContext &Ctx, +getFixIts(FixableGadgetSets &FixablesForAllVars, const FixitStrategy &S, + ASTContext &Ctx, /* The function decl under analysis */ const Decl *D, const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler, const VariableGroupsManager &VarGrpMgr) { @@ -2724,11 +2801,14 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S, } template -static Strategy +static FixitStrategy getNaiveStrategy(llvm::iterator_range UnsafeVars) { - Strategy S; + FixitStrategy S; for (const VarDecl *VD : UnsafeVars) { - S.set(VD, Strategy::Kind::Span); + if (isa(VD->getType().getCanonicalType())) + S.set(VD, FixitStrategy::Kind::Array); + else + S.set(VD, FixitStrategy::Kind::Span); } return S; } @@ -3027,7 +3107,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D, // We assign strategies to variables that are 1) in the graph and 2) can be // fixed. Other variables have the default "Won't fix" strategy. - Strategy NaiveStrategy = getNaiveStrategy(llvm::make_filter_range( + FixitStrategy NaiveStrategy = getNaiveStrategy(llvm::make_filter_range( VisitedVars, [&FixablesForAllVars](const VarDecl *V) { // If a warned variable has no "Fixable", it is considered unfixable: return FixablesForAllVars.byVar.count(V); @@ -3050,9 +3130,9 @@ void clang::checkUnsafeBufferUsage(const Decl *D, auto FixItsIt = FixItsForVariableGroup.find(VD); Handler.handleUnsafeVariableGroup(VD, VarGrpMgr, FixItsIt != FixItsForVariableGroup.end() - ? std::move(FixItsIt->second) - : FixItList{}, - D); + ? std::move(FixItsIt->second) + : FixItList{}, + D, NaiveStrategy); for (const auto &G : WarningGadgets) { Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true, D->getASTContext()); diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 78b9f324e1390..8239ba49429d3 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2297,7 +2297,8 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { void handleUnsafeVariableGroup(const VarDecl *Variable, const VariableGroupsManager &VarGrpMgr, - FixItList &&Fixes, const Decl *D) override { + FixItList &&Fixes, const Decl *D, + const FixitStrategy &VarTargetTypes) override { assert(!SuggestSuggestions && "Unsafe buffer usage fixits displayed without suggestions!"); S.Diag(Variable->getLocation(), diag::warn_unsafe_buffer_variable) @@ -2312,7 +2313,18 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { // NOT explain how the variables are grouped as the reason is non-trivial // and irrelavant to users' experience: const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(Variable, &BriefMsg); - unsigned FixItStrategy = 0; // For now we only have 'std::span' strategy + unsigned FixItStrategy = 0; + switch (VarTargetTypes.lookup(Variable)) { + case clang::FixitStrategy::Kind::Span: + FixItStrategy = 0; + break; + case clang::FixitStrategy::Kind::Array: + FixItStrategy = 1; + break; + default: + assert(false && "We support only std::span and std::array"); + }; + const auto &FD = S.Diag(Variable->getLocation(), BriefMsg ? diag::note_unsafe_buffer_variable_fixit_together diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp new file mode 100644 index 0000000000000..90c11b1be95c2 --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp @@ -0,0 +1,24 @@ +// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \ +// RUN: -fsafe-buffer-usage-suggestions \ +// RUN: -verify %s + +// CHECK-NOT: [-Wunsafe-buffer-usage] + + +void foo(unsigned idx) { + int buffer[10]; // expected-warning{{'buffer' is an unsafe buffer that does not perform bounds checks}} + // expected-note@-1{{change type of 'buffer' to 'std::array' to label it for hardening}} + buffer[idx] = 0; // expected-note{{used in buffer access here}} +} + +int global_buffer[10]; // expected-warning{{'global_buffer' is an unsafe buffer that does not perform bounds checks}} +void foo2(unsigned idx) { + global_buffer[idx] = 0; // expected-note{{used in buffer access here}} +} + +struct Foo { + int member_buffer[10]; +}; +void foo2(Foo& f, unsigned idx) { + f.member_buffer[idx] = 0; // expected-warning{{unsafe buffer access}} +} diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp index e08f70d97e391..3a360566d1f3a 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp @@ -32,15 +32,6 @@ void foo() { // debug-note{{safe buffers debug: gadget 'ULCArraySubscript' refused to produce a fix}} } -void failed_decl() { - int a[10]; // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}} \ - // debug-note{{safe buffers debug: failed to produce fixit for declaration 'a' : not a pointer}} - - for (int i = 0; i < 10; i++) { - a[i] = i; // expected-note{{used in buffer access here}} - } -} - void failed_multiple_decl() { int *a = new int[4], b; // expected-warning{{'a' is an unsafe pointer used for buffer access}} \ // debug-note{{safe buffers debug: failed to produce fixit for declaration 'a' : multiple VarDecls}} diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp new file mode 100644 index 0000000000000..3adfc324dfbe3 --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp @@ -0,0 +1,228 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \ +// RUN: -fsafe-buffer-usage-suggestions \ +// RUN: -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s +typedef int * Int_ptr_t; +typedef int Int_t; + +void simple(unsigned idx) { + int buffer[10]; +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array buffer" + buffer[idx] = 0; +} + +void array2d(unsigned idx) { + int buffer[10][10]; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]] + buffer[idx][idx] = 0; +} + +void array2d_vla(unsigned sz, unsigned idx) { + int buffer1[10][sz]; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]] + int buffer2[sz][10]; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]] + buffer1[idx][idx] = 0; + buffer2[idx][idx] = 0; +} + +void array2d_assign_from_elem(unsigned idx) { + int buffer[10][10]; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]] + int a = buffer[idx][idx]; +} + +void array2d_use(int *); +void array2d_call(unsigned idx) { + int buffer[10][10]; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]] + array2d_use(buffer[idx]); +} +void array2d_call_vla(unsigned sz, unsigned idx) { + int buffer[10][sz]; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]] + array2d_use(buffer[idx]); +} + +void array2d_typedef(unsigned idx) { + typedef int ten_ints_t[10]; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]] + ten_ints_t buffer[10]; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]] + buffer[idx][idx] = 0; +} + +void whitespace_in_declaration(unsigned idx) { + int buffer_w [ 10 ]; +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:35}:"std::array buffer_w" + buffer_w[idx] = 0; +} + +void comments_in_declaration(unsigned idx) { + int /* [A] */ buffer_w /* [B] */ [ /* [C] */ 10 /* [D] */ ] ; +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:69}:"std::array buffer_w" + buffer_w[idx] = 0; +} + +void initializer(unsigned idx) { + int buffer[3] = {0}; +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::array buffer" + + int buffer2[3] = {0, 1, 2}; +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array buffer2" + + buffer[idx] = 0; + buffer2[idx] = 0; +} + +void auto_size(unsigned idx) { + int buffer[] = {0, 1, 2}; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]] +// FIXME: implement support + + buffer[idx] = 0; +} + +void universal_initialization(unsigned idx) { + int buffer[] {0, 1, 2}; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]] +// FIXME: implement support + + buffer[idx] = 0; +} + +void multi_decl1(unsigned idx) { + int a, buffer[10]; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]] +// FIXME: implement support + + buffer[idx] = 0; +} + +void multi_decl2(unsigned idx) { + int buffer[10], b; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]] +// FIXME: implement support + + buffer[idx] = 0; +} + +void local_array_ptr_to_const(unsigned idx, const int*& a) { + const int * buffer[10] = {a}; +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:25}:"std::array buffer" + a = buffer[idx]; +} + +void local_array_const_ptr(unsigned idx, int*& a) { + int * const buffer[10] = {a}; +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:25}:"std::array buffer" + + a = buffer[idx]; +} + +void local_array_const_ptr_via_typedef(unsigned idx, int*& a) { + typedef int * const my_const_ptr; + my_const_ptr buffer[10] = {a}; +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:26}:"std::array buffer" + + a = buffer[idx]; +} + +void local_array_const_ptr_to_const(unsigned idx, const int*& a) { + const int * const buffer[10] = {a}; +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:31}:"std::array buffer" + + a = buffer[idx]; + +} + +template +void unsupported_local_array_in_template(unsigned idx) { + T buffer[10]; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*} + buffer[idx] = 0; +} +// Instantiate the template function to force its analysis. +template void unsupported_local_array_in_template(unsigned); + +typedef unsigned int my_uint; +void typedef_as_elem_type(unsigned idx) { + my_uint buffer[10]; +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:21}:"std::array buffer" + buffer[idx] = 0; +} + +void decltype_as_elem_type(unsigned idx) { + int a; + decltype(a) buffer[10]; +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:25}:"std::array buffer" + buffer[idx] = 0; +} + +void macro_as_elem_type(unsigned idx) { +#define MY_INT int + MY_INT buffer[10]; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*} +// FIXME: implement support + + buffer[idx] = 0; +#undef MY_INT +} + +void macro_as_identifier(unsigned idx) { +#define MY_BUFFER buffer + int MY_BUFFER[10]; +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:20}:"std::array MY_BUFFER" + MY_BUFFER[idx] = 0; +#undef MY_BUFFER +} + +void macro_as_size(unsigned idx) { +#define MY_TEN 10 + int buffer[MY_TEN]; +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:21}:"std::array buffer" + buffer[idx] = 0; +#undef MY_TEN +} + +typedef unsigned int my_array[42]; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*} +void typedef_as_array_type(unsigned idx) { + my_array buffer; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*} + buffer[idx] = 0; +} + +void decltype_as_array_type(unsigned idx) { + int buffer[42]; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*} + decltype(buffer) buffer2; +// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*} + buffer2[idx] = 0; +} + +void constant_as_size(unsigned idx) { + const unsigned my_const = 10; + int buffer[my_const]; +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:23}:"std::array buffer" + buffer[idx] = 0; +} + +void subscript_negative() { + int buffer[10]; +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array buffer" + + // For constant-size arrays any negative index will lead to buffer underflow. + // std::array::operator[] has unsigned parameter so the value will be casted to unsigned. + // This will very likely be buffer overflow but hardened std::array catch these at runtime. + buffer[-5] = 0; +} + +void subscript_signed(int signed_idx) { + int buffer[10]; +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array buffer" + + // For constant-size arrays any negative index will lead to buffer underflow. + // std::array::operator[] has unsigned parameter so the value will be casted to unsigned. + // This will very likely be buffer overflow but hardened std::array catches these at runtime. + buffer[signed_idx] = 0; +} diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp index c5f0a9ef92937..67cdf252d6a8b 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp @@ -61,6 +61,7 @@ void testArraySubscripts(int *p, int **pp) { ); int a[10]; // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}} + // expected-note@-1{{change type of 'a' to 'std::array' to label it for hardening}} int b[10][10]; // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}} foo(a[1], 1[a], // expected-note2{{used in buffer access here}} @@ -174,6 +175,7 @@ auto file_scope_lambda = [](int *ptr) { void testLambdaCapture() { int a[10]; // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}} int b[10]; // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}} + // expected-note@-1{{change type of 'b' to 'std::array' to label it for hardening}} int c[10]; auto Lam1 = [a]() { @@ -191,7 +193,9 @@ void testLambdaCapture() { void testLambdaImplicitCapture() { int a[10]; // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}} + // expected-note@-1{{change type of 'a' to 'std::array' to label it for hardening}} int b[10]; // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}} + // expected-note@-1{{change type of 'b' to 'std::array' to label it for hardening}} auto Lam1 = [=]() { return a[1]; // expected-note{{used in buffer access here}} @@ -344,6 +348,7 @@ template void fArr(T t[]) { // expected-warning@-1{{'t' is an unsafe pointer used for buffer access}} foo(t[1]); // expected-note{{used in buffer access here}} T ar[8]; // expected-warning{{'ar' is an unsafe buffer that does not perform bounds checks}} + // expected-note@-1{{change type of 'ar' to 'std::array' to label it for hardening}} foo(ar[5]); // expected-note{{used in buffer access here}} }