Skip to content

Commit

Permalink
[-Wunsafe-buffer-usage] Handle pointer initializations for grouping r…
Browse files Browse the repository at this point in the history
…elated variables

Differential Revision: https://reviews.llvm.org/D150489
  • Loading branch information
t-rasmud committed Jun 21, 2023
1 parent 1df10f1 commit db3dced
Show file tree
Hide file tree
Showing 7 changed files with 252 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ FIXABLE_GADGET(UPCAddressofArraySubscript) // '&DRE[any]' in an Unspecified Poin
FIXABLE_GADGET(UPCStandalonePointer)
FIXABLE_GADGET(UPCPreIncrement) // '++Ptr' in an Unspecified Pointer Context
FIXABLE_GADGET(PointerAssignment)
FIXABLE_GADGET(PointerInit)

#undef FIXABLE_GADGET
#undef WARNING_GADGET
Expand Down
84 changes: 72 additions & 12 deletions clang/lib/Analysis/UnsafeBufferUsage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -533,23 +533,66 @@ class PointerArithmeticGadget : public WarningGadget {
// FIXME: this gadge will need a fix-it
};

/// A pointer initialization expression of the form:
/// \code
/// int *p = q;
/// \endcode
class PointerInitGadget : public FixableGadget {
private:
static constexpr const char *const PointerInitLHSTag = "ptrInitLHS";
static constexpr const char *const PointerInitRHSTag = "ptrInitRHS";
const VarDecl * PtrInitLHS; // the LHS pointer expression in `PI`
const DeclRefExpr * PtrInitRHS; // the RHS pointer expression in `PI`

public:
PointerInitGadget(const MatchFinder::MatchResult &Result)
: FixableGadget(Kind::PointerInit),
PtrInitLHS(Result.Nodes.getNodeAs<VarDecl>(PointerInitLHSTag)),
PtrInitRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerInitRHSTag)) {}

static bool classof(const Gadget *G) {
return G->getKind() == Kind::PointerInit;
}

static Matcher matcher() {
auto PtrInitStmt = declStmt(hasSingleDecl(varDecl(
hasInitializer(ignoringImpCasts(declRefExpr(
hasPointerType()).
bind(PointerInitRHSTag)))).
bind(PointerInitLHSTag)));

return stmt(PtrInitStmt);
}

virtual std::optional<FixItList> getFixits(const Strategy &S) const override;

virtual const Stmt *getBaseStmt() const override { return nullptr; }

virtual DeclUseList getClaimedVarUseSites() const override {
return DeclUseList{PtrInitRHS};
}

virtual std::optional<std::pair<const VarDecl *, const VarDecl *>>
getStrategyImplications() const override {
return std::make_pair(PtrInitLHS,
cast<VarDecl>(PtrInitRHS->getDecl()));
}
};

/// A pointer assignment expression of the form:
/// \code
/// p = q;
/// \endcode
class PointerAssignmentGadget : public FixableGadget {
private:
static constexpr const char *const PointerAssignmentTag = "ptrAssign";
static constexpr const char *const PointerAssignLHSTag = "ptrLHS";
static constexpr const char *const PointerAssignRHSTag = "ptrRHS";
const BinaryOperator *PA; // pointer arithmetic expression
const DeclRefExpr * PtrLHS; // the LHS pointer expression in `PA`
const DeclRefExpr * PtrRHS; // the RHS pointer expression in `PA`

public:
PointerAssignmentGadget(const MatchFinder::MatchResult &Result)
: FixableGadget(Kind::PointerAssignment),
PA(Result.Nodes.getNodeAs<BinaryOperator>(PointerAssignmentTag)),
PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)),
PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {}

Expand All @@ -566,13 +609,12 @@ class PointerAssignmentGadget : public FixableGadget {
to(varDecl())).
bind(PointerAssignLHSTag))));

//FIXME: Handle declarations at assignments
return stmt(isInUnspecifiedUntypedContext(PtrAssignExpr));
}

virtual std::optional<FixItList> getFixits(const Strategy &S) const override;

virtual const Stmt *getBaseStmt() const override { return PA; }
virtual const Stmt *getBaseStmt() const override { return nullptr; }

virtual DeclUseList getClaimedVarUseSites() const override {
return DeclUseList{PtrLHS, PtrRHS};
Expand Down Expand Up @@ -769,8 +811,8 @@ class UPCAddressofArraySubscriptGadget : public FixableGadget {

namespace {
// An auxiliary tracking facility for the fixit analysis. It helps connect
// declarations to its and make sure we've covered all uses with our analysis
// before we try to fix the declaration.
// declarations to its uses and make sure we've covered all uses with our
// analysis before we try to fix the declaration.
class DeclUseTracker {
using UseSetTy = SmallSet<const DeclRefExpr *, 16>;
using DefMapTy = DenseMap<const VarDecl *, const DeclStmt *>;
Expand Down Expand Up @@ -1174,6 +1216,24 @@ PointerAssignmentGadget::getFixits(const Strategy &S) const {
return std::nullopt;
}

std::optional<FixItList>
PointerInitGadget::getFixits(const Strategy &S) const {
const auto *LeftVD = PtrInitLHS;
const auto *RightVD = cast<VarDecl>(PtrInitRHS->getDecl());
switch (S.lookup(LeftVD)) {
case Strategy::Kind::Span:
if (S.lookup(RightVD) == Strategy::Kind::Span)
return FixItList{};
return std::nullopt;
case Strategy::Kind::Wontfix:
return std::nullopt;
case Strategy::Kind::Iterator:
case Strategy::Kind::Array:
case Strategy::Kind::Vector:
llvm_unreachable("unsupported strategies for FixableGadgets");
}
return std::nullopt;
}

std::optional<FixItList>
ULCArraySubscriptGadget::getFixits(const Strategy &S) const {
Expand Down Expand Up @@ -2020,10 +2080,10 @@ static bool overlapWithMacro(const FixItList &FixIts) {
});
}

static bool impossibleToFixForVar(const FixableGadgetSets &FixablesForUnsafeVars,
static bool impossibleToFixForVar(const FixableGadgetSets &FixablesForAllVars,
const Strategy &S,
const VarDecl * Var) {
for (const auto &F : FixablesForUnsafeVars.byVar.find(Var)->second) {
for (const auto &F : FixablesForAllVars.byVar.find(Var)->second) {
std::optional<FixItList> Fixits = F->getFixits(S);
if (!Fixits) {
return true;
Expand All @@ -2033,13 +2093,13 @@ static bool impossibleToFixForVar(const FixableGadgetSets &FixablesForUnsafeVars
}

static std::map<const VarDecl *, FixItList>
getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S,
getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
ASTContext &Ctx,
/* The function decl under analysis */ const Decl *D,
const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler,
const DefMapTy &VarGrpMap) {
std::map<const VarDecl *, FixItList> FixItsForVariable;
for (const auto &[VD, Fixables] : FixablesForUnsafeVars.byVar) {
for (const auto &[VD, Fixables] : FixablesForAllVars.byVar) {
FixItsForVariable[VD] =
fixVariable(VD, S.lookup(VD), D, Tracker, Ctx, Handler);
// If we fail to produce Fix-It for the declaration we have to skip the
Expand Down Expand Up @@ -2073,7 +2133,7 @@ getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S,
if (V == VD) {
continue;
}
if (impossibleToFixForVar(FixablesForUnsafeVars, S, V)) {
if (impossibleToFixForVar(FixablesForAllVars, S, V)) {
ImpossibleToFix = true;
break;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
// RUN: -fsafe-buffer-usage-suggestions \
// RUN: -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s

void lhs_span_multi_assign() {
int *a = new int[2];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> a"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 2}"
int *b = a;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> b"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", <# placeholder #>}"
int *c = b;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> c"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", <# placeholder #>}"
int *d = c;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> d"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", <# placeholder #>}"
int tmp = d[2]; // expected-note{{used in buffer access here}}
}

void rhs_span1() {
int *q = new int[12];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> q"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 12}"
int *p = q;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", <# placeholder #>}"
p[5] = 10;
int *r = q;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> r"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", <# placeholder #>}"
r[10] = 5; // expected-note{{used in buffer access here}}
}

void rhs_span2() {
int *q = new int[6];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> q"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 6}"
int *p = q;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", <# placeholder #>}"
p[5] = 10; // expected-note{{used in buffer access here}}
}

void rhs_span3() {
int *q = new int[6];
int *p = q; // expected-warning{{'p' is an unsafe pointer used for buffer access}}
p[5] = 10; // expected-note{{used in buffer access here}}
int *r = q;
}

void test_grouping() {
int *z = new int[8];
int tmp;
int *y = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> y"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
tmp = y[5];

int *x = new int[10];
x = y;

int *w = z;
}

void test_crash() {
int *r = new int[8];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> r"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 8}"
int *q = r;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> q"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", <# placeholder #>}"
int *p;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span<int> p"
p = q;
int tmp = p[9]; // expected-note{{used in buffer access here}}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fsafe-buffer-usage-suggestions -verify %s

void lhs_span_multi_assign() {
int *a = new int[2];
int *b = a;
int *c = b;
int *d = c; // expected-warning{{'d' is an unsafe pointer used for buffer access}} expected-note-re{{{{^change type of 'd' to 'std::span' to preserve bounds information, and change ('a', 'b', and 'c'|'a', 'c', and 'b'|'b', 'a', and 'c'|'b', 'c', and 'a'|'c', 'a', and 'b'|'c', 'b', and 'a') to 'std::span' to propagate bounds information between them$}}}}
int tmp = d[2]; // expected-note{{used in buffer access here}}
}

void rhs_span1() {
int *q = new int[12];
int *p = q; // expected-warning{{'p' is an unsafe pointer used for buffer access}} expected-note-re{{{{^change type of 'p' to 'std::span' to preserve bounds information, and change ('q' and 'r'|'r' and 'q') to 'std::span' to propagate bounds information between them$}}}}
p[5] = 10; // expected-note{{used in buffer access here}}
int *r = q; // expected-warning{{'r' is an unsafe pointer used for buffer access}} expected-note-re{{{{^change type of 'r' to 'std::span' to preserve bounds information, and change ('p' and 'q'|'q' and 'p') to 'std::span' to propagate bounds information between them$}}}}
r[10] = 5; // expected-note{{used in buffer access here}}
}

void rhs_span2() {
int *q = new int[6];
int *p = q; // expected-warning{{'p' is an unsafe pointer used for buffer access}} expected-note-re{{{{^change type of 'p' to 'std::span' to preserve bounds information, and change 'q' to 'std::span' to propagate bounds information between them$}}}}
p[5] = 10; // expected-note{{used in buffer access here}}
}

// FIXME: Suggest fixits for p, q, and r since span a valid fixit for r.
void rhs_span3() {
int *q = new int[6];
int *p = q; // expected-warning{{'p' is an unsafe pointer used for buffer access}}
p[5] = 10; // expected-note{{used in buffer access here}}
int *r = q;
}

void test_grouping() {
int *z = new int[8];
int tmp;
int *y = new int[10]; // expected-warning{{'y' is an unsafe pointer used for buffer access}} expected-note-re{{{{^change type of 'y' to 'std::span' to preserve bounds information$}}}}
tmp = y[5]; // expected-note{{used in buffer access here}}

int *x = new int[10];
x = y;

int *w = z;
}

void test_crash() {
int *r = new int[8];
int *q = r;
int *p; // expected-warning{{'p' is an unsafe pointer used for buffer access}} expected-note-re{{{{^change type of 'p' to 'std::span' to preserve bounds information, and change ('r' and 'q'|'q' and 'r') to 'std::span' to propagate bounds information between them$}}}}
p = q;
int tmp = p[9]; // expected-note{{used in buffer access here}}
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,21 @@ void uuc_if_body1(bool flag) {
p[5] = 4;
}

void uuc_if_body2_ptr_init(bool flag) {
int *r = new int[7];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> r"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 7}"
if (flag) {
} else {
int* p = r;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:5-[[@LINE-1]]:13}:"std::span<int> p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:14}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:", <# placeholder #>}"
p[5] = 4;
}
}

void uuc_if_cond_no_unsafe_op() {
int *r = new int[7];
int *p = new int[4];
Expand All @@ -58,32 +73,32 @@ void uuc_if_cond_no_unsafe_op() {

void uuc_if_cond_unsafe_op() {
int *r = new int[7];
int *p = new int[4]; //expected-warning{{'p' is an unsafe pointer used for buffer access}}
int *p = new int[4];
if ((p = r)) {
p[3] = 2; // expected-note{{used in buffer access here}}
p[3] = 2;
}
}

void uuc_if_cond_unsafe_op1() {
int *r = new int[7]; // expected-warning{{'r' is an unsafe pointer used for buffer access}}
int *r = new int[7];
int *p = new int[4];
if ((p = r)) {
r[3] = 2; // expected-note{{used in buffer access here}}
r[3] = 2;
}
}

void uuc_if_cond_unsafe_op2() {
int *r = new int[7]; // expected-warning{{'r' is an unsafe pointer used for buffer access}}
int *p = new int[4]; // expected-warning{{'p' is an unsafe pointer used for buffer access}}
int *r = new int[7];
int *p = new int[4];
if ((p = r)) {
r[3] = 2; // expected-note{{used in buffer access here}}
r[3] = 2;
}
p[4] = 6; // expected-note{{used in buffer access here}}
p[4] = 6;
}

void uuc_call1() {
int *w = new int[4]; // expected-warning{{'w' is an unsafe pointer used for buffer access}}
int *w = new int[4];
int *y = new int[4];
bar(w = y);
w[5] = 0; // expected-note{{used in buffer access here}}
w[5] = 0;
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@ void uuc_if_body2(bool flag) {
p[5] = 4; // expected-note{{used in buffer access here}}
}

void uuc_if_body2_ptr_init(bool flag) {
int *r = new int[7];
if (flag) {
} else {
int* p = r; // expected-warning{{'p' is an unsafe pointer used for buffer access}} // expected-note-re{{{{^change type of 'p' to 'std::span' to preserve bounds information, and change 'r' to 'std::span' to propagate bounds information between them$}}}}
p[5] = 4; // expected-note{{used in buffer access here}}
}
}

void uuc_if_cond_no_unsafe_op() {
int *r = new int[7];
int *p = new int[4];
Expand Down

0 comments on commit db3dced

Please sign in to comment.