Skip to content

Commit

Permalink
[-Wunsafe-buffer-usage] Add Fixable for simple pointer dereference
Browse files Browse the repository at this point in the history
This patch introduces PointerDereferenceGadget, a FixableGadget that emits
fixits to handle cases where a pointer that is identified as unsafe is
dereferenced. The current implementation only handles cases where the strategy
is to change the type of the raw pointer to std::span. The fixit for this
strategy is to fetch the first element from the corresponding span instance.

For example for the code below, the PointerDereferenceGadget emits a fixit for
S3 (S1, S2 are to be handled by other gadgets):

  S1: int *ptr = new int[10];
  S2: int val1 = ptr[k]; // Unsafe operation
  S3: int val2 = *ptr; => Fixit: int val2 = ptr[0];

Differential revision: https://reviews.llvm.org/D143206
  • Loading branch information
MalavikaSamak committed Mar 22, 2023
1 parent 201fdef commit e7596a9
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ WARNING_GADGET(PointerArithmetic)
WARNING_GADGET(UnsafeBufferUsageAttr)
FIXABLE_GADGET(ULCArraySubscript)
FIXABLE_GADGET(DerefSimplePtrArithFixable)
FIXABLE_GADGET(PointerDereference)

#undef FIXABLE_GADGET
#undef WARNING_GADGET
Expand Down
70 changes: 70 additions & 0 deletions clang/lib/Analysis/UnsafeBufferUsage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,45 @@ class ULCArraySubscriptGadget : public FixableGadget {
return {};
}
};

class PointerDereferenceGadget : public FixableGadget {
static constexpr const char *const BaseDeclRefExprTag = "BaseDRE";
static constexpr const char *const OperatorTag = "op";

const DeclRefExpr *BaseDeclRefExpr = nullptr;
const UnaryOperator *Op = nullptr;

public:
PointerDereferenceGadget(const MatchFinder::MatchResult &Result)
: FixableGadget(Kind::PointerDereference),
BaseDeclRefExpr(
Result.Nodes.getNodeAs<DeclRefExpr>(BaseDeclRefExprTag)),
Op(Result.Nodes.getNodeAs<UnaryOperator>(OperatorTag)) {}

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

static Matcher matcher() {
auto Target =
unaryOperator(
hasOperatorName("*"),
has(expr(ignoringParenImpCasts(
declRefExpr(to(varDecl())).bind(BaseDeclRefExprTag)))))
.bind(OperatorTag);

return expr(isInUnspecifiedLvalueContext(Target));
}

DeclUseList getClaimedVarUseSites() const override {
return {BaseDeclRefExpr};
}

virtual const Stmt *getBaseStmt() const final { return Op; }

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

} // namespace

namespace {
Expand Down Expand Up @@ -914,6 +953,37 @@ DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const {
return std::nullopt; // something wrong or unsupported, give up
}

std::optional<FixItList>
PointerDereferenceGadget::getFixits(const Strategy &S) const {
const VarDecl *VD = cast<VarDecl>(BaseDeclRefExpr->getDecl());
switch (S.lookup(VD)) {
case Strategy::Kind::Span: {
ASTContext &Ctx = VD->getASTContext();
SourceManager &SM = Ctx.getSourceManager();
// Required changes: *(ptr); => (ptr[0]); and *ptr; => ptr[0]
// Deletes the *operand
CharSourceRange derefRange = clang::CharSourceRange::getCharRange(
Op->getBeginLoc(), Op->getBeginLoc().getLocWithOffset(1));
// Inserts the [0]
std::optional<SourceLocation> endOfOperand =
getEndCharLoc(BaseDeclRefExpr, SM, Ctx.getLangOpts());
if (endOfOperand) {
return FixItList{{FixItHint::CreateRemoval(derefRange),
FixItHint::CreateInsertion(
endOfOperand.value().getLocWithOffset(1), "[0]")}};
}
}
case Strategy::Kind::Iterator:
case Strategy::Kind::Array:
case Strategy::Kind::Vector:
llvm_unreachable("Strategy not implemented yet!");
case Strategy::Kind::Wontfix:
llvm_unreachable("Invalid strategy!");
}

return std::nullopt;
}

// 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s

void basic_dereference() {
int tmp;
auto p = new int[10];
// 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]]:23-[[@LINE-3]]:23}:", 10}"
tmp = p[5];
int val = *p;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:""
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"[0]"
}

int return_method() {
auto p = new int[10];
// 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]]:23-[[@LINE-3]]:23}:", 10}"
int tmp = p[5];
return *p;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:11}:""
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"[0]"
}

void foo(int v) {
}

void method_invocation() {
auto p = new int[10];
// 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]]:23-[[@LINE-3]]:23}:", 10}"

int tmp = p[5];

foo(*p);
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:8}:""
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:9-[[@LINE-2]]:9}:"[0]"
}

void binary_operation() {
auto p = new int[10];
// 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]]:23-[[@LINE-3]]:23}:", 10}"

int tmp = p[5];

int k = *p + 20;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:12}:""
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"[0]"

}

0 comments on commit e7596a9

Please sign in to comment.