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

ziqingluo-90 · 2025-02-06T21:09:43Z

Add support of

overloaded subscript operator of hardened view/containers, and
calls to const member functions of 0 arguments

Now it can match patterns like:

std::span<char> D, S;
std::array<std::span<char>, 10> A;

memcpy(D.first(S.size()).data(), S.data(), S.size());
memcpy(D.first(A[x].size()).data(), A[x].data(), A[x].size());

(rdar://143986251)

…ts to counted_by parameters Add support of - overloaded subscript operator of hardened view/containers, and - calls to const member functions of 0 arguments Now it can match patterns like: ``` std::span<char> D, S; std::array<std::span<char>, 10> A; memcpy(D.first(S.size()).data(), S.data(), S.size()); memcpy(D.first(A[x].size()).data(), A[x].data(), A[x].size()); ``` (rdar://143986251)

ziqingluo-90 · 2025-02-06T21:12:26Z

CC: @jkorous-apple @dtarditi

ziqingluo-90 · 2025-02-06T21:12:40Z

@swift-ci test
@swift-ci test llvm

patrykstefanski · 2025-02-07T20:48:39Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

+               Visit(SelfOpCall->getArg(1), OtherOpCall->getArg(1));
+      }
+    return false;
+  }


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?

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

patrykstefanski · 2025-02-07T20:51:02Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

+    if (MD->isConst() && MD->param_empty()) {
+      if (auto *OtherCall =
+              dyn_cast<CXXMemberCallExpr>(Other->IgnoreParenImpCasts())) {
+        if (OtherCall->getMethodDecl() == MD)


Is the pointer MD and OtherCall->getMethodDecl() different if they are in different template instances?

They should be different. I will add a test for it.

hmm... having an integrated test for the case where only methods are different is actually difficult. Because if methods are from different template instances, the implicit objects cannot be identical. Eventually, the test does not tell which condition fails the compatibility checking.

patrykstefanski · 2025-02-07T21:00:04Z

clang/test/SemaCXX/warn-unsafe-buffer-usage-count-attributed-pointer-argument.cpp

  const T *data() const noexcept;
  T *data() noexcept;
  size_t size() const noexcept;
+  T& operator[](size_t idx) const {return *data_ptr;}


I think just a declaration without the implementation would be fine and consistent with above declarations. Then, you could remove T * data_ptr above.

const operator[] should return const T &. You probably can drop const from operator[] to make the tests work.

Nit: clang-format T& operator[] -> T &operator[].

It seems to require a definition when the template parameter T has no linkage: https://godbolt.org/z/4MP4afqq5. I didn't know this before. I will fix it.

patrykstefanski · 2025-02-07T21:00:32Z

clang/test/SemaCXX/warn-unsafe-buffer-usage-count-attributed-pointer-argument.cpp

@@ -40,6 +42,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;
+  T& operator[](size_t idx) const {return *data();};


patrykstefanski · 2025-02-07T21:16:02Z

clang/test/SemaCXX/warn-unsafe-buffer-usage-count-attributed-pointer-argument.cpp

+    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}}


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

patrykstefanski · 2025-02-07T21:28:30Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

+    if (const auto *OtherME =
+            dyn_cast<MemberExpr>(Other->IgnoreParenImpCasts())) {
+      return Self->getMemberDecl() == OtherME->getMemberDecl() &&
+             Visit(Self->getBase(), OtherME->getBase());


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.

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

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?

I see what you mean. I agree.

ziqingluo-90 · 2025-02-11T01:00:24Z

@swift-ci test
@swift-ci test llvm

patrykstefanski · 2025-02-11T22:07:05Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

+
+    if (auto *MD = dyn_cast<CXXMethodDecl>(SelfOpCall->getCalleeDecl()))
+      if (!MD->isConst())
+        return false;


If I understand correctly, we check MD->isConst() to ensure that both calls return the same value, and thus we can assume that the pattern is always safe.

Can dyn_cast<CXXMethodDecl>(SelfOpCall->getCalleeDecl()) be nullptr? If so, we skip the MD->isConst() check and we might consider as safe something we don't want. Maybe we can change this to:

const auto *MD = dyn_cast<CXXMethodDecl>(SelfOpCall->getCalleeDecl()); if (!MD || !MD->isConst()) return false;

Thanks for the catch. It is my bad code! Yeah, I think it should be in the form you suggested.
I just learned that operator[] can be static: https://en.cppreference.com/w/cpp/language/operators.
So we do need the dyn_cast.

#10156) * [-Wunsafe-buffer-usage][BoundsSafety] Extend safe patterns of arguments to counted_by parameters (#9971) Add support of - overloaded subscript operator of hardened view/containers, and - calls to const member functions of 0 arguments Now it can match patterns like: ``` std::span<char> D, S; std::array<std::span<char>, 10> A; memcpy(D.first(S.size()).data(), S.data(), S.size()); memcpy(D.first(A[x].size()).data(), A[x].data(), A[x].size()); ``` (rdar://143986251) * [Safe Buffers][BoundsSafety] Fix a bug in the interop analysis that can cause infinite loops The interop analysis substitutes formal parameters to arguments in AST visiting. Conceptually, the substitution should be done exactly once. But the previous implementation never checked if the substitution has happened. In cases where the argument contains DREs referring to the Decl of the corresponding formal parameter, the analysis enters an infinite loop. A typical example is the following. ``` void f(int * __counted_by(n) p, size_t n) { f(p, n); } ``` This commit fixes the issue. (rdar://145705060) (cherry picked from commit 31f72f7)

ziqingluo-90 requested review from rapidsna, malavikasamak, t-rasmud, hnrklssn and patrykstefanski February 6, 2025 21:09

patrykstefanski reviewed Feb 7, 2025

View reviewed changes

ziqingluo-90 added 2 commits February 10, 2025 15:17

Address comments

e1517f2

Remove 'isSameMemberBase'

78f9454

patrykstefanski approved these changes Feb 11, 2025

View reviewed changes

Fix a bad code

0300771

ziqingluo-90 merged commit 49f126f into swiftlang:next Feb 12, 2025

ziqingluo-90 deleted the dev/ziqing/PR-143986251 branch February 25, 2025 19:35

ziqingluo-90 mentioned this pull request Mar 4, 2025

Cherrypick for 143986251 & 145705060 #10156

Merged

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

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

Uh oh!

Conversation

ziqingluo-90 commented Feb 6, 2025

Uh oh!

ziqingluo-90 commented Feb 6, 2025

Uh oh!

ziqingluo-90 commented Feb 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ziqingluo-90 Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ziqingluo-90 commented Feb 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ziqingluo-90 Feb 10, 2025 •

edited

Loading