[-Wunsafe-buffer-usage] Fix the crash introduced by the unsafe invocation of span::data warning

[malavikasamak]

The patch fixes the crash introduced by the DataInvocation warning gadget designed to warn against unsafe invocations of span::data method.

It also now considers the invocation of span::data method inside parenthesis.

Radar: 121223051

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: Malavika Samak (malavikasamak)

Changes

The patch fixes the crash introduced by the DataInvocation warning gadget designed to warn against unsafe invocations of span::data method.

Radar: 121223051

---

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

3 Files Affected:

• (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+3-2)
• (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+19-7)
• (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-data-invocation.cpp (+16-8)

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 724c4304a072420..7df706beb22662c 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -739,9 +739,10 @@ class DataInvocationGadget : public WarningGadget {
   }
 
   static Matcher matcher() {
+    Matcher callExpr = cxxMemberCallExpr(
+        callee(cxxMethodDecl(hasName("data"), ofClass(hasName("std::span")))));
     return stmt(
-        explicitCastExpr(has(cxxMemberCallExpr(callee(cxxMethodDecl(
-                             hasName("data"), ofClass(hasName("std::span")))))))
+        explicitCastExpr(anyOf(has(callExpr), has(parenExpr(has(callExpr)))))
             .bind(OpTag));
   }
   const Stmt *getBaseStmt() const override { return Op; }
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 9eb1df5f0240596..749655d03342cca 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2263,15 +2263,27 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
         MsgParam = 3;
       } else if (const auto *ECE = dyn_cast<ExplicitCastExpr>(Operation)) {
         QualType destType = ECE->getType();
-        const uint64_t dSize =
-            Ctx.getTypeSize(destType.getTypePtr()->getPointeeType());
-        if (const auto *CE = dyn_cast<CXXMemberCallExpr>(ECE->getSubExpr())) {
-          QualType srcType = CE->getType();
-          const uint64_t sSize =
-              Ctx.getTypeSize(srcType.getTypePtr()->getPointeeType());
-          if (sSize >= dSize)
+        if (!isa<PointerType>(destType))
+          return;
+
+        const Expr *subExpr = ECE->getSubExpr();
+        // Check if related to DataInvocation warning gadget.
+        if (!isa<CXXMemberCallExpr>(subExpr)) {
+          if (const auto *SE = dyn_cast<ParenExpr>(subExpr)) {
+            if (!isa<CXXMemberCallExpr>(SE->getSubExpr()))
+              return;
+          } else
             return;
         }
+        const uint64_t dSize =
+            Ctx.getTypeSize(destType.getTypePtr()->getPointeeType());
+
+        QualType srcType = ECE->getSubExpr()->getType();
+        const uint64_t sSize =
+            Ctx.getTypeSize(srcType.getTypePtr()->getPointeeType());
+        if (sSize >= dSize)
+          return;
+
         MsgParam = 4;
       }
       Loc = Operation->getBeginLoc();
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-data-invocation.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-data-invocation.cpp
index 79eb3bb4bacc6e7..7b39bef04114236 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-data-invocation.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-data-invocation.cpp
@@ -22,6 +22,8 @@ namespace std {
 using size_t = __typeof(sizeof(int));
 void *malloc(size_t);
 
+typedef long int  intptr_t;
+
 void foo(int v) {
 }
 
@@ -90,15 +92,18 @@ void cast_without_data(int *ptr) {
 void warned_patterns(std::span<int> span_ptr, std::span<Base> base_span, span<int> span_without_qual) {
     A *a1 = (A*)span_ptr.data(); // expected-warning{{unsafe invocation of span::data}}
     a1 = (A*)span_ptr.data(); // expected-warning{{unsafe invocation of span::data}}
-  
-    A *a2 = (A*) span_without_qual.data(); // expected-warning{{unsafe invocation of span::data}}
-   
-    // TODO:: Should we warn when we cast from base to derived type?
-    Derived *b = dynamic_cast<Derived*> (base_span.data());// expected-warning{{unsafe invocation of span::data}}
 
-   // TODO:: This pattern is safe. We can add special handling for it, if we decide this
-   // is the recommended fixit for the unsafe invocations.
-   A *a3 = (A*)span_ptr.subspan(0, sizeof(A)).data(); // expected-warning{{unsafe invocation of span::data}}
+    a1 = (A*)(span_ptr.data()); // expected-warning{{unsafe invocation of span::data}}
+    A *a2 = (A*) (span_without_qual.data()); // expected-warning{{unsafe invocation of span::data}}
+
+    a2 = (A*) span_without_qual.data(); // expected-warning{{unsafe invocation of span::data}}
+
+     // TODO:: Should we warn when we cast from base to derived type?
+     Derived *b = dynamic_cast<Derived*> (base_span.data());// expected-warning{{unsafe invocation of span::data}}
+
+    // TODO:: This pattern is safe. We can add special handling for it, if we decide this
+    // is the recommended fixit for the unsafe invocations.
+    A *a3 = (A*)span_ptr.subspan(0, sizeof(A)).data(); // expected-warning{{unsafe invocation of span::data}}
 }
 
 void not_warned_patterns(std::span<A> span_ptr, std::span<Base> base_span) {
@@ -108,6 +113,9 @@ void not_warned_patterns(std::span<A> span_ptr, std::span<Base> base_span) {
 
     p = (int*) span_ptr.data();
     A *a = (A*) span_ptr.hello(); // Invoking other methods.
+   
+     intptr_t k = (intptr_t) span_ptr.data();
+    k = (intptr_t) (span_ptr.data());
 }
 
 // We do not want to warn about other types

[ziqingluo-90]

LGTM! Feel free to ignore my comment if it doesn't make sense to you.

[ziqingluo-90]

nitpick: not sure if we need this check. DataInvocation warning gadget is the only possible explicit-cast kind Operation here. This is completely my personal taste: this function sort of implies unique correspondence between warning gadgets and Operation kinds if without the check.

[malavikasamak]

You are right that we currently don't have any other warning gadget that this could correspond to. I was being a little cautious that a future gadget may accidentally match here. Let's get rid of it for now.