[-Wunsafe-buffer-usage] Match unsafe pointers being casted to bool or participating in pointer subtractions

[ziqingluo-90]

Summary

To add two new unique cases to the Unspecified Pointer Context (UPC):

• A pointer being casted to a boolean value is in a UPC;
• A pointer participating in pointer subtraction is in a UPC.

[malavikasamak]

clang/lib/Analysis/UnsafeBufferUsage.cpp

168

I don't understand why we are special handling only pointer subtraction here. Won't pointer addition be also considered UPC? If so, can we just add it to this patch.

[malavikasamak]

clang/lib/Analysis/UnsafeBufferUsage.cpp

168

Never mind. It looks like pointer addition is not even legal and subtraction is a special case.

[NoQ]

Looks great overall!

clang/lib/Analysis/UnsafeBufferUsage.cpp

168

I think this is about operations between two pointers, like p - q. You can subtract a pointer from a pointer (it gives you the distance between points in memory) but you can't add a pointer to a pointer (it makes no sense).

But yeah, could be clarified.

188–191

Tabs vs. spaces.

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp

96

I think the original CHECK-NOT was better because it allows you to write more tests below it. It's good to have individual test cases self-contained and not affect the rest of the file.

[ziqingluo-90]

Address comments

[NoQ]

LGTM!

clang/lib/Analysis/UnsafeBufferUsage.cpp

188–191

Still a bit off!

clang/lib/Analysis/UnsafeBufferUsage.cpp

 
 // Returns a matcher that matches any expression `e` such that `InnerMatcher`
 // matches `e` and `e` is in an Unspecified Pointer Context (UPC).
 static internal::Matcher<Stmt>
 isInUnspecifiedPointerContext(internal::Matcher<Stmt> InnerMatcher) {
   // A UPC can be
   // 1. an argument of a function call (except the callee has [[unsafe_...]]
   // attribute), or
-  // 2. the operand of a cast operation; or
+  // 2. the operand of a cast-to-(Integer or Boolean) operation; or
+  // 3. the operand of a pointer subtraction operation
+  //    (i.e., computing the distance between two pointers); or
+  // 4. the operand of a pointer comparison operation; or
   // ...
   auto CallArgMatcher =
-      callExpr(forEachArgumentWithParam(InnerMatcher, 
+      callExpr(forEachArgumentWithParam(InnerMatcher,
                   hasPointerType() /* array also decays to pointer type*/),
           unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage)))));
 
   auto CastOperandMatcher =
-      explicitCastExpr(hasCastKind(CastKind::CK_PointerToIntegral),
-                       castSubExpr(allOf(hasPointerType(), InnerMatcher)));
+    castExpr(anyOf(hasCastKind(CastKind::CK_PointerToIntegral),
+       hasCastKind(CastKind::CK_PointerToBoolean)),
+       castSubExpr(allOf(hasPointerType(), InnerMatcher)));
 
+  // A matcher that matches pointer subtractions:
+  auto PtrSubtractionMatcher =
+    binaryOperator(hasOperatorName("-"),
+                   // Note that here we need both LHS and RHS to be
+                   // pointer. Then the inner matcher can match any of
+                   // them:
+                   allOf(hasLHS(hasPointerType()),
+                               hasRHS(hasPointerType())),
+                   eachOf(hasLHS(InnerMatcher),
+                                hasRHS(InnerMatcher)));
+
   auto CompOperandMatcher =
       binaryOperator(hasAnyOperatorName("!=", "==", "<", "<=", ">", ">="),
                      eachOf(hasLHS(allOf(hasPointerType(), InnerMatcher)),
                             hasRHS(allOf(hasPointerType(), InnerMatcher))));
 
-  return stmt(anyOf(CallArgMatcher, CastOperandMatcher, CompOperandMatcher));
+  return stmt(anyOf(CallArgMatcher, CastOperandMatcher, CompOperandMatcher, PtrSubtractionMatcher));
   // FIXME: any more cases? (UPC excludes the RHS of an assignment.  For now we
   // don't have to check that.)
 }
 } // namespace clang::ast_matchers
 
 namespace {
 // Because the analysis revolves around variables and their types, we'll need to
 // track uses of variables (aka DeclRefExprs).

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp

 // RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 int f(unsigned long, void *);
 
 [[clang::unsafe_buffer_usage]]
 int unsafe_f(unsigned long, void *);
 
 void address_to_integer(int x) {
   int * p = new int[10];
   unsigned long n = (unsigned long) &p[5];
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:37-[[@LINE-1]]:42}:"&p.data()[5]"
   unsigned long m = (unsigned long) &p[x];
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:37-[[@LINE-1]]:42}:"&p.data()[x]"
 }
 
+void address_to_bool(int x) {
+  int * p = new int[10];
+  bool a = (bool) &p[5];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:19-[[@LINE-1]]:24}:"&p.data()[5]"
+  bool b = (bool) &p[x];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:19-[[@LINE-1]]:24}:"&p.data()[x]"
+
+  bool a1 = &p[5];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:18}:"&p.data()[5]"
+  bool b1 = &p[x];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:18}:"&p.data()[x]"
+
+  if (&p[5]) {
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:12}:"&p.data()[5]"
+    return;
+  }
+}
+
 void call_argument(int x) {
   int * p = new int[10];
 
   f((unsigned long) &p[5], &p[x]);
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:21-[[@LINE-1]]:26}:"&p.data()[5]"
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:28-[[@LINE-2]]:33}:"&p.data()[x]"
 }
 
void index_is_zero() {
   int n = p[5];
 
   f((unsigned long)&p[0],
     // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:20-[[@LINE-1]]:25}:"p.data()"
     &p[0]);
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:5-[[@LINE-1]]:10}:"p.data()"
 }
 
-// CHECK-NOT: fix-it
+void pointer_subtraction(int x) {
+  int * p = new int[10];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
+
+  int n = &p[9] - &p[4];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:16}:"&p.data()[9]"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:19-[[@LINE-2]]:24}:"&p.data()[4]"
+  if (&p[9] - &p[x]) {
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:12}:"&p.data()[9]"
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:20}:"&p.data()[x]"
+    return;
+  }
+}
+
 // To test multiple function declarations, each of which carries
-// different incomplete informations:
+// different incomplete informations.
+// no fix-it in the rest of this test:
+
 [[clang::unsafe_buffer_usage]]
 void unsafe_g(void*);
 
 void unsafe_g(void*);
 
 void multiple_unsafe_fundecls() {
   int * p = new int[10];
-
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
   unsafe_g(&p[5]);
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
 }
 
 void unsafe_h(void*);
 
 [[clang::unsafe_buffer_usage]]
 void unsafe_h(void*);
 
 void unsafe_h(void* p) { ((char*)p)[10]; }
 
 void multiple_unsafe_fundecls2() {
   int * p = new int[10];
-
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
   unsafe_h(&p[5]);
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
 }