diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def @@ -31,6 +31,7 @@ WARNING_GADGET(PointerArithmetic) WARNING_GADGET(UnsafeBufferUsageAttr) FIXABLE_GADGET(ULCArraySubscript) +FIXABLE_GADGET(DerefSimplePtrArithFixable) #undef FIXABLE_GADGET #undef WARNING_GADGET diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "clang/Analysis/Analyses/UnsafeBufferUsage.h" +#include "clang/AST/Decl.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Lex/Lexer.h" @@ -558,6 +559,56 @@ }; } // namespace +// Representing a fixable expression of the form `*(ptr + 123)` or `*(123 + +// ptr)`: +class DerefSimplePtrArithFixableGadget : public FixableGadget { + static constexpr const char *const BaseDeclRefExprTag = "BaseDRE"; + static constexpr const char *const DerefOpTag = "DerefOp"; + static constexpr const char *const AddOpTag = "AddOp"; + static constexpr const char *const OffsetTag = "Offset"; + + const DeclRefExpr *BaseDeclRefExpr = nullptr; + const UnaryOperator *DerefOp = nullptr; + const BinaryOperator *AddOp = nullptr; + const IntegerLiteral *Offset = nullptr; + +public: + DerefSimplePtrArithFixableGadget(const MatchFinder::MatchResult &Result) + : FixableGadget(Kind::DerefSimplePtrArithFixable), + BaseDeclRefExpr( + Result.Nodes.getNodeAs(BaseDeclRefExprTag)), + DerefOp(Result.Nodes.getNodeAs(DerefOpTag)), + AddOp(Result.Nodes.getNodeAs(AddOpTag)), + Offset(Result.Nodes.getNodeAs(OffsetTag)) {} + + static Matcher matcher() { + // clang-format off + auto ThePtr = expr(hasPointerType(), + ignoringImpCasts(declRefExpr(to(varDecl())).bind(BaseDeclRefExprTag))); + auto PlusOverPtrAndInteger = expr(anyOf( + binaryOperator(hasOperatorName("+"), hasLHS(ThePtr), + hasRHS(integerLiteral().bind(OffsetTag))) + .bind(AddOpTag), + binaryOperator(hasOperatorName("+"), hasRHS(ThePtr), + hasLHS(integerLiteral().bind(OffsetTag))) + .bind(AddOpTag))); + return isInUnspecifiedLvalueContext(unaryOperator( + hasOperatorName("*"), + hasUnaryOperand(ignoringParens(PlusOverPtrAndInteger))) + .bind(DerefOpTag)); + // clang-format on + } + + virtual std::optional getFixits(const Strategy &s) const final; + + // TODO remove this method from FixableGadget interface + virtual const Stmt *getBaseStmt() const final { return nullptr; } + + virtual DeclUseList getClaimedVarUseSites() const final { + return {BaseDeclRefExpr}; + } +}; + /// Scan the function and return a list of gadgets found with provided kits. static std::tuple findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler) { @@ -812,6 +863,57 @@ LangOpts); } +std::optional +DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const { + const VarDecl *VD = dyn_cast(BaseDeclRefExpr->getDecl()); + + if (VD && s.lookup(VD) == Strategy::Kind::Span) { + ASTContext &Ctx = VD->getASTContext(); + // std::span can't represent elements before its begin() + if (auto ConstVal = Offset->getIntegerConstantExpr(Ctx)) + if (ConstVal->isNegative()) + return std::nullopt; + + // note that the expr may (oddly) has multiple layers of parens + // example: + // *((..(pointer + 123)..)) + // goal: + // pointer[123] + // Fix-It: + // remove '*(' + // replace ' + ' with '[' + // replace ')' with ']' + + // example: + // *((..(123 + pointer)..)) + // goal: + // 123[pointer] + // Fix-It: + // remove '*(' + // replace ' + ' with '[' + // replace ')' with ']' + + const Expr *LHS = AddOp->getLHS(), *RHS = AddOp->getRHS(); + const SourceManager &SM = Ctx.getSourceManager(); + const LangOptions &LangOpts = Ctx.getLangOpts(); + CharSourceRange StarWithTrailWhitespace = + clang::CharSourceRange::getCharRange(DerefOp->getOperatorLoc(), + LHS->getBeginLoc()); + CharSourceRange PlusWithSurroundingWhitespace = + clang::CharSourceRange::getCharRange(getPastLoc(LHS, SM, LangOpts), + RHS->getBeginLoc()); + CharSourceRange ClosingParenWithPrecWhitespace = + clang::CharSourceRange::getCharRange(getPastLoc(AddOp, SM, LangOpts), + getPastLoc(DerefOp, SM, LangOpts)); + + return FixItList{ + {FixItHint::CreateRemoval(StarWithTrailWhitespace), + FixItHint::CreateReplacement(PlusWithSurroundingWhitespace, "["), + FixItHint::CreateReplacement(ClosingParenWithPrecWhitespace, "]")}}; + } + return std::nullopt; // something wrong or unsupported, 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. diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp new file mode 100644 --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp @@ -0,0 +1,199 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fsyntax-only %s 2>&1 | FileCheck %s + +// TODO test we don't mess up vertical whitespace +// TODO test different whitespaces +// TODO test different contexts + // when it's on the right side + +void basic() { + int *ptr; +// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span ptr" + *(ptr+5)=1; +// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:5}:"" +// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:8-[[@LINE-2]]:9}:"[" +// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:11}:"]" +} + +// The weird preceding semicolon ensures that we preserve that range intact. +void char_ranges() { + int *p; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span p" + + ;* ( p + 5 ) = 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:8}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:9-[[@LINE-2]]:12}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:15}:"]" + + ;* (p+5)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:11}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"]" + + ;*( p+5)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:11}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"]" + + ;*( p+5)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:11}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"]" + + ;*( p +5)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:7}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:8-[[@LINE-2]]:12}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:14}:"]" + + ;*(p+ 5)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:11}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"]" + + ;*(p+ 5 )= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:9}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:14}:"]" + + ;*(p+ 5) = 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:9}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:11}:"]" + + ; *(p+5)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:9}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:11}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"]" + + ;*(p+123456)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:8}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:14-[[@LINE-3]]:15}:"]" + + ;* (p+123456)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:11}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:18}:"]" + + ;*( p+123456)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:11}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:18}:"]" + + ;*( p+123456)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:11}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:18}:"]" + + ;*(p +123456)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:11}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:18}:"]" + + ;*(p+ 123456)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:11}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:18}:"]" + + ;*(p+123456 )= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:8}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:14-[[@LINE-3]]:18}:"]" + + ;*(p+123456) = 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:8}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:14-[[@LINE-3]]:15}:"]" + + int *ptrrrrrr; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span ptrrrrrr" + + ;* ( ptrrrrrr + 123456 )= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:8}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:19}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:25-[[@LINE-3]]:27}:"]" + + ;* (ptrrrrrr+123456)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:18}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:25}:"]" + + ;*( ptrrrrrr+123456)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:18}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:25}:"]" + + ;*( ptrrrrrr+123456)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:18}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:25}:"]" + + ;*(ptrrrrrr +123456)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:18}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:25}:"]" + + ;*(ptrrrrrr+ 123456)= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:18}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:25}:"]" + + ;*(ptrrrrrr+123456 )= 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:15}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:21-[[@LINE-3]]:25}:"]" + + ;*(ptrrrrrr+123456) = 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:15}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:21-[[@LINE-3]]:22}:"]" +} + +void base_on_rhs() { + int* ptr; + *(10 + ptr) = 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:5}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:10}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:14}:"]" +} + +void many_parens() { + int* ptr; + *(( (10 + ptr)) ) = 1; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:13}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:16-[[@LINE-3]]:20}:"]" +} + +void lvaue_to_rvalue() { + int * ptr; + int tmp = *(ptr + 10); + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:15}:"" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:21}:"[" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:24}:"]" +} + +// Fixits emitted for the cases below would be incorrect. +// CHECK-NOT: fix-it: +// Array subsctipt opertor of std::span accepts unsigned integer. +void negative() { + int* ptr; + *(ptr + -5) = 1; // skip +} + +void subtraction() { + int* ptr; + *(ptr - 5) = 1; // skip +} + +void subtraction_of_negative() { + int* ptr; + *(ptr - -5) = 1; // FIXME: implement fixit (uncommon case - low priority) +} + + +void bindingDecl(int *p, int *q) { + int * a[2] = {p, q}; + auto [x, y] = a; + + *(x + 1) = 1; // FIXME: deal with `BindingDecl`s +}