[-Wunsafe-buffer-usage] Handle pointer assignments only in unspecified contexts

[t-rasmud]

Summary

This patch ensures that the assignment statements responsible for grouping variables (as in https://reviews.llvm.org/D145739) are in an unspecified context (and not in arguments of call expressions, conditions of if statements, etc)

[NoQ]

Great, looks correct to me!

It's probably better to squash this into D145739 so that to avoid the temporary incorrectness. (I hope it won't cause too many merge conflicts.)

clang/lib/Analysis/UnsafeBufferUsage.cpp

194–195

I think we need better comments for all three of those 😅 Like, explain what we mean by "unspecified" and write down the exact requirements on types and value kinds of replacement expressions.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

197

All the names of our contexts start with "Unspecified-". So maybe adding "Untyped" to the name makes it more clear.

[t-rasmud]

Address comments about documentation.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

197

Yeah these context names are terrible and that's entirely on me 😅

[NoQ]

LGTM!

[t-rasmud]

Squashed this commit in https://reviews.llvm.org/D145739

clang/lib/Analysis/UnsafeBufferUsage.cpp

 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
   // ...
   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)));
 
   auto CompOperandMatcher =
       binaryOperator(hasAnyOperatorName("!=", "==", "<", "<=", ">", ">="),
                      eachOf(hasLHS(allOf(hasPointerType(), InnerMatcher)),
                             hasRHS(allOf(hasPointerType(), InnerMatcher))));
 
   return stmt(anyOf(CallArgMatcher, CastOperandMatcher, CompOperandMatcher));
   // FIXME: any more cases? (UPC excludes the RHS of an assignment.  For now we
   // don't have to check that.)
 }
+
+// Returns a matcher that matches any expression 'e' such that `innerMatcher`
+// matches 'e' and 'e' is in an unspecified untyped context (i.e the expression
+// 'e' isn't evaluated to an RValue). For example, consider the following code:
+//    int *p = new int[4];
+//    int *q = new int[4];
+//    if ((p = q)) {}
+//    p = q;
+// The expression `p = q` in the conditional of the `if` statement
+// `if ((p = q))` is evaluated as an RValue, whereas the expression `p = q;`
+// in the assignment statement is in an untyped context.
+static internal::Matcher<Stmt>
+isInUnspecifiedUntypedContext(internal::Matcher<Stmt> InnerMatcher) {
+  // An unspecified context can be
+  // 1. A compound statement,
+  // 2. The body of an if statement
+  // 3. Body of a loop
+  auto CompStmt = compoundStmt(forEach(InnerMatcher));
+  auto IfStmtThen = ifStmt(hasThen(InnerMatcher));
+  auto IfStmtElse = ifStmt(hasElse(InnerMatcher));
+  // FIXME: Handle loop bodies.
+  return stmt(anyOf(CompStmt, IfStmtThen, IfStmtElse));
+}
 } // namespace clang::ast_matchers
 
 namespace {
 // Because the analysis revolves around variables and their types, we'll need to
 // track uses of variables (aka DeclRefExprs).
 using DeclUseList = SmallVector<const DeclRefExpr *, 1>;
 
 // Convenience typedef.
PointerAssignmentGadget(const MatchFinder::MatchResult &Result)
     PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)),
     PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {}
 
   static bool classof(const Gadget *G) {
     return G->getKind() == Kind::PointerAssignment;
   }
 
   static Matcher matcher() {
-    auto PtrAssignExpr = allOf(hasOperatorName("="),
+    auto PtrAssignExpr = binaryOperator(allOf(hasOperatorName("="),
       hasRHS(ignoringParenImpCasts(declRefExpr(hasPointerType(),
                                                to(varDecl())).
                                    bind(PointerAssignRHSTag))),
                             hasLHS(declRefExpr(hasPointerType(),
                                                to(varDecl())).
-                                   bind(PointerAssignLHSTag)));
+                                   bind(PointerAssignLHSTag))));
 
     //FIXME: Handle declarations at assignments
 
-    return stmt(binaryOperator(PtrAssignExpr));
+    return stmt(isInUnspecifiedUntypedContext(PtrAssignExpr));
   }
 
   virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
 
   virtual const Stmt *getBaseStmt() const override { return PA; }
 
   virtual DeclUseList getClaimedVarUseSites() const override {
     return DeclUseList{PtrLHS, PtrRHS};

clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-uuc-fixits.cpp

+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+void bar(int * param) {}
+
+void foo1a() {
+  int *r = new int[7];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> r"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 7}"
+  int *p = new int[4];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 4}"
+  p = r;
+  int tmp = p[9];
+  int *q;
+  q = r;
+}
+
+void uuc_if_body() {
+  int *r = new int[7];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> r"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 7}"
+  int *p = new int[4];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 4}"
+  if (true)
+    p = r;
+  p[5] = 4;
+}
+
+void uuc_if_body1(bool flag) {
+  int *r = new int[7];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> r"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 7}"
+  int *p = new int[4];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 4}"
+  if (flag) {
+    p = r;
+  }
+  p[5] = 4;
+}
+
+void uuc_if_cond_no_unsafe_op() {
+  int *r = new int[7];
+  int *p = new int[4];
+  if ((p = r)) {
+    int x = 0;
+  }
+}
+
+void uuc_if_cond_unsafe_op() {
+  int *r = new int[7];
+  int *p = new int[4];  //expected-warning{{'p' is an unsafe pointer used for buffer access}}
+  if ((p = r)) {
+    p[3] = 2;  // expected-note{{used in buffer access here}}
+  }
+}
+
+void uuc_if_cond_unsafe_op1() {
+  int *r = new int[7];  // expected-warning{{'r' is an unsafe pointer used for buffer access}}
+  int *p = new int[4];
+  if ((p = r)) {
+    r[3] = 2;  // expected-note{{used in buffer access here}}
+  }
+}
+
+void uuc_if_cond_unsafe_op2() {
+  int *r = new int[7];  // expected-warning{{'r' is an unsafe pointer used for buffer access}}
+  int *p = new int[4];  // expected-warning{{'p' is an unsafe pointer used for buffer access}}
+  if ((p = r)) {
+    r[3] = 2;  // expected-note{{used in buffer access here}}
+  }
+  p[4] = 6;  // expected-note{{used in buffer access here}}
+}
+
+void uuc_call1() {
+  int *w = new int[4];  // expected-warning{{'w' is an unsafe pointer used for buffer access}}
+  int *y = new int[4];
+  bar(w = y);
+  w[5] = 0;  // expected-note{{used in buffer access here}}
+}

clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-uuc.cpp

+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify %s
+void bar(int * param) {}
+
+void foo1a() {
+  int *r = new int[7];
+  int *p = new int[4];  // expected-warning{{'p' is an unsafe pointer used for buffer access}} expected-note-re{{{{^change type of 'p' to 'std::span' to preserve bounds information, and change 'r' to 'std::span' to propagate bounds information betwen them$}}}}
+  p = r;
+  int tmp = p[9];  // expected-note{{used in buffer access here}}
+  int *q;
+  q = r;
+}
+
+void uuc_if_body() {
+  int *r = new int[7];
+  int *p = new int[4];  // expected-warning{{'p' is an unsafe pointer used for buffer access}} // expected-note-re{{{{^change type of 'p' to 'std::span' to preserve bounds information, and change 'r' to 'std::span' to propagate bounds information betwen them$}}}}
+  if (true)
+    p = r;
+  p[5] = 4;  // expected-note{{used in buffer access here}}
+}
+
+void uuc_if_body1(bool flag) {
+  int *r = new int[7];
+  int *p = new int[4];  // expected-warning{{'p' is an unsafe pointer used for buffer access}} // expected-note-re{{{{^change type of 'p' to 'std::span' to preserve bounds information, and change 'r' to 'std::span' to propagate bounds information betwen them$}}}}
+  if (flag) {
+    p = r;
+  }
+  p[5] = 4;  // expected-note{{used in buffer access here}}
+}
+
+void uuc_if_body2(bool flag) {
+  int *r = new int[7];
+  int *p = new int[4];  // expected-warning{{'p' is an unsafe pointer used for buffer access}} // expected-note-re{{{{^change type of 'p' to 'std::span' to preserve bounds information, and change 'r' to 'std::span' to propagate bounds information betwen them$}}}}
+  if (flag) {
+  } else {
+    p = r;
+  }
+  
+  p[5] = 4;  // expected-note{{used in buffer access here}}
+}
+
+void uuc_if_cond_no_unsafe_op() {
+  int *r = new int[7];
+  int *p = new int[4];
+  if ((p = r)) {
+    int x = 0;
+  }
+}
+
+void uuc_if_cond_no_unsafe_op1() {
+  int *r = new int[7];
+  int *p = new int[4];
+  if (true) {
+    int x = 0;
+  } else if ((p = r))
+    int y = 10;
+}
+
+void uuc_if_cond_unsafe_op() {
+  int *r = new int[7];
+  int *p = new int[4];  //expected-warning{{'p' is an unsafe pointer used for buffer access}}
+  if ((p = r)) {
+    p[3] = 2;  // expected-note{{used in buffer access here}}
+  }
+}
+
+void uuc_if_cond_unsafe_op1() {
+  int *r = new int[7];  // expected-warning{{'r' is an unsafe pointer used for buffer access}}
+  int *p = new int[4];
+  if ((p = r)) {
+    r[3] = 2;  // expected-note{{used in buffer access here}}
+  }
+}
+
+void uuc_if_cond_unsafe_op2() {
+  int *r = new int[7];  // expected-warning{{'r' is an unsafe pointer used for buffer access}}
+  int *p = new int[4];  // expected-warning{{'p' is an unsafe pointer used for buffer access}}
+  if ((p = r)) {
+    r[3] = 2;  // expected-note{{used in buffer access here}}
+  }
+  p[4] = 6;  // expected-note{{used in buffer access here}}
+}
+
+void uuc_call1() {
+  int *w = new int[4];  // expected-warning{{'w' is an unsafe pointer used for buffer access}}
+  int *y = new int[4];
+  bar(w = y);
+  w[5] = 0;  // expected-note{{used in buffer access here}}
+}