Skip to content

[-Wunsafe-buffer-usage][BoundsSafety] Extend safe patterns of arguments to counted_by parameters #9971

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 12, 2025
Merged

[-Wunsafe-buffer-usage][BoundsSafety] Extend safe patterns of arguments to counted_by parameters #9971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c61aa03
[-Wunsafe-buffer-usage][BoundsSafety] Extend safe patterns of argumen…
ziqingluo-90 Feb 6, 2025
e1517f2
Address comments
ziqingluo-90 Feb 10, 2025
78f9454
Remove 'isSameMemberBase'
ziqingluo-90 Feb 10, 2025
0300771
Fix a bad code
ziqingluo-90 Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 75 additions & 43 deletions clang/lib/Analysis/UnsafeBufferUsage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,48 +408,23 @@ getDependentValuesFromCall(const CountAttributedType *CAT,
return {std::move(Values)};
}

// Checks if Self and Other are the same member bases. This supports only very
// simple forms of member bases.
bool isSameMemberBase(const Expr *Self, const Expr *Other) {
for (;;) {
if (Self == Other)
return true;

const auto *SelfICE = dyn_cast<ImplicitCastExpr>(Self);
const auto *OtherICE = dyn_cast<ImplicitCastExpr>(Other);
if (SelfICE && OtherICE &&
SelfICE->getCastKind() == OtherICE->getCastKind() &&
(SelfICE->getCastKind() == CK_LValueToRValue ||
SelfICE->getCastKind() == CK_UncheckedDerivedToBase)) {
Self = SelfICE->getSubExpr();
Other = OtherICE->getSubExpr();
}

const auto *SelfDRE = dyn_cast<DeclRefExpr>(Self);
const auto *OtherDRE = dyn_cast<DeclRefExpr>(Other);
if (SelfDRE && OtherDRE)
return SelfDRE->getDecl() == OtherDRE->getDecl();

if (isa<CXXThisExpr>(Self) && isa<CXXThisExpr>(Other)) {
// `Self` and `Other` should be evaluated at the same state so `this` must
// mean the same thing for both:
return true;
}

const auto *SelfME = dyn_cast<MemberExpr>(Self);
const auto *OtherME = dyn_cast<MemberExpr>(Other);
if (!SelfME || !OtherME ||
SelfME->getMemberDecl() != OtherME->getMemberDecl()) {
return false;
}

Self = SelfME->getBase();
Other = OtherME->getBase();
}
}

// Impl of `isCompatibleWithCountExpr`. See `isCompatibleWithCountExpr` for
// document.
// high-level document.
//
// This visitor compares two expressions in a tiny subset of the language,
// including DRE of VarDecls, constants, binary operators, subscripts,
// dereferences, member accesses, and member function calls.
//
// - For constants, they are literal constants and expressions have
// compile-time constant values.
// - For a supported dereference expression, it can be either of the forms '*e'
// or '*&e', where 'e' is a supported expression.
// - For a subscript expression, it can be either an array subscript or
// overloaded subscript operator.
// - For a member function call, it must be a call to a 'const' (non-static)
// member function with zero argument. This is to ensure side-effect free.
// Other kinds of function calls are not supported, so an expression of the
// form `f(...)` is not supported.
struct CompatibleCountExprVisitor
: public ConstStmtVisitor<CompatibleCountExprVisitor, bool, const Expr *> {
using BaseVisitor =
Expand Down Expand Up @@ -523,6 +498,10 @@ struct CompatibleCountExprVisitor
return false;
}

bool VisitCXXThisExpr(const CXXThisExpr *SelfThis, const Expr *Other) {
return isa<CXXThisExpr>(Other->IgnoreParenImpCasts());
}

bool VisitDeclRefExpr(const DeclRefExpr *SelfDRE, const Expr *Other) {
const ValueDecl *SelfVD = SelfDRE->getDecl();

Expand All @@ -544,7 +523,7 @@ struct CompatibleCountExprVisitor
const auto *OtherME = dyn_cast<MemberExpr>(O);
if (MemberBase && OtherME) {
return OtherME->getMemberDecl() == SelfVD &&
isSameMemberBase(OtherME->getBase(), MemberBase);
Visit(OtherME->getBase(), MemberBase);
}

return false;
Expand All @@ -557,7 +536,12 @@ struct CompatibleCountExprVisitor
return true;
if (const auto *DRE = dyn_cast<DeclRefExpr>(Other->IgnoreParenImpCasts()))
return MemberBase && Self->getMemberDecl() == DRE->getDecl() &&
isSameMemberBase(Self->getBase(), MemberBase);
Visit(Self->getBase(), MemberBase);
if (const auto *OtherME =
dyn_cast<MemberExpr>(Other->IgnoreParenImpCasts())) {
return Self->getMemberDecl() == OtherME->getMemberDecl() &&
Visit(Self->getBase(), OtherME->getBase());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work if the member base is CXXThisExpr? For example:

struct Foo {
  std::array<int, 10> A;

  void foo(std::span<int> S) {
    cb_cint(S.first(A[5]).data(), A[5]);
  }
};

We probably can implement VisitCXXThisExpr to make it work. Moreover, if this works, we might be able to remove isSameMemberBase.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I should have added tests of this case. I think it works. But why we can remove isSameMemberBase?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using here Visit(Self->getBase(), OtherME->getBase()) to check if the base is the same. Couldn't we replace isSameMemberBase(Self->getBase(), MemberBase) above by Visit(Self->getBase(), MemberBase) to achieve the same?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. I agree.

}
return false;
}

Expand Down Expand Up @@ -587,6 +571,54 @@ struct CompatibleCountExprVisitor

return false;
}

// Support any overloaded operator[] so long as it is a const method.
bool VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *SelfOpCall,
const Expr *Other) {
if (SelfOpCall->getOperator() != OverloadedOperatorKind::OO_Subscript)
return false;

const auto *MD = dyn_cast<CXXMethodDecl>(SelfOpCall->getCalleeDecl());

if (!MD || !MD->isConst())
return false;
if (const auto *OtherOpCall =
dyn_cast<CXXOperatorCallExpr>(Other->IgnoreParenImpCasts()))
if (SelfOpCall->getOperator() == OtherOpCall->getOperator()) {
return Visit(SelfOpCall->getArg(0), OtherOpCall->getArg(0)) &&
Visit(SelfOpCall->getArg(1), OtherOpCall->getArg(1));
}
return false;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we check for overloaded operator[] and not just any operator[]? Why do we need to check for hardened containers/view instead of any container?
Is this related to how the analysis work and gadgets are matched? Say, if we allow operator[] on a pointer here and consider the call safe, then other gadget for pointer arithmetic won't match?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right. It should be just any operator[].


// Support array/pointer subscript. Even though these operators are generally
// considered unsafe, they can be safely used on constant arrays with
// known-safe literal indexes.
bool VisitArraySubscriptExpr(const ArraySubscriptExpr *SelfAS,
const Expr *Other) {
if (const auto *OtherAS =
dyn_cast<ArraySubscriptExpr>(Other->IgnoreParenImpCasts()))
return Visit(SelfAS->getLHS(), OtherAS->getLHS()) &&
Visit(SelfAS->getRHS(), OtherAS->getRHS());
return false;
}

// Support non-static member call:
bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *SelfCall,
const Expr *Other) {
const CXXMethodDecl *MD = SelfCall->getMethodDecl();

// The callee member function must be a const function with no parameter:
if (MD->isConst() && MD->param_empty()) {
if (auto *OtherCall =
dyn_cast<CXXMemberCallExpr>(Other->IgnoreParenImpCasts())) {
return OtherCall->getMethodDecl() == MD &&
Visit(SelfCall->getImplicitObjectArgument(),
OtherCall->getImplicitObjectArgument());
}
}
return false;
}
};

// TL'DR:
Expand Down
128 changes: 126 additions & 2 deletions clang/test/SemaCXX/warn-unsafe-buffer-usage-count-attributed-pointer-argument.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@
#include <stddef.h>

namespace std {

template <typename T, size_t N>
struct array {
const T *data() const noexcept;
T *data() noexcept;
size_t size() const noexcept;
const T &operator[](const size_t idx) const;
};

template <typename CharT>
Expand Down Expand Up @@ -40,6 +40,7 @@ struct span {
span<T> first(size_t count) const noexcept;
span<T> last(size_t count) const noexcept;
span<T> subspan(size_t offset, size_t count) const noexcept;
const T &operator[](const size_t idx) const;
};

template <typename T>
Expand Down Expand Up @@ -71,7 +72,7 @@ void cb_cchar_42(const char *__counted_by(42) s);
// expected-note@+1 19{{consider using a safe container and passing '.data()' to the parameter 'p' and '.size()' to its dependent parameter 'count' or 'std::span' and passing '.first(...).data()' to the parameter 'p'}}
void cb_int(int *__counted_by(count) p, size_t count);

// expected-note@+1 11{{consider using a safe container and passing '.data()' to the parameter 'p' and '.size()' to its dependent parameter 'count' or 'std::span' and passing '.first(...).data()' to the parameter 'p'}}
// expected-note@+1 33{{consider using a safe container and passing '.data()' to the parameter 'p' and '.size()' to its dependent parameter 'count' or 'std::span' and passing '.first(...).data()' to the parameter 'p'}}
void cb_cint(const int *__counted_by(count) p, size_t count);

// expected-note@+1 10{{consider using 'std::span' and passing '.first(...).data()' to the parameter 'p'}}
Expand Down Expand Up @@ -367,6 +368,129 @@ void single_variable() {
sb_cvoid(&Arr, sizeof(decltype(Var))); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}
}

namespace test_subscript {
void foo(std::span<int> S) {
std::array<int, 10> A;

cb_cint(S.first(S[5]).data(), S[5]);
cb_cint(S.first(A[5]).data(), A[5]);
cb_cint(S.subspan(0, S[5]).data(), S[5]);
cb_cint(S.subspan(0, A[5]).data(), A[5]);
cb_cint(S.subspan(0, S[5]).data(), A[5]); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}
cb_cint(S.subspan(0, S[5]).data(), S[6]); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}

unsigned x;

cb_cint(S.first(S[x]).data(), S[x]);
cb_cint(S.first(A[x]).data(), A[x]);
cb_cint(S.subspan(0, S[x]).data(), S[x]);
cb_cint(S.subspan(0, A[x]).data(), A[x]);
cb_cint(S.first(S[x + 1]).data(), S[x + 1]);
cb_cint(S.subspan(0, A[x + 1]).data(), A[x + 1]);
cb_cint(S.first(A[A[x]]).data(), A[A[x]]);
cb_cint(S.first(A[A[x] + 1]).data(), A[A[x] + 1]);
cb_cint(S.first(S[x + 1]).data(), S[x - 1]); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}
cb_cint(S.first(A[A[x]]).data(), A[A[x] + 1]); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}

std::array<std::array<int, 10>, 10> A2d;

cb_cint(S.first(A2d[x][5]).data(), A2d[x][5]);
cb_cint(S.first(A2d[x][5]).data(), A2d[5][x]); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}
}

struct T {
std::array<int, 10> a;
} t, *tp;

struct TT {
struct T t;
struct T * tp;
} tt, *ttp;

void bar(std::span<int> S) {
unsigned x;

cb_cint(S.first(t.a[x]).data(), t.a[x]);
cb_cint(S.first(tp->a[x]).data(), tp->a[x]);
cb_cint(S.first(tt.t.a[x]).data(), tt.t.a[x]);
cb_cint(S.first(ttp->t.a[x]).data(), ttp->t.a[x]);
cb_cint(S.first(tt.tp->a[x]).data(), tt.tp->a[x]);
cb_cint(S.first(ttp->tp->a[x]).data(), ttp->tp->a[x]);
cb_cint(S.first(t.a[x]).data(), tp->a[x]); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}
cb_cint(S.first(tt.t.a[x]).data(), tt.tp->a[x]); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}
cb_cint(S.first(ttp->t.a[x]).data(), ttp->tp->a[x]); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a few tests where subscript is in a member base? For example:

    std::array<TT, 10> arr;
    cb_cint(S.first(arr[1].t.a[x]).data(), arr[1].t.a[x]);
    cb_cint(S.first(arr[1].t.a[x]).data(), arr[2].t.a[x]); // warn


int cArr[10];

cb_cint(S.first(cArr[5]).data(), cArr[5]);
cb_cint(S.first(cArr[1]).data(), cArr[sizeof(char)]);
cb_cint(S.first(cArr[5]).data(), cArr[6]); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}
cb_cint(S.first(cArr[5]).data(), cArr[sizeof(char)]);// expected-warning{{unsafe assignment to function parameter of count-attributed type}}

std::array<struct TT, 10> ttA;

cb_cint(S.first(ttA[4].t.a[x]).data(), ttA[4].t.a[x]);
cb_cint(S.first(ttA[4].tp->a[x]).data(), ttA[4].tp->a[x]);
cb_cint(S.first(ttA[3].t.a[x]).data(), ttA[4].t.a[x]);// expected-warning{{unsafe assignment to function parameter of count-attributed type}}
cb_cint(S.first(ttA[3].tp->a[x]).data(), ttA[4].tp->a[x]);// expected-warning{{unsafe assignment to function parameter of count-attributed type}}
}
} //namespace test_subscript

namespace test_member_fun_call {
struct T {
unsigned size() {return 0;} // missing 'const'
unsigned size(int x) const {return 0;} // has arguments
unsigned supported_size() const {return 0;}
} t, *tp;

void foo(std::span<int> S1, std::span<int> S2) {
cb_cint(S1.first(S2.size()).data(), S2.size());
cb_cint(S1.first(S2.size()).data(), S1.size()); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}


cb_cint(S1.first(t.supported_size()).data(), t.supported_size());
cb_cint(S1.first(tp->supported_size()).data(), tp->supported_size());
cb_cint(S1.first(t.size()).data(), t.size()); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}
cb_cint(S1.first(t.size(0)).data(), t.size(0)); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}
cb_cint(S1.first(t.supported_size()).data(), tp->supported_size()); // expected-warning{{unsafe assignment to function parameter of count-attributed}}
}
void bar(std::span<int> S) {
std::array<struct T, 10> Ts;
std::array<struct T*, 10> Tps;
unsigned x;

cb_cint(S.first(Ts[5].supported_size()).data(), Ts[5].supported_size());
cb_cint(S.first(Ts[x].supported_size()).data(), Ts[x].supported_size());
cb_cint(S.first(Tps[5]->supported_size()).data(), Tps[5]->supported_size());
cb_cint(S.first(Ts[x].supported_size()).data(), Ts[5].supported_size()); // expected-warning{{unsafe assignment to function parameter of count-attributed}}
cb_cint(S.first(Ts[x].supported_size()).data(), Tps[x]->supported_size());// expected-warning{{unsafe assignment to function parameter of count-attributed}}
}

template<typename Ty, size_t N>
struct GenT {
unsigned size() {return 0;} // missing 'const'
unsigned size(int x) const {return 0;} // has arguments
static unsigned static_size() {return 0;} // static function is not supported
unsigned supported_size() const {return 0;}

void test(std::span<int> S) {
cb_cint(S.first(supported_size()).data(), supported_size());
cb_cint(S.first(supported_size()).data(), size()); // expected-warning{{unsafe assignment to function parameter of count-attributed}}
}
};

void baz(std::span<int> S) {
GenT<int, 5> T5;
GenT<int, 10> T10;

cb_cint(S.first(T5.supported_size()).data(), T5.supported_size());
cb_cint(S.first(T5.static_size()).data(), T5.static_size()); // expected-warning{{unsafe assignment to function parameter of count-attributed}}
cb_cint(S.first(GenT<int, 5>::static_size()).data(), GenT<int, 5>::static_size()); // expected-warning{{unsafe assignment to function parameter of count-attributed}}
cb_cint(S.first(T5.supported_size()).data(), T10.supported_size()); // expected-warning{{unsafe assignment to function parameter of count-attributed}}
T5.test(S);
}
} // namespace test_member_fun_call

namespace test_member_access {
class Base {
public:
Expand Down