Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def =================================================================== --- clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def +++ clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def @@ -32,6 +32,8 @@ WARNING_GADGET(UnsafeBufferUsageAttr) FIXABLE_GADGET(ULCArraySubscript) +FIXABLE_GADGET(DerefSimplePtrArithFixable) + #undef FIXABLE_GADGET #undef WARNING_GADGET #undef GADGET Index: clang/lib/Analysis/UnsafeBufferUsage.cpp =================================================================== --- clang/lib/Analysis/UnsafeBufferUsage.cpp +++ clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -7,8 +7,14 @@ //===----------------------------------------------------------------------===// #include "clang/Analysis/Analyses/UnsafeBufferUsage.h" +#include "clang/AST/Decl.h" +#include "clang/AST/Expr.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Basic/CharInfo.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/Basic/SourceManager.h" #include "clang/Lex/Lexer.h" #include "llvm/ADT/SmallVector.h" #include @@ -554,6 +560,117 @@ }; } // namespace +namespace { + /// \returns either begin of sequence of horizontal whitespace preceding + /// \p Loc or Loc if there's no preceding whitespace or Loc is invalid. + SourceLocation getBeginOfPrecHWSpace(SourceLocation Loc, SourceManager& SM){ + assert(Loc.isValid()); + + while(true) { + SourceLocation PrecLoc = Loc.getLocWithOffset(-1); + if (PrecLoc.isInvalid()) + break; + if (isHorizontalWhitespace(*SM.getCharacterData(PrecLoc))) { + Loc = PrecLoc; + continue; + } + break; + } + return Loc; + } +} // namespace + +class DerefSimplePtrArithFixableGadget : public FixableGadget { + static constexpr const char *const BaseDeclRefExprTag = "BaseDRE"; + static constexpr const char *const DerefOpTag = "DerefOp"; + static constexpr const char *const ParenExprTag = "ParenExpr"; + static constexpr const char *const AddOpTag = "AddOp"; + static constexpr const char *const RHSTag = "RHS"; + + const DeclRefExpr *BaseDeclRefExpr = nullptr; + const UnaryOperator *DerefOp = nullptr; + const ParenExpr *ParenEx = nullptr; + const BinaryOperator *AddOp = nullptr; + const IntegerLiteral *RHS = nullptr; +public: + DerefSimplePtrArithFixableGadget(const MatchFinder::MatchResult &Result) + : FixableGadget(Kind::DerefSimplePtrArithFixable), + BaseDeclRefExpr(Result.Nodes.getNodeAs(BaseDeclRefExprTag)), + DerefOp(Result.Nodes.getNodeAs(DerefOpTag)), + ParenEx(Result.Nodes.getNodeAs(ParenExprTag)), + AddOp(Result.Nodes.getNodeAs(AddOpTag)), + RHS(Result.Nodes.getNodeAs(RHSTag)) + {} + + static Matcher matcher() { +// FIXME: implement the mirror version: idx + pointer +// FIXME: generalize for any number of parens and impl casts +// clang-format off + return isInUnspecifiedLvalueContext( + unaryOperator( + hasOperatorName("*"), + hasUnaryOperand( + parenExpr( + has( + binaryOperator( + hasOperatorName("+"), + hasLHS( + expr( + hasPointerType(), + ignoringImpCasts( + declRefExpr().bind(BaseDeclRefExprTag)))), + hasRHS(integerLiteral().bind(RHSTag) + )).bind(AddOpTag) + )).bind(ParenExprTag) + )).bind(DerefOpTag)); +// clang-format on + } + + virtual std::optional getFixits(const Strategy &s) const final { + const VarDecl* VD = dyn_cast(BaseDeclRefExpr->getDecl()); + assert(VD); + if (s.lookup(VD) == Strategy::Kind::Span) { + ASTContext& Ctx = VD->getASTContext(); + // std::span can't represent elements before its begin() + if (RHS->getIntegerConstantExpr(Ctx)->isNegative()) + return std::nullopt; + + // example: + // *(pointer + 123) + // goal: + // pointer[123] + // Fix-It: + // remove '*(' + // replace ' + ' with '[' + // replace ')' with ']' + + SourceManager &SM = Ctx.getSourceManager(); + CharSourceRange StarWithTrailWhitespace = clang::CharSourceRange::getCharRange(DerefOp->getOperatorLoc(), BaseDeclRefExpr->getBeginLoc()); + CharSourceRange PlusWithSurroundingWhitespace = clang::CharSourceRange::getCharRange(getBeginOfPrecHWSpace(AddOp->getOperatorLoc(), SM), RHS->getLocation()); + CharSourceRange ClosingParenWithPrecWhitespace = clang::CharSourceRange::getCharRange(getBeginOfPrecHWSpace(ParenEx->getEndLoc(), SM), ParenEx->getRParen().getLocWithOffset(1)); + + return FixItList{{ + FixItHint::CreateRemoval(StarWithTrailWhitespace), + FixItHint::CreateReplacement(PlusWithSurroundingWhitespace, "["), + FixItHint::CreateReplacement(ClosingParenWithPrecWhitespace, "]") + }}; + } + + llvm_unreachable("unsupported strategies for FixableGadgets"); + return std::nullopt; + } + + // 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) { Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp =================================================================== --- /dev/null +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp @@ -0,0 +1,172 @@ +// 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}:"]" +} + +// 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 base_on_rhs() { + int* ptr; + *(10 + ptr) = 1; // FIXME: not implemented yet +}