Skip to content

Commit

Permalink
[-Wunsafe-buffer-usage] Add Fixable for dereference of simple ptr ari…
Browse files Browse the repository at this point in the history 
…thmetic

For each expression `e` of the form `*(DRE + n)` (or `*(n + DRE)`), where
`DRE` has a pointer type and `n` is an integer literal, `e` will be
transformed to `DRE[n]` (or `n[DRE]` respectively), if
- `e` is at the left-hand side of an assignment or is an lvalue being casted to an rvalue; and
- the variable referred by `DRE` is going to be transformed to be of `std::span` type.

Reviewed by: jkorous, NoQ

Differential revision: https://reviews.llvm.org/D142795
  • Loading branch information
@ziqingluo-90
ziqingluo-90 committed Mar 21, 2023
1 parent b6f7e59 commit 6a0f2e5
Show file tree
Hide file tree
Showing 3 changed files with 302 additions and 0 deletions.
1 change: 1 addition & 0 deletions clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
View file
Expand Up @@ -31,6 +31,7 @@ WARNING_GADGET(ArraySubscript)
WARNING_GADGET(PointerArithmetic)
WARNING_GADGET(UnsafeBufferUsageAttr)
FIXABLE_GADGET(ULCArraySubscript)
FIXABLE_GADGET(DerefSimplePtrArithFixable)

#undef FIXABLE_GADGET
#undef WARNING_GADGET
Expand Down
102 changes: 102 additions & 0 deletions clang/lib/Analysis/UnsafeBufferUsage.cpp
View file
Expand Up @@ -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"
Expand Down Expand Up @@ -558,6 +559,56 @@ class Strategy {
};
} // 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<DeclRefExpr>(BaseDeclRefExprTag)),
DerefOp(Result.Nodes.getNodeAs<UnaryOperator>(DerefOpTag)),
AddOp(Result.Nodes.getNodeAs<BinaryOperator>(AddOpTag)),
Offset(Result.Nodes.getNodeAs<IntegerLiteral>(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<FixItList> 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<FixableGadgetList, WarningGadgetList, DeclUseTracker>
findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler) {
Expand Down Expand Up @@ -812,6 +863,57 @@ static StringRef getExprText(const Expr *E, const SourceManager &SM,
LangOpts);
}

std::optional<FixItList>
DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const {
const VarDecl *VD = dyn_cast<VarDecl>(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.
Expand Down
199 changes: 199 additions & 0 deletions clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
View file
@@ -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<int> 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<int> 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<int> 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
}

0 comments on commit 6a0f2e5

Please sign in to comment.