[-Wunsafe-buffer-usage] Add fixits for array to pointer assignment #81343

jkorous-apple · 2024-02-10T00:54:08Z

depends on
#80347

github-actions · 2024-02-10T00:57:05Z

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

…PtrAssignment

…d to pointer Introducing CArrayToPtrAssignment gadget and implementing fixits for some cases of array being assigned to pointer. Key observations: - const size array can be assigned to std::span and bounds are propagated - const size array can't be on LHS of assignment This means array to pointer assignment has no strategy implications. Fixits are implemented for cases where one of the variables in the assignment is safe. For assignment of a safe array to unsafe pointer we know that the RHS will never be transformed since it's safe and can immediately emit the optimal fixit. Similarly for assignment of unsafe array to safe pointer. (Obviously this is not and can't be future-proof in regards to what variables we consider unsafe and that is fine.) Fixits for assignment from unsafe array to unsafe pointer (from Array to Span strategy) are not implemented in this patch as that needs to be properly designed first - we might possibly implement optimal fixits for partially transformed cases, put both variables in a single fixit group or do something else.

llvmbot · 2024-02-15T02:19:10Z

@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/81343.diff

4 Files Affected:

(modified) clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def (+2-1)
(modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+99-15)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp (+1-1)
(added) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp (+43)

diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 07f805ebb11013..3273c642eed517 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -45,7 +45,8 @@ FIXABLE_GADGET(UPCAddressofArraySubscript) // '&DRE[any]' in an Unspecified Poin
 FIXABLE_GADGET(UPCStandalonePointer)
 FIXABLE_GADGET(UPCPreIncrement)            // '++Ptr' in an Unspecified Pointer Context
 FIXABLE_GADGET(UUCAddAssign)            // 'Ptr += n' in an Unspecified Untyped Context
-FIXABLE_GADGET(PointerAssignment)
+FIXABLE_GADGET(PtrToPtrAssignment)
+FIXABLE_GADGET(CArrayToPtrAssignment)
 FIXABLE_GADGET(PointerInit)
 
 #undef FIXABLE_GADGET
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index a74c113e29f1cf..8e810876950c81 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -7,11 +7,14 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
+#include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Lex/Lexer.h"
@@ -799,7 +802,8 @@ class PointerInitGadget : public FixableGadget {
 ///  \code
 ///  p = q;
 ///  \endcode
-class PointerAssignmentGadget : public FixableGadget {
+/// where both `p` and `q` are pointers.
+class PtrToPtrAssignmentGadget : public FixableGadget {
 private:
   static constexpr const char *const PointerAssignLHSTag = "ptrLHS";
   static constexpr const char *const PointerAssignRHSTag = "ptrRHS";
@@ -807,13 +811,13 @@ class PointerAssignmentGadget : public FixableGadget {
   const DeclRefExpr * PtrRHS;         // the RHS pointer expression in `PA`
 
 public:
-  PointerAssignmentGadget(const MatchFinder::MatchResult &Result)
-      : FixableGadget(Kind::PointerAssignment),
-    PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)),
-    PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {}
+  PtrToPtrAssignmentGadget(const MatchFinder::MatchResult &Result)
+      : FixableGadget(Kind::PtrToPtrAssignment),
+        PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)),
+        PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {}
 
   static bool classof(const Gadget *G) {
-    return G->getKind() == Kind::PointerAssignment;
+    return G->getKind() == Kind::PtrToPtrAssignment;
   }
 
   static Matcher matcher() {
@@ -848,6 +852,60 @@ class PointerAssignmentGadget : public FixableGadget {
   }
 };
 
+/// An assignment expression of the form:
+///  \code
+///  ptr = array;
+///  \endcode
+/// where `p` is a pointer and `array` is a constant size array.
+class CArrayToPtrAssignmentGadget : public FixableGadget {
+private:
+  static constexpr const char *const PointerAssignLHSTag = "ptrLHS";
+  static constexpr const char *const PointerAssignRHSTag = "ptrRHS";
+  const DeclRefExpr *PtrLHS; // the LHS pointer expression in `PA`
+  const DeclRefExpr *PtrRHS; // the RHS pointer expression in `PA`
+
+public:
+  CArrayToPtrAssignmentGadget(const MatchFinder::MatchResult &Result)
+      : FixableGadget(Kind::CArrayToPtrAssignment),
+        PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)),
+        PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {}
+
+  static bool classof(const Gadget *G) {
+    return G->getKind() == Kind::CArrayToPtrAssignment;
+  }
+
+  static Matcher matcher() {
+    auto PtrAssignExpr = binaryOperator(
+        allOf(hasOperatorName("="),
+              hasRHS(ignoringParenImpCasts(
+                  declRefExpr(hasType(hasCanonicalType(constantArrayType())),
+                              toSupportedVariable())
+                      .bind(PointerAssignRHSTag))),
+              hasLHS(declRefExpr(hasPointerType(), toSupportedVariable())
+                         .bind(PointerAssignLHSTag))));
+
+    return stmt(isInUnspecifiedUntypedContext(PtrAssignExpr));
+  }
+
+  virtual std::optional<FixItList>
+  getFixits(const FixitStrategy &S) const override;
+
+  virtual const Stmt *getBaseStmt() const override {
+    // FIXME: This should be the binary operator, assuming that this method
+    // makes sense at all on a FixableGadget.
+    return PtrLHS;
+  }
+
+  virtual DeclUseList getClaimedVarUseSites() const override {
+    return DeclUseList{PtrLHS, PtrRHS};
+  }
+
+  virtual std::optional<std::pair<const VarDecl *, const VarDecl *>>
+  getStrategyImplications() const override {
+    return {};
+  }
+};
+
 /// A call of a function or method that performs unchecked buffer operations
 /// over one of its pointer parameters.
 class UnsafeBufferUsageAttrGadget : public WarningGadget {
@@ -1471,7 +1529,7 @@ bool clang::internal::anyConflict(const SmallVectorImpl<FixItHint> &FixIts,
 }
 
 std::optional<FixItList>
-PointerAssignmentGadget::getFixits(const FixitStrategy &S) const {
+PtrToPtrAssignmentGadget::getFixits(const FixitStrategy &S) const {
   const auto *LeftVD = cast<VarDecl>(PtrLHS->getDecl());
   const auto *RightVD = cast<VarDecl>(PtrRHS->getDecl());
   switch (S.lookup(LeftVD)) {
@@ -1490,6 +1548,26 @@ PointerAssignmentGadget::getFixits(const FixitStrategy &S) const {
   return std::nullopt;
 }
 
+/// \returns fixit that adds .data() call after \DRE.
+static inline std::optional<FixItList> createDataFixit(const ASTContext &Ctx,
+                                                       const DeclRefExpr *DRE);
+
+std::optional<FixItList>
+CArrayToPtrAssignmentGadget::getFixits(const FixitStrategy &S) const {
+  const auto *LeftVD = cast<VarDecl>(PtrLHS->getDecl());
+  const auto *RightVD = cast<VarDecl>(PtrRHS->getDecl());
+  if (S.lookup(LeftVD) == FixitStrategy::Kind::Span) {
+    if (S.lookup(RightVD) == FixitStrategy::Kind::Wontfix) {
+      return FixItList{};
+    }
+  } else if (S.lookup(LeftVD) == FixitStrategy::Kind::Wontfix) {
+    if (S.lookup(RightVD) == FixitStrategy::Kind::Array) {
+      return createDataFixit(RightVD->getASTContext(), PtrRHS);
+    }
+  }
+  return std::nullopt;
+}
+
 std::optional<FixItList>
 PointerInitGadget::getFixits(const FixitStrategy &S) const {
   const auto *LeftVD = PtrInitLHS;
@@ -1907,6 +1985,19 @@ PointerDereferenceGadget::getFixits(const FixitStrategy &S) const {
   return std::nullopt;
 }
 
+static inline std::optional<FixItList> createDataFixit(const ASTContext &Ctx,
+                                                       const DeclRefExpr *DRE) {
+  const SourceManager &SM = Ctx.getSourceManager();
+  // Inserts the .data() after the DRE
+  std::optional<SourceLocation> EndOfOperand =
+      getPastLoc(DRE, SM, Ctx.getLangOpts());
+
+  if (EndOfOperand)
+    return FixItList{{FixItHint::CreateInsertion(*EndOfOperand, ".data()")}};
+
+  return std::nullopt;
+}
+
 // Generates fix-its replacing an expression of the form UPC(DRE) with
 // `DRE.data()`
 std::optional<FixItList>
@@ -1915,14 +2006,7 @@ UPCStandalonePointerGadget::getFixits(const FixitStrategy &S) const {
   switch (S.lookup(VD)) {
   case FixitStrategy::Kind::Array:
   case FixitStrategy::Kind::Span: {
-    ASTContext &Ctx = VD->getASTContext();
-    SourceManager &SM = Ctx.getSourceManager();
-    // Inserts the .data() after the DRE
-    std::optional<SourceLocation> EndOfOperand =
-        getPastLoc(Node, SM, Ctx.getLangOpts());
-
-    if (EndOfOperand)
-      return FixItList{{FixItHint::CreateInsertion(*EndOfOperand, ".data()")}};
+    return createDataFixit(VD->getASTContext(), Node);
     // FIXME: Points inside a macro expansion.
     break;
   }
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
index a5b578b98d4e5b..4cc1948f28a773 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
@@ -53,7 +53,7 @@ void unclaimed_use() {
 void implied_unclaimed_var(int *b) {  // expected-warning{{'b' is an unsafe pointer used for buffer access}}
   int *a = new int[3];  // expected-warning{{'a' is an unsafe pointer used for buffer access}}
   a[4] = 7;  // expected-note{{used in buffer access here}}
-  a = b;  // debug-note{{safe buffers debug: gadget 'PointerAssignment' refused to produce a fix}}
+  a = b;  // debug-note{{safe buffers debug: gadget 'PtrToPtrAssignment' refused to produce a fix}}
   b++;  // expected-note{{used in pointer arithmetic here}} \
         // debug-note{{safe buffers debug: failed to produce fixit for 'b' : has an unclaimed use}}
 }
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp
new file mode 100644
index 00000000000000..020a4520e2ccb4
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN:            -fsafe-buffer-usage-suggestions \
+// RUN:            -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+void safe_array_assigned_to_safe_ptr(unsigned idx) {
+  int buffer[10];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int* ptr;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  ptr = buffer;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+}
+
+void safe_array_assigned_to_unsafe_ptr(unsigned idx) {
+  int buffer[10];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int* ptr;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> ptr"
+  ptr = buffer;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  ptr[idx] = 0;
+}
+
+void unsafe_array_assigned_to_safe_ptr(unsigned idx) {
+  int buffer[10];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array<int, 10> buffer"
+  int* ptr;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  ptr = buffer;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:15-[[@LINE-1]]:15}:".data()"
+  buffer[idx] = 0;
+}
+
+void unsafe_array_assigned_to_unsafe_ptr(unsigned idx) {
+  int buffer[10];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}
+  int* ptr;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}
+  ptr = buffer;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}
+  buffer[idx] = 0;
+  ptr[idx] = 0;
+}

haoNoQ · 2024-02-15T02:31:31Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

+  virtual std::optional<std::pair<const VarDecl *, const VarDecl *>>
+  getStrategyImplications() const override {
+    return {};
+  }


This is probably the default behavior 🤔

haoNoQ · 2024-02-15T02:32:53Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

+                                                       const DeclRefExpr *DRE);
+
+std::optional<FixItList>
+CArrayToPtrAssignmentGadget::getFixits(const FixitStrategy &S) const {


I think there should be a comment explaining why the "both sides are fixed" case is tricky. Otherwise somebody may accidentally implement it 😅

haoNoQ · 2024-02-15T02:34:20Z

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp

+  buffer[idx] = 0;
+}
+
+void unsafe_array_assigned_to_unsafe_ptr(unsigned idx) {


Similarly, let's mark this test as a FIXME test, and add a comment explaining why this is hard and the naive fix would be incorrect.

haoNoQ

LGTM! I have a couple minor nitpicks.

…signment

haoNoQ

LGTM let's land!

…lvm#81343) Introducing CArrayToPtrAssignment gadget and implementing fixits for some cases of array being assigned to pointer. Key observations: - const size array can be assigned to std::span and bounds are propagated - const size array can't be on LHS of assignment This means array to pointer assignment has no strategy implications. Fixits are implemented for cases where one of the variables in the assignment is safe. For assignment of a safe array to unsafe pointer we know that the RHS will never be transformed since it's safe and can immediately emit the optimal fixit. Similarly for assignment of unsafe array to safe pointer. (Obviously this is not and can't be future-proof in regards to what variables we consider unsafe and that is fine.) Fixits for assignment from unsafe array to unsafe pointer (from Array to Span strategy) are not implemented in this patch as that needs to be properly designed first - we might possibly implement optimal fixits for partially transformed cases, put both variables in a single fixit group or do something else. (cherry picked from commit 6fce42f)

jkorous-apple marked this pull request as draft February 13, 2024 01:46

jkorous-apple force-pushed the cxx-safe-buffers/array-assigned-to-ptr branch 2 times, most recently from 386a346 to d7b1ae0 Compare February 14, 2024 22:49

jkorous-apple marked this pull request as ready for review February 14, 2024 22:49

jkorous-apple added 4 commits February 14, 2024 18:17

[-Wunsafe-buffer-usage][NFC] Factor out .data() fixit to a function

791130c

[-Wunsafe-buffer-usage][NFC] Rename PointerAssignment gadget to PtrTo…

1b12f74

…PtrAssignment

[-Wunsafe-buffer-usage][NFC] clang-format the PR

5591e8e

jkorous-apple force-pushed the cxx-safe-buffers/array-assigned-to-ptr branch from 8e73706 to 5591e8e Compare February 15, 2024 02:18

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

jkorous-apple requested review from t-rasmud, ziqingluo-90, haoNoQ and malavikasamak and removed request for t-rasmud February 15, 2024 02:19

haoNoQ reviewed Feb 15, 2024

View reviewed changes

[-Wunsafe-buffer-usage][NFC] Comment on trickiness of array to ptr as…

3260557

…signment

haoNoQ approved these changes Feb 15, 2024

View reviewed changes

jkorous-apple merged commit 6fce42f into llvm:main Feb 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[-Wunsafe-buffer-usage] Add fixits for array to pointer assignment #81343

[-Wunsafe-buffer-usage] Add fixits for array to pointer assignment #81343

Uh oh!

jkorous-apple commented Feb 10, 2024

Uh oh!

github-actions bot commented Feb 10, 2024 •

edited

Loading

Uh oh!

llvmbot commented Feb 15, 2024 •

edited

Loading

Uh oh!

haoNoQ Feb 15, 2024

Uh oh!

haoNoQ Feb 15, 2024

Uh oh!

haoNoQ Feb 15, 2024

Uh oh!

haoNoQ left a comment

Uh oh!

haoNoQ left a comment

Uh oh!

Uh oh!

[-Wunsafe-buffer-usage] Add fixits for array to pointer assignment #81343

[-Wunsafe-buffer-usage] Add fixits for array to pointer assignment #81343

Uh oh!

Conversation

jkorous-apple commented Feb 10, 2024

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

haoNoQ Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

haoNoQ Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

haoNoQ Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

haoNoQ left a comment

Choose a reason for hiding this comment

Uh oh!

haoNoQ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Feb 10, 2024 •

edited

Loading

llvmbot commented Feb 15, 2024 •

edited

Loading