Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def =================================================================== --- clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def +++ clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def @@ -31,6 +31,8 @@ WARNING_GADGET(PointerArithmetic) 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 @@ -527,6 +533,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) { @@ -704,7 +821,7 @@ static std::string getAPIntText(APInt Val) { SmallVector Txt; Val.toString(Txt, 10, true); - return Txt.data(); + return std::string(Txt.data(), Txt.size()); } // Return the SourceLocation right before the given `Loc` 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,95 @@ +// RUN: cp %s %t.cpp +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fsyntax-only -fixit %t.cpp +// RUN: grep -v CHECK %t.cpp | 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; + *(ptr+5)=1; + // CHECK: ptr[5]=1; +} + +// The weird preceding semicolon ensures that we preserve that range intact. +void char_ranges() { + int *p; + ;* ( p + 5 ) = 1; + // CHECK: ;p[5] = 1; + ;* (p+5)= 1; + // CHECK: ;p[5]= 1; + ;*( p+5)= 1; + // CHECK: ;p[5]= 1; + ;*( p+5)= 1; + // CHECK: ;p[5]= 1; + ;*( p +5)= 1; + // CHECK: ;p[5]= 1; + ;*(p+ 5)= 1; + // CHECK: ;p[5]= 1; + ;*(p+ 5 )= 1; + //CHECK: ;p[5]= 1; + ;*(p+ 5) = 1; + // CHECK: ;p[5] = 1; + ; *(p+5)= 1; + // CHECK: ; p[5]= 1; + + ;*(p+123456)= 1; + // CHECK: ;p[123456]= 1; + ;* (p+123456)= 1; + // CHECK: ;p[123456]= 1; + ;*( p+123456)= 1; + // CHECK: ;p[123456]= 1; + ;*( p+123456)= 1; + // CHECK: ;p[123456]= 1; + ;*(p +123456)= 1; + // CHECK: ;p[123456]= 1; + ;*(p+ 123456)= 1; + // CHECK: ;p[123456]= 1; + ;*(p+123456 )= 1; + // CHECK: ;p[123456]= 1; + ;*(p+123456) = 1; + // CHECK: ;p[123456] = 1; + + int *ptrrrrrr; + ;* ( ptrrrrrr + 123456 )= 1; + // CHECK: ;ptrrrrrr[123456]= 1; + ;* (ptrrrrrr+123456)= 1; + // CHECK: ;ptrrrrrr[123456]= 1; + ;*( ptrrrrrr+123456)= 1; + // CHECK: ;ptrrrrrr[123456]= 1; + ;*( ptrrrrrr+123456)= 1; + // CHECK: ;ptrrrrrr[123456]= 1; + ;*(ptrrrrrr +123456)= 1; + // CHECK: ;ptrrrrrr[123456]= 1; + ;*(ptrrrrrr+ 123456)= 1; + // CHECK: ;ptrrrrrr[123456]= 1; + ;*(ptrrrrrr+123456 )= 1; + // CHECK: ;ptrrrrrr[123456]= 1; + ;*(ptrrrrrr+123456) = 1; + // CHECK: ;ptrrrrrr[123456] = 1; +} + +// Fixits emitted for the cases below would be incorrect. +// CHECK-NOT: [ +// 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 +}