diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def index ff687a0d178bd..757ee452ced74 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def @@ -36,6 +36,7 @@ FIXABLE_GADGET(PointerDereference) FIXABLE_GADGET(UPCAddressofArraySubscript) // '&DRE[any]' in an Unspecified Pointer Context FIXABLE_GADGET(UPCStandalonePointer) FIXABLE_GADGET(UPCPreIncrement) // '++Ptr' in an Unspecified Pointer Context +FIXABLE_GADGET(UUCAddAssign) // 'Ptr += n' in an Unspecified Untyped Context FIXABLE_GADGET(PointerAssignment) FIXABLE_GADGET(PointerInit) diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index e332a3609290a..a1efb76be68b7 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -1028,6 +1028,46 @@ class UPCPreIncrementGadget : public FixableGadget { } }; +// Representing a pointer type expression of the form `Ptr += n` in an +// Unspecified Untyped Context (UUC): +class UUCAddAssignGadget : public FixableGadget { +private: + static constexpr const char *const UUCAddAssignTag = + "PointerAddAssignUnderUUC"; + static constexpr const char *const OffsetTag = "Offset"; + + const BinaryOperator *Node; // the `Ptr += n` node + const Expr *Offset = nullptr; + +public: + UUCAddAssignGadget(const MatchFinder::MatchResult &Result) + : FixableGadget(Kind::UUCAddAssign), + Node(Result.Nodes.getNodeAs(UUCAddAssignTag)), + Offset(Result.Nodes.getNodeAs(OffsetTag)) { + assert(Node != nullptr && "Expecting a non-null matching result"); + } + + static bool classof(const Gadget *G) { + return G->getKind() == Kind::UUCAddAssign; + } + + static Matcher matcher() { + return stmt(isInUnspecifiedUntypedContext(expr(ignoringImpCasts( + binaryOperator(hasOperatorName("+="), + hasLHS(declRefExpr(toSupportedVariable())), + hasRHS(expr().bind(OffsetTag))) + .bind(UUCAddAssignTag))))); + } + + virtual std::optional getFixits(const Strategy &S) const override; + + virtual const Stmt *getBaseStmt() const override { return Node; } + + virtual DeclUseList getClaimedVarUseSites() const override { + return {dyn_cast(Node->getLHS())}; + } +}; + // Representing a fixable expression of the form `*(ptr + 123)` or `*(123 + // ptr)`: class DerefSimplePtrArithFixableGadget : public FixableGadget { @@ -1312,6 +1352,16 @@ PointerInitGadget::getFixits(const Strategy &S) const { return std::nullopt; } +static bool isNonNegativeIntegerExpr(const Expr *Expr, const VarDecl *VD, + const ASTContext &Ctx) { + if (auto ConstVal = Expr->getIntegerConstantExpr(Ctx)) { + if (ConstVal->isNegative()) + return false; + } else if (!Expr->getType()->isUnsignedIntegerType()) + return false; + return true; +} + std::optional ULCArraySubscriptGadget::getFixits(const Strategy &S) const { if (const auto *DRE = @@ -1319,14 +1369,12 @@ ULCArraySubscriptGadget::getFixits(const Strategy &S) const { if (const auto *VD = dyn_cast(DRE->getDecl())) { switch (S.lookup(VD)) { case Strategy::Kind::Span: { + // If the index has a negative constant value, we give up as no valid // fix-it can be generated: const ASTContext &Ctx = // FIXME: we need ASTContext to be passed in! VD->getASTContext(); - if (auto ConstVal = Node->getIdx()->getIntegerConstantExpr(Ctx)) { - if (ConstVal->isNegative()) - return std::nullopt; - } else if (!Node->getIdx()->getType()->isUnsignedIntegerType()) + if (!isNonNegativeIntegerExpr(Node->getIdx(), VD, Ctx)) return std::nullopt; // no-op is a good fix-it, otherwise return FixItList{}; @@ -1405,10 +1453,8 @@ static std::optional getPastLoc(const NodeTy *Node, const LangOptions &LangOpts) { SourceLocation Loc = Lexer::getLocForEndOfToken(Node->getEndLoc(), 0, SM, LangOpts); - if (Loc.isValid()) return Loc; - return std::nullopt; } @@ -1766,6 +1812,47 @@ fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node) { FixItHint::CreateReplacement(Node->getSourceRange(), SS.str())}; } +std::optional +UUCAddAssignGadget::getFixits(const Strategy &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) { + FixItList Fixes; + + const Stmt *AddAssignNode = getBaseStmt(); + StringRef varName = VD->getName(); + const ASTContext &Ctx = VD->getASTContext(); + + if (!isNonNegativeIntegerExpr(Offset, VD, Ctx)) + return std::nullopt; + + // To transform UUC(p += n) to UUC(p = p.subspan(..)): + bool NotParenExpr = + (Offset->IgnoreParens()->getBeginLoc() == Offset->getBeginLoc()); + std::string SS = varName.str() + " = " + varName.str() + ".subspan"; + if (NotParenExpr) + SS += "("; + + std::optional AddAssignLocation = getEndCharLoc( + AddAssignNode, Ctx.getSourceManager(), Ctx.getLangOpts()); + if (!AddAssignLocation) + return std::nullopt; + + Fixes.push_back(FixItHint::CreateReplacement( + SourceRange(AddAssignNode->getBeginLoc(), Node->getOperatorLoc()), + SS)); + if (NotParenExpr) + Fixes.push_back(FixItHint::CreateInsertion( + Offset->getEndLoc().getLocWithOffset(1), ")")); + return Fixes; + } + } + return std::nullopt; // Not in the cases that we can handle for now, give up. +} std::optional UPCPreIncrementGadget::getFixits(const Strategy &S) const { DeclUseList DREs = getClaimedVarUseSites(); diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp new file mode 100644 index 0000000000000..5c03cc10025c6 --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp @@ -0,0 +1,59 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \ +// RUN: -fsafe-buffer-usage-suggestions \ +// RUN: -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s +void foo(int * , int *); + +void add_assign_test(unsigned int n, int *a, int y) { + int *p = new int[10]; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p" + // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{" + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}" + p += 2; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:7}:"p = p.subspan(" + // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:9-[[@LINE-2]]:9}:")" + + int *r = p; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span r" + // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{" + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", <# placeholder #>}" + while (*r != 0) { + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:11}:"" + // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"[0]" + r += 2; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:5-[[@LINE-1]]:9}:"r = r.subspan(" + // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:11-[[@LINE-2]]:11}:")" + } + + if (*p == 0) { + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:8}:"" + // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:9-[[@LINE-2]]:9}:"[0]" + p += n; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:5-[[@LINE-1]]:9}:"p = p.subspan(" + // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:11-[[@LINE-2]]:11}:")" + } + + if (*p == 1) + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:8}:"" + // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:9-[[@LINE-2]]:9}:"[0]" + p += 3; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:5-[[@LINE-1]]:9}:"p = p.subspan(" + // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:11-[[@LINE-2]]:11}:")" + + a += -9; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:5-[[@LINE-1]]:9}:"p = p.subspan(" + + a += y; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:5-[[@LINE-1]]:9}:"p = p.subspan(" +} + +int expr_test(unsigned x, int *q, int y) { + char *p = new char[8]; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span p" + // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{" + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 8}" + p += (x + 1); + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:7}:"p = p.subspan" + + q += (y + 7); + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:7}:"q = q.subspan" +}