[-Wunsafe-buffer-usage] Fixits for unsafe arguments of function pointer calls #80358

jkorous-apple · 2024-02-01T22:46:30Z

depends on #80347

github-actions · 2024-02-01T22:48:52Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvmbot · 2024-02-13T22:16:48Z

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: None (jkorous-apple)

Changes

depends on #80347

Full diff: https://github.com/llvm/llvm-project/pull/80358.diff

3 Files Affected:

(modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+3-3)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp (+49-1)
(added) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp (+48)

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 3c2a6fd81b1d8f..c5a87f14bc8880 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -282,8 +282,8 @@ isInUnspecifiedPointerContext(internal::Matcher<Stmt> InnerMatcher) {
   //    (i.e., computing the distance between two pointers); or ...
 
   auto CallArgMatcher =
-      callExpr(forEachArgumentWithParam(InnerMatcher,
-                  hasPointerType() /* array also decays to pointer type*/),
+      callExpr(forEachArgumentWithParamType(InnerMatcher,
+                  isAnyPointer() /* array also decays to pointer type*/),
           unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage)))));
 
   auto CastOperandMatcher =
@@ -1876,6 +1876,7 @@ std::optional<FixItList>
 UPCStandalonePointerGadget::getFixits(const FixitStrategy &S) const {
   const auto VD = cast<VarDecl>(Node->getDecl());
   switch (S.lookup(VD)) {
+  case FixitStrategy::Kind::Array:
   case FixitStrategy::Kind::Span: {
     ASTContext &Ctx = VD->getASTContext();
     SourceManager &SM = Ctx.getSourceManager();
@@ -1890,7 +1891,6 @@ UPCStandalonePointerGadget::getFixits(const FixitStrategy &S) const {
   }
   case FixitStrategy::Kind::Wontfix:
   case FixitStrategy::Kind::Iterator:
-  case FixitStrategy::Kind::Array:
     return std::nullopt;
   case FixitStrategy::Kind::Vector:
     llvm_unreachable("unsupported strategies for FixableGadgets");
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
index ca19702c7ec300..f94072015ff87d 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
@@ -83,12 +83,27 @@ void unsafe_method_invocation_single_param() {
 
 }
 
+void unsafe_method_invocation_single_param_array() {
+  int p[32];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::array<int, 32> p"
+
+  int tmp = p[5];
+  foo(p);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:8-[[@LINE-1]]:8}:".data()"
+}
+
 void safe_method_invocation_single_param() {
   int* p = new int[10];
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}-[[@LINE-1]]:{{.*}}}
   foo(p);
 }
 
+void safe_method_invocation_single_param_array() {
+  int p[10];
+  foo(p);
+  // CHECK-NO: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}-[[@LINE-1]]:{{.*}}}:".data()"
+}
+
 void unsafe_method_invocation_double_param() {
   int* p = new int[10];
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
@@ -111,6 +126,20 @@ void unsafe_method_invocation_double_param() {
   m1(q, q, 8);
 }
 
+void unsafe_method_invocation_double_param_array() {
+  int p[14];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::array<int, 14> p"
+
+  int q[40];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::array<int, 40> q"
+
+  q[5] = p[5];
+
+  m1(p, p, 10);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:".data()"
+}
+
 void unsafe_access_in_lamda() {
   int* p = new int[10];
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
@@ -177,4 +206,23 @@ void fixits_in_lambda_capture_rename() {
   };
 
   p[5] = 10;
-} 
+}
+
+bool ptr_comparison(int* ptr, unsigned idx) {
+  int arr[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::array<int, 10> arr"
+  arr[idx] = idx;
+
+  return arr > ptr;
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:13}:".data()"
+}
+
+int long long ptr_distance(int* ptr, unsigned idx) {
+  int arr[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::array<int, 10> arr"
+  arr[idx] = idx;
+
+  int long long dist = arr - ptr;
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:27-[[@LINE-1]]:27}:".data()"
+  return dist;
+}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
new file mode 100644
index 00000000000000..0459d6549fd86f
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN:            -fsafe-buffer-usage-suggestions \
+// RUN:            -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+void unsafe_array_func_ptr_call(void (*fn_ptr)(int *param)) {
+  int p[32];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::array<int, 32> p"
+
+  p[5] = 10;
+  fn_ptr(p);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:11}:".data()"
+}
+
+void unsafe_ptr_func_ptr_call(void (*fn_ptr)(int *param)) {
+  int *p;
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span<int> p"
+
+  p[5] = 10;
+  fn_ptr(p);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:11}:".data()"
+}
+
+void addr_of_unsafe_ptr_func_ptr_call(void (*fn_ptr)(int *param)) {
+  int *p;
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span<int> p"
+
+  p[5] = 10;
+  fn_ptr(&p[0]);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:15}:"p.data()"
+}
+
+void addr_of_unsafe_ptr_w_offset_func_ptr_call(void (*fn_ptr)(int *param)) {
+  int *p;
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span<int> p"
+
+  p[5] = 10;
+  fn_ptr(&p[3]);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:15}:"&p.data()[3]"
+}
+
+void preincrement_unsafe_ptr_func_ptr_call(void (*fn_ptr)(int *param)) {
+  int *p;
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span<int> p"
+
+  p[5] = 10;
+  fn_ptr(++p);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:13}:"(p = p.subspan(1)).data()"
+}

haoNoQ · 2024-02-13T23:29:42Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

-      callExpr(forEachArgumentWithParam(InnerMatcher,
-                  hasPointerType() /* array also decays to pointer type*/),
+      callExpr(forEachArgumentWithParamType(InnerMatcher,
+                  isAnyPointer() /* array also decays to pointer type*/),


Original code uses hasPointerType() which is inconvenient here because it includes the hasType() part, but another thing it was doing was hasCanonicalType() which is now missing. Are you sure it's not necessary in this case?

Removing hasCanonicalType() is intentional but not well communicated by the commit message - I'll fix that.

There's 2 different things going on and I probably could've separated this tiny change into two even-more-tiny changes.

Support for calls on function pointers - that's mostly the forEachArgumentWithParam -> forEachArgumentWithParamType.

Suport for const size arrays - that's removing the hasCanonicalType() to allow for array to pointer decay (which the comment in the snippet suggests we already expected).

Yeah but I mean, does the new code handle typedefs correctly? That's probably why hasCanonicalType() was there in the first place, to handle things like typedefs.

Yes, type sugar is fine, we have about 20 tests cases for just typedef.

From the architectural standpoint this is a definition of DRE context, it shouldn't have used hasCanonicalType in the first place and the comment /* array also decays to pointer type*/ suggests it wasn't the intention.

Handling or not handling typedefs is the responsibility of fixVariable function and perhaps individual FixableGadgets' getFixits() method.

Hmm looks like parameter types are always canonical. Probably because we need them to be the same for overloading purposes even if they're typedef'ed differently. The new code appears to be entirely equivalent to hasCanonicalType(pointerType()) or just pointerType().

Side note, isAnyPointer() only adds ObjC pointer types to the mix. We don't really care about ObjC pointers because you can't do pointer arithmetic on them. So might as well use pointerType().

Handling or not handling typedefs is the responsibility of fixVariable function and perhaps individual FixableGadgets' getFixits() method.

In many cases you need to actively skip typedefs in order to get the gadget to match at all. But it looks like, indeed, it doesn't matter in this case.

Summary of an offline discussion: Actually, the above is wrong because we're talking about the parameter type, not argument type but it also voids the concern.

…s calls Currently we ignore calls on function pointers (unlike direct calls of functions and class methods). This patch adds support for function pointers as well. The change is to simply replace use of forEachArgumentWithParam matcher in UPC gadget with forEachArgumentWithParamType. from the documentation of forEachArgumentWithParamType: /// Matches all arguments and their respective types for a \c CallExpr or /// \c CXXConstructExpr. It is very similar to \c forEachArgumentWithParam but /// it works on calls through function pointers as well. Currently the matcher also uses hasPointerType() which checks that the canonical type of an argument is pointer and won't match on arrays decayed to pointer. Replacing hasPointerType() with isAnyPointerType() which allows implicit casts allows for the arrays to be matched as well and this way we get fixits for array arguments to function pointer calls too.

…nterContext ...and turn off clang-format for the block of AST matchers.

…#80358) Currently we ignore calls on function pointers (unlike direct calls of functions and class methods). This patch adds support for function pointers as well. The change is to simply replace use of forEachArgumentWithParam matcher in UPC gadget with forEachArgumentWithParamType. from the documentation of forEachArgumentWithParamType: /// Matches all arguments and their respective types for a \c CallExpr or /// \c CXXConstructExpr. It is very similar to \c forEachArgumentWithParam but /// it works on calls through function pointers as well. Currently the matcher also uses hasPointerType() which checks that the canonical type of an argument is pointer and won't match on arrays decayed to pointer. Replacing hasPointerType() with isAnyPointerType() which allows implicit casts allows for the arrays to be matched as well and this way we get fixits for array arguments to function pointer calls too. (cherry picked from commit 4cd7616)

…e/func-ptr-calls [-Wunsafe-buffer-usage] Fixits for array args of func-ptr calls (llvm#80358)

jkorous-apple mentioned this pull request Feb 2, 2024

[-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts #80504

Merged

jkorous-apple force-pushed the cxx-safe-buffers/func-ptr-calls branch from 8788dc6 to e42e084 Compare February 13, 2024 22:16

llvmbot added clang Clang issues not falling into any other category clang:analysis labels Feb 13, 2024

jkorous-apple requested review from t-rasmud, haoNoQ, malavikasamak and ziqingluo-90 February 13, 2024 22:18

jkorous-apple force-pushed the cxx-safe-buffers/func-ptr-calls branch 2 times, most recently from d95caeb to 9bf4f71 Compare February 13, 2024 23:03

haoNoQ reviewed Feb 13, 2024

View reviewed changes

jkorous-apple added 3 commits February 13, 2024 15:38

[-Wunsafe-buffer-usage][NFC] Add tests for function pointer call fixits

44dab96

[-Wunsafe-buffer-usage][NFC] Format AST matcher in isInUnspecifiedPoi…

76a91cf

…nterContext ...and turn off clang-format for the block of AST matchers.

jkorous-apple force-pushed the cxx-safe-buffers/func-ptr-calls branch from 9bf4f71 to 76a91cf Compare February 13, 2024 23:43

haoNoQ approved these changes Feb 14, 2024

View reviewed changes

jkorous-apple merged commit 4cd7616 into llvm:main Feb 15, 2024

shahmishal added a commit to swiftlang/llvm-project that referenced this pull request Feb 15, 2024

Merge pull request #8195 from jkorous-apple/cxx-safe-buffers/integrat…

0df1cf0

…e/func-ptr-calls [-Wunsafe-buffer-usage] Fixits for array args of func-ptr calls (llvm#80358)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[-Wunsafe-buffer-usage] Fixits for unsafe arguments of function pointer calls #80358

[-Wunsafe-buffer-usage] Fixits for unsafe arguments of function pointer calls #80358

Uh oh!

jkorous-apple commented Feb 1, 2024

Uh oh!

github-actions bot commented Feb 1, 2024 •

edited

Loading

Uh oh!

llvmbot commented Feb 13, 2024 •

edited

Loading

Uh oh!

haoNoQ Feb 13, 2024

Uh oh!

jkorous-apple Feb 13, 2024

Uh oh!

haoNoQ Feb 14, 2024

Uh oh!

jkorous-apple Feb 14, 2024 •

edited

Loading

Uh oh!

haoNoQ Feb 14, 2024

Uh oh!

jkorous-apple Feb 15, 2024

Uh oh!

Uh oh!

[-Wunsafe-buffer-usage] Fixits for unsafe arguments of function pointer calls #80358

[-Wunsafe-buffer-usage] Fixits for unsafe arguments of function pointer calls #80358

Uh oh!

Conversation

jkorous-apple commented Feb 1, 2024

Uh oh!

github-actions bot commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haoNoQ Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

jkorous-apple Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

haoNoQ Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

jkorous-apple Feb 14, 2024 • 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!

haoNoQ Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

jkorous-apple Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Feb 1, 2024 •

edited

Loading

llvmbot commented Feb 13, 2024 •

edited

Loading

jkorous-apple Feb 14, 2024 •

edited

Loading