diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h index c2bd6c50f41ef..d195e162b7110 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 dab89da790f24..687b13f520a0e 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -11966,9 +11966,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 5e7b5aba49d0a..579e36c42ce90 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 { @@ -487,7 +488,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; } @@ -736,7 +737,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 @@ -787,7 +789,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 @@ -889,7 +892,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; } @@ -929,7 +933,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; } @@ -971,7 +976,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 @@ -1004,7 +1010,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; } @@ -1083,45 +1090,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 { @@ -1152,7 +1120,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; } @@ -1197,7 +1166,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; } @@ -1247,7 +1217,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; } @@ -1457,37 +1428,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) + case FixitStrategy::Kind::Span: + if (S.lookup(RightVD) == FixitStrategy::Kind::Span) return FixItList{}; return std::nullopt; - case Strategy::Kind::Wontfix: + case FixitStrategy::Kind::Wontfix: return std::nullopt; - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: + 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 { +std::optional +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) + case FixitStrategy::Kind::Span: + if (S.lookup(RightVD) == FixitStrategy::Kind::Span) return FixItList{}; return std::nullopt; - case Strategy::Kind::Wontfix: + case FixitStrategy::Kind::Wontfix: return std::nullopt; - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: + case FixitStrategy::Kind::Iterator: + case FixitStrategy::Kind::Array: + return std::nullopt; + case FixitStrategy::Kind::Vector: llvm_unreachable("unsupported strategies for FixableGadgets"); } return std::nullopt; @@ -1504,12 +1478,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: @@ -1520,10 +1494,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"); } } @@ -1534,17 +1509,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 @@ -1795,10 +1771,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)) @@ -1858,10 +1834,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] @@ -1876,11 +1852,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!"); } @@ -1890,10 +1867,10 @@ 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 { +UPCStandalonePointerGadget::getFixits(const FixitStrategy &S) const { const auto VD = cast(Node->getDecl()); switch (S.lookup(VD)) { - case Strategy::Kind::Span: { + case FixitStrategy::Kind::Span: { ASTContext &Ctx = VD->getASTContext(); SourceManager &SM = Ctx.getSourceManager(); // Inserts the .data() after the DRE @@ -1905,10 +1882,11 @@ UPCStandalonePointerGadget::getFixits(const Strategy &S) const { // FIXME: Points inside a macro expansion. break; } - 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"); } @@ -1953,45 +1931,14 @@ fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node) { } std::optional -UPCPreIncrementGadget::getFixits(const Strategy &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) { - FixItList Fixes; - std::stringstream SS; - const Stmt *PreIncNode = getBaseStmt(); - StringRef varName = VD->getName(); - const ASTContext &Ctx = VD->getASTContext(); - - // To transform UPC(++p) to UPC((p = p.subspan(1)).data()): - SS << "(" << varName.data() << " = " << varName.data() - << ".subspan(1)).data()"; - std::optional PreIncLocation = - getEndCharLoc(PreIncNode, Ctx.getSourceManager(), Ctx.getLangOpts()); - if (!PreIncLocation) - return std::nullopt; - - Fixes.push_back(FixItHint::CreateReplacement( - SourceRange(PreIncNode->getBeginLoc(), *PreIncLocation), SS.str())); - return Fixes; - } - } - return std::nullopt; // Not in the cases that we can handle for now, give up. -} - -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(); @@ -2025,6 +1972,36 @@ 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 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) == FixitStrategy::Kind::Span) { + FixItList Fixes; + std::stringstream SS; + const Stmt *PreIncNode = getBaseStmt(); + StringRef varName = VD->getName(); + const ASTContext &Ctx = VD->getASTContext(); + + // To transform UPC(++p) to UPC((p = p.subspan(1)).data()): + SS << "(" << varName.data() << " = " << varName.data() + << ".subspan(1)).data()"; + std::optional PreIncLocation = + getEndCharLoc(PreIncNode, Ctx.getSourceManager(), Ctx.getLangOpts()); + if (!PreIncLocation) + return std::nullopt; + + Fixes.push_back(FixItHint::CreateReplacement( + SourceRange(PreIncNode->getBeginLoc(), *PreIncLocation), SS.str())); + return Fixes; + } + } + 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 @@ -2256,7 +2233,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: @@ -2273,9 +2250,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; @@ -2286,7 +2263,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; @@ -2491,10 +2469,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) { @@ -2525,7 +2596,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); @@ -2536,11 +2607,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!"); @@ -2601,7 +2679,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 = @@ -2621,7 +2700,7 @@ static FixItList createFunctionOverloadsForParms( // Constructs self-contained fix-its for each variable in `FixablesForAllVars`. static std::map -getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S, +getFixIts(FixableGadgetSets &FixablesForAllVars, const FixitStrategy &S, ASTContext &Ctx, /* The function decl under analysis */ const Decl *D, const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler, @@ -2720,11 +2799,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; } @@ -3030,7 +3112,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); @@ -3055,7 +3137,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D, FixItsIt != FixItsForVariableGroup.end() ? std::move(FixItsIt->second) : FixItList{}, - D); + 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 f629fb9819cdb..e2fb5f404e2b0 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2273,7 +2273,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) @@ -2288,7 +2289,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 5fff0854d4546..a5b578b98d4e5 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}} }