Skip to content

Commit

Permalink
[-Wunsafe-buffer-usage] Warning for unsafe invocation of span::data (#…
Browse files Browse the repository at this point in the history
…75650)

…-Wunsafe-buffer-usage,

there maybe accidental re-introduction of new OutOfBound accesses into
the code bases. One such case is invoking span::data() method on a span
variable to retrieve a pointer, which is then cast to a larger type and
dereferenced. Such dereferences can introduce OutOfBound accesses.

To address this, a new WarningGadget is being introduced to warn against
such invocations.

---------

Co-authored-by: MalavikaSamak <malavika2@apple.com>
  • Loading branch information
malavikasamak and MalavikaSamak committed Jan 2, 2024
1 parent e32b1d1 commit 7122f55
Show file tree
Hide file tree
Showing 6 changed files with 182 additions and 8 deletions.
2 changes: 1 addition & 1 deletion clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ class UnsafeBufferUsageHandler {

/// Invoked when an unsafe operation over raw pointers is found.
virtual void handleUnsafeOperation(const Stmt *Operation,
bool IsRelatedToDecl) = 0;
bool IsRelatedToDecl, ASTContext &Ctx) = 0;

/// Invoked when a fix is suggested against a variable. This function groups
/// all variables that must be fixed together (i.e their types must be changed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ WARNING_GADGET(Decrement)
WARNING_GADGET(ArraySubscript)
WARNING_GADGET(PointerArithmetic)
WARNING_GADGET(UnsafeBufferUsageAttr)
WARNING_GADGET(DataInvocation)
FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified Lvalue Context
FIXABLE_GADGET(DerefSimplePtrArithFixable)
FIXABLE_GADGET(PointerDereference)
Expand Down
2 changes: 1 addition & 1 deletion clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -12064,7 +12064,7 @@ def warn_unsafe_buffer_variable : Warning<
InGroup<UnsafeBufferUsage>, DefaultIgnore;
def warn_unsafe_buffer_operation : Warning<
"%select{unsafe pointer operation|unsafe pointer arithmetic|"
"unsafe buffer access|function introduces unsafe buffer manipulation}0">,
"unsafe buffer access|function introduces unsafe buffer manipulation|unsafe invocation of span::data}0">,
InGroup<UnsafeBufferUsage>, DefaultIgnore;
def note_unsafe_buffer_operation : Note<
"used%select{| in pointer arithmetic| in buffer access}0 here">;
Expand Down
38 changes: 34 additions & 4 deletions clang/lib/Analysis/UnsafeBufferUsage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -721,6 +721,34 @@ class UnsafeBufferUsageAttrGadget : public WarningGadget {
DeclUseList getClaimedVarUseSites() const override { return {}; }
};

// Warning gadget for unsafe invocation of span::data method.
// Triggers when the pointer returned by the invocation is immediately
// cast to a larger type.

class DataInvocationGadget : public WarningGadget {
constexpr static const char *const OpTag = "data_invocation_expr";
const ExplicitCastExpr *Op;

public:
DataInvocationGadget(const MatchFinder::MatchResult &Result)
: WarningGadget(Kind::DataInvocation),
Op(Result.Nodes.getNodeAs<ExplicitCastExpr>(OpTag)) {}

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

static Matcher matcher() {
return stmt(
explicitCastExpr(has(cxxMemberCallExpr(callee(cxxMethodDecl(
hasName("data"), ofClass(hasName("std::span")))))))
.bind(OpTag));
}
const Stmt *getBaseStmt() const override { return Op; }

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

// Represents expressions of the form `DRE[*]` in the Unspecified Lvalue
// Context (see `isInUnspecifiedLvalueContext`).
// Note here `[]` is the built-in subscript operator.
Expand Down Expand Up @@ -2657,8 +2685,8 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
// every problematic operation and consider it done. No need to deal
// with fixable gadgets, no need to group operations by variable.
for (const auto &G : WarningGadgets) {
Handler.handleUnsafeOperation(G->getBaseStmt(),
/*IsRelatedToDecl=*/false);
Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false,
D->getASTContext());
}

// This return guarantees that most of the machine doesn't run when
Expand Down Expand Up @@ -2893,7 +2921,8 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
Tracker, Handler, VarGrpMgr);

for (const auto &G : UnsafeOps.noVar) {
Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false);
Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false,
D->getASTContext());
}

for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) {
Expand All @@ -2904,7 +2933,8 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
: FixItList{},
D);
for (const auto &G : WarningGadgets) {
Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true);
Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true,
D->getASTContext());
}
}
}
16 changes: 14 additions & 2 deletions clang/lib/Sema/AnalysisBasedWarnings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2226,8 +2226,8 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
UnsafeBufferUsageReporter(Sema &S, bool SuggestSuggestions)
: S(S), SuggestSuggestions(SuggestSuggestions) {}

void handleUnsafeOperation(const Stmt *Operation,
bool IsRelatedToDecl) override {
void handleUnsafeOperation(const Stmt *Operation, bool IsRelatedToDecl,
ASTContext &Ctx) override {
SourceLocation Loc;
SourceRange Range;
unsigned MsgParam = 0;
Expand Down Expand Up @@ -2261,6 +2261,18 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
// note_unsafe_buffer_operation doesn't have this mode yet.
assert(!IsRelatedToDecl && "Not implemented yet!");
MsgParam = 3;
} else if (const auto *ECE = dyn_cast<ExplicitCastExpr>(Operation)) {
QualType destType = ECE->getType();
const uint64_t dSize =
Ctx.getTypeSize(destType.getTypePtr()->getPointeeType());
if (const auto *CE = dyn_cast<CXXMemberCallExpr>(ECE->getSubExpr())) {
QualType srcType = CE->getType();
const uint64_t sSize =
Ctx.getTypeSize(srcType.getTypePtr()->getPointeeType());
if (sSize >= dSize)
return;
}
MsgParam = 4;
}
Loc = Operation->getBeginLoc();
Range = Operation->getSourceRange();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
// RUN: -fsafe-buffer-usage-suggestions \
// RUN: -fblocks -include %s -verify %s

// RUN: %clang -x c++ -frtti -fsyntax-only -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
// RUN: %clang_cc1 -std=c++11 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
// RUN: %clang_cc1 -std=c++20 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
// CHECK-NOT: [-Wunsafe-buffer-usage]

#ifndef INCLUDED
#define INCLUDED
#pragma clang system_header

// no spanification warnings for system headers
#else

namespace std {
class type_info;
class bad_cast;
class bad_typeid;
}
using size_t = __typeof(sizeof(int));
void *malloc(size_t);

void foo(int v) {
}

void foo(int *p){}

namespace std{
template <typename T> class span {

T *elements;

span(T *, unsigned){}

public:

constexpr span<T> subspan(size_t offset, size_t count) const {
return span<T> (elements+offset, count); // expected-warning{{unsafe pointer arithmetic}}
}

constexpr T* data() const noexcept {
return elements;
}


constexpr T* hello() const noexcept {
return elements;
}
};

template <typename T> class span_duplicate {
span_duplicate(T *, unsigned){}

T array[10];

public:

T* data() {
return array;
}

};
}

using namespace std;

class A {
int a, b, c;
};

class B {
int a, b, c;
};

struct Base {
virtual ~Base() = default;
};

struct Derived: Base {
int d;
};

void cast_without_data(int *ptr) {
A *a = (A*) ptr;
float *p = (float*) ptr;
}

void warned_patterns(std::span<int> span_ptr, std::span<Base> base_span, span<int> span_without_qual) {
A *a1 = (A*)span_ptr.data(); // expected-warning{{unsafe invocation of span::data}}
a1 = (A*)span_ptr.data(); // expected-warning{{unsafe invocation of span::data}}

A *a2 = (A*) span_without_qual.data(); // expected-warning{{unsafe invocation of span::data}}

// TODO:: Should we warn when we cast from base to derived type?
Derived *b = dynamic_cast<Derived*> (base_span.data());// expected-warning{{unsafe invocation of span::data}}

// TODO:: This pattern is safe. We can add special handling for it, if we decide this
// is the recommended fixit for the unsafe invocations.
A *a3 = (A*)span_ptr.subspan(0, sizeof(A)).data(); // expected-warning{{unsafe invocation of span::data}}
}

void not_warned_patterns(std::span<A> span_ptr, std::span<Base> base_span) {
int *p = (int*) span_ptr.data(); // Cast to a smaller type

B *b = (B*) span_ptr.data(); // Cast to a type of same size.

p = (int*) span_ptr.data();
A *a = (A*) span_ptr.hello(); // Invoking other methods.
}

// We do not want to warn about other types
void other_classes(std::span_duplicate<int> span_ptr) {
int *p;
A *a = (A*)span_ptr.data();
a = (A*)span_ptr.data();
}

// Potential source for false negatives

A false_negatives(std::span<int> span_pt, span<A> span_A) {
int *ptr = span_pt.data();

A *a1 = (A*)ptr; //TODO: We want to warn here eventually.

A *a2= span_A.data();
return *a2; // TODO: Can cause OOB if span_pt is empty

}
#endif

0 comments on commit 7122f55

Please sign in to comment.