Skip to content

Commit 6a0f2e5

Browse files
ziqingluo-90ziqingluo-90
committed
[-Wunsafe-buffer-usage] Add Fixable for dereference of simple ptr arithmetic
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
1 parent b6f7e59 commit 6a0f2e5

File tree

3 files changed

+302
-0
lines changed

3 files changed

+302
-0
lines changed

‎clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ WARNING_GADGET(ArraySubscript)
3131
WARNING_GADGET(PointerArithmetic)
3232
WARNING_GADGET(UnsafeBufferUsageAttr)
3333
FIXABLE_GADGET(ULCArraySubscript)
34+
FIXABLE_GADGET(DerefSimplePtrArithFixable)
3435

3536
#undef FIXABLE_GADGET
3637
#undef WARNING_GADGET

‎clang/lib/Analysis/UnsafeBufferUsage.cpp

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
//===----------------------------------------------------------------------===//
88

99
#include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
10+
#include "clang/AST/Decl.h"
1011
#include "clang/AST/RecursiveASTVisitor.h"
1112
#include "clang/ASTMatchers/ASTMatchFinder.h"
1213
#include "clang/Lex/Lexer.h"
@@ -558,6 +559,56 @@ class Strategy {
558559
};
559560
} // namespace
560561

562+
// Representing a fixable expression of the form `*(ptr + 123)` or `*(123 +
563+
// ptr)`:
564+
class DerefSimplePtrArithFixableGadget : public FixableGadget {
565+
static constexpr const char *const BaseDeclRefExprTag = "BaseDRE";
566+
static constexpr const char *const DerefOpTag = "DerefOp";
567+
static constexpr const char *const AddOpTag = "AddOp";
568+
static constexpr const char *const OffsetTag = "Offset";
569+
570+
const DeclRefExpr *BaseDeclRefExpr = nullptr;
571+
const UnaryOperator *DerefOp = nullptr;
572+
const BinaryOperator *AddOp = nullptr;
573+
const IntegerLiteral *Offset = nullptr;
574+
575+
public:
576+
DerefSimplePtrArithFixableGadget(const MatchFinder::MatchResult &Result)
577+
: FixableGadget(Kind::DerefSimplePtrArithFixable),
578+
BaseDeclRefExpr(
579+
Result.Nodes.getNodeAs<DeclRefExpr>(BaseDeclRefExprTag)),
580+
DerefOp(Result.Nodes.getNodeAs<UnaryOperator>(DerefOpTag)),
581+
AddOp(Result.Nodes.getNodeAs<BinaryOperator>(AddOpTag)),
582+
Offset(Result.Nodes.getNodeAs<IntegerLiteral>(OffsetTag)) {}
583+
584+
static Matcher matcher() {
585+
// clang-format off
586+
auto ThePtr = expr(hasPointerType(),
587+
ignoringImpCasts(declRefExpr(to(varDecl())).bind(BaseDeclRefExprTag)));
588+
auto PlusOverPtrAndInteger = expr(anyOf(
589+
binaryOperator(hasOperatorName("+"), hasLHS(ThePtr),
590+
hasRHS(integerLiteral().bind(OffsetTag)))
591+
.bind(AddOpTag),
592+
binaryOperator(hasOperatorName("+"), hasRHS(ThePtr),
593+
hasLHS(integerLiteral().bind(OffsetTag)))
594+
.bind(AddOpTag)));
595+
return isInUnspecifiedLvalueContext(unaryOperator(
596+
hasOperatorName("*"),
597+
hasUnaryOperand(ignoringParens(PlusOverPtrAndInteger)))
598+
.bind(DerefOpTag));
599+
// clang-format on
600+
}
601+
602+
virtual std::optional<FixItList> getFixits(const Strategy &s) const final;
603+
604+
// TODO remove this method from FixableGadget interface
605+
virtual const Stmt *getBaseStmt() const final { return nullptr; }
606+
607+
virtual DeclUseList getClaimedVarUseSites() const final {
608+
return {BaseDeclRefExpr};
609+
}
610+
};
611+
561612
/// Scan the function and return a list of gadgets found with provided kits.
562613
static std::tuple<FixableGadgetList, WarningGadgetList, DeclUseTracker>
563614
findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler) {
@@ -812,6 +863,57 @@ static StringRef getExprText(const Expr *E, const SourceManager &SM,
812863
LangOpts);
813864
}
814865

866+
std::optional<FixItList>
867+
DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const {
868+
const VarDecl *VD = dyn_cast<VarDecl>(BaseDeclRefExpr->getDecl());
869+
870+
if (VD && s.lookup(VD) == Strategy::Kind::Span) {
871+
ASTContext &Ctx = VD->getASTContext();
872+
// std::span can't represent elements before its begin()
873+
if (auto ConstVal = Offset->getIntegerConstantExpr(Ctx))
874+
if (ConstVal->isNegative())
875+
return std::nullopt;
876+
877+
// note that the expr may (oddly) has multiple layers of parens
878+
// example:
879+
// *((..(pointer + 123)..))
880+
// goal:
881+
// pointer[123]
882+
// Fix-It:
883+
// remove '*('
884+
// replace ' + ' with '['
885+
// replace ')' with ']'
886+
887+
// example:
888+
// *((..(123 + pointer)..))
889+
// goal:
890+
// 123[pointer]
891+
// Fix-It:
892+
// remove '*('
893+
// replace ' + ' with '['
894+
// replace ')' with ']'
895+
896+
const Expr *LHS = AddOp->getLHS(), *RHS = AddOp->getRHS();
897+
const SourceManager &SM = Ctx.getSourceManager();
898+
const LangOptions &LangOpts = Ctx.getLangOpts();
899+
CharSourceRange StarWithTrailWhitespace =
900+
clang::CharSourceRange::getCharRange(DerefOp->getOperatorLoc(),
901+
LHS->getBeginLoc());
902+
CharSourceRange PlusWithSurroundingWhitespace =
903+
clang::CharSourceRange::getCharRange(getPastLoc(LHS, SM, LangOpts),
904+
RHS->getBeginLoc());
905+
CharSourceRange ClosingParenWithPrecWhitespace =
906+
clang::CharSourceRange::getCharRange(getPastLoc(AddOp, SM, LangOpts),
907+
getPastLoc(DerefOp, SM, LangOpts));
908+
909+
return FixItList{
910+
{FixItHint::CreateRemoval(StarWithTrailWhitespace),
911+
FixItHint::CreateReplacement(PlusWithSurroundingWhitespace, "["),
912+
FixItHint::CreateReplacement(ClosingParenWithPrecWhitespace, "]")}};
913+
}
914+
return std::nullopt; // something wrong or unsupported, give up
915+
}
916+
815917
// For a non-null initializer `Init` of `T *` type, this function returns
816918
// `FixItHint`s producing a list initializer `{Init, S}` as a part of a fix-it
817919
// to output stream.

‎clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fsyntax-only %s 2>&1 | FileCheck %s
2+
3+
// TODO test we don't mess up vertical whitespace
4+
// TODO test different whitespaces
5+
// TODO test different contexts
6+
// when it's on the right side
7+
8+
void basic() {
9+
int *ptr;
10+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> ptr"
11+
*(ptr+5)=1;
12+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:5}:""
13+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:8-[[@LINE-2]]:9}:"["
14+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:11}:"]"
15+
}
16+
17+
// The weird preceding semicolon ensures that we preserve that range intact.
18+
void char_ranges() {
19+
int *p;
20+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span<int> p"
21+
22+
;* ( p + 5 ) = 1;
23+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:8}:""
24+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:9-[[@LINE-2]]:12}:"["
25+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:15}:"]"
26+
27+
;* (p+5)= 1;
28+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:""
29+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:11}:"["
30+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"]"
31+
32+
;*( p+5)= 1;
33+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:""
34+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:11}:"["
35+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"]"
36+
37+
;*( p+5)= 1;
38+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:""
39+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:11}:"["
40+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"]"
41+
42+
;*( p +5)= 1;
43+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:7}:""
44+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:8-[[@LINE-2]]:12}:"["
45+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:14}:"]"
46+
47+
;*(p+ 5)= 1;
48+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:""
49+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:11}:"["
50+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"]"
51+
52+
;*(p+ 5 )= 1;
53+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:""
54+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:9}:"["
55+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:14}:"]"
56+
57+
;*(p+ 5) = 1;
58+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:""
59+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:9}:"["
60+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:11}:"]"
61+
62+
; *(p+5)= 1;
63+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:9}:""
64+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:11}:"["
65+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"]"
66+
67+
;*(p+123456)= 1;
68+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:""
69+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:8}:"["
70+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:14-[[@LINE-3]]:15}:"]"
71+
72+
;* (p+123456)= 1;
73+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:""
74+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:11}:"["
75+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:18}:"]"
76+
77+
;*( p+123456)= 1;
78+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:""
79+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:11}:"["
80+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:18}:"]"
81+
82+
;*( p+123456)= 1;
83+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:""
84+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:11}:"["
85+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:18}:"]"
86+
87+
;*(p +123456)= 1;
88+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:""
89+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:11}:"["
90+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:18}:"]"
91+
92+
;*(p+ 123456)= 1;
93+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:""
94+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:11}:"["
95+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:18}:"]"
96+
97+
;*(p+123456 )= 1;
98+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:""
99+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:8}:"["
100+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:14-[[@LINE-3]]:18}:"]"
101+
102+
;*(p+123456) = 1;
103+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:""
104+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:8}:"["
105+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:14-[[@LINE-3]]:15}:"]"
106+
107+
int *ptrrrrrr;
108+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span<int> ptrrrrrr"
109+
110+
;* ( ptrrrrrr + 123456 )= 1;
111+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:8}:""
112+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:19}:"["
113+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:25-[[@LINE-3]]:27}:"]"
114+
115+
;* (ptrrrrrr+123456)= 1;
116+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:""
117+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:18}:"["
118+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:25}:"]"
119+
120+
;*( ptrrrrrr+123456)= 1;
121+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:""
122+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:18}:"["
123+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:25}:"]"
124+
125+
;*( ptrrrrrr+123456)= 1;
126+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:9}:""
127+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:18}:"["
128+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:25}:"]"
129+
130+
;*(ptrrrrrr +123456)= 1;
131+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:""
132+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:18}:"["
133+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:25}:"]"
134+
135+
;*(ptrrrrrr+ 123456)= 1;
136+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:""
137+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:18}:"["
138+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:25}:"]"
139+
140+
;*(ptrrrrrr+123456 )= 1;
141+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:""
142+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:15}:"["
143+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:21-[[@LINE-3]]:25}:"]"
144+
145+
;*(ptrrrrrr+123456) = 1;
146+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:""
147+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:15}:"["
148+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:21-[[@LINE-3]]:22}:"]"
149+
}
150+
151+
void base_on_rhs() {
152+
int* ptr;
153+
*(10 + ptr) = 1;
154+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:5}:""
155+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:10}:"["
156+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:14}:"]"
157+
}
158+
159+
void many_parens() {
160+
int* ptr;
161+
*(( (10 + ptr)) ) = 1;
162+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:""
163+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:13}:"["
164+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:16-[[@LINE-3]]:20}:"]"
165+
}
166+
167+
void lvaue_to_rvalue() {
168+
int * ptr;
169+
int tmp = *(ptr + 10);
170+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:15}:""
171+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:21}:"["
172+
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:24}:"]"
173+
}
174+
175+
// Fixits emitted for the cases below would be incorrect.
176+
// CHECK-NOT: fix-it:
177+
// Array subsctipt opertor of std::span accepts unsigned integer.
178+
void negative() {
179+
int* ptr;
180+
*(ptr + -5) = 1; // skip
181+
}
182+
183+
void subtraction() {
184+
int* ptr;
185+
*(ptr - 5) = 1; // skip
186+
}
187+
188+
void subtraction_of_negative() {
189+
int* ptr;
190+
*(ptr - -5) = 1; // FIXME: implement fixit (uncommon case - low priority)
191+
}
192+
193+
194+
void bindingDecl(int *p, int *q) {
195+
int * a[2] = {p, q};
196+
auto [x, y] = a;
197+
198+
*(x + 1) = 1; // FIXME: deal with `BindingDecl`s
199+
}

0 commit comments

Comments
 (0)