[clang] 6b652f6 - [attributes][-Wunsafe-buffer-usage] Support adding unsafe_buffer_usage attribute to struct fields (#101585)

via cfe-commits cfe-commits at lists.llvm.org
Wed Aug 14 10:45:47 PDT 2024


Author: Malavika Samak
Date: 2024-08-14T10:45:43-07:00
New Revision: 6b652f6edaa84a9e4934885f6f12b973efb1b810

URL: https://github.com/llvm/llvm-project/commit/6b652f6edaa84a9e4934885f6f12b973efb1b810
DIFF: https://github.com/llvm/llvm-project/commit/6b652f6edaa84a9e4934885f6f12b973efb1b810.diff

LOG: [attributes][-Wunsafe-buffer-usage] Support adding unsafe_buffer_usage attribute to struct fields (#101585)

Extend the unsafe_buffer_usage attribute, so they can also be added to
struct fields. This will cause the compiler to warn about the unsafe
field at their access sites.

Co-authored-by: MalavikaSamak <malavika2 at apple.com>

Added: 
    clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp

Modified: 
    clang/include/clang/Basic/Attr.td
    clang/include/clang/Basic/AttrDocs.td
    clang/include/clang/Basic/DiagnosticSemaKinds.td
    clang/lib/Analysis/UnsafeBufferUsage.cpp
    clang/lib/Sema/AnalysisBasedWarnings.cpp
    clang/test/Misc/pragma-attribute-supported-attributes-list.test

Removed: 
    


################################################################################
diff  --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 8ac2079099c854..ccdbb8e4fd3283 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -4467,7 +4467,7 @@ def ReleaseHandle : InheritableParamAttr {
 
 def UnsafeBufferUsage : InheritableAttr {
   let Spellings = [Clang<"unsafe_buffer_usage">];
-  let Subjects = SubjectList<[Function]>;
+  let Subjects = SubjectList<[Function, Field]>;
   let Documentation = [UnsafeBufferUsageDocs];
 }
 

diff  --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index 94c284fc731589..19cbb9a0111a28 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -6802,77 +6802,103 @@ def UnsafeBufferUsageDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 The attribute ``[[clang::unsafe_buffer_usage]]`` should be placed on functions
-that need to be avoided as they are prone to buffer overflows. It is designed to
-work together with the off-by-default compiler warning ``-Wunsafe-buffer-usage``
-to help codebases transition away from raw pointer based buffer management,
-in favor of safer abstractions such as C++20 ``std::span``. The attribute causes
-``-Wunsafe-buffer-usage`` to warn on every use of the function, and it may
-enable ``-Wunsafe-buffer-usage`` to emit automatic fix-it hints
-which would help the user replace such unsafe functions with safe
+that need to be avoided as they are prone to buffer overflows or unsafe buffer
+struct fields. It is designed to work together with the off-by-default compiler
+warning ``-Wunsafe-buffer-usage``to help codebases transition away from raw pointer
+based buffer management, in favor of safer abstractions such as C++20 ``std::span``.
+The attribute causes ``-Wunsafe-buffer-usage`` to warn on every use of the function or
+the field it is attached to, and it may also lead to emission of automatic fix-it
+hints which would help the user replace the use of unsafe functions(/fields) with safe
 alternatives, though the attribute can be used even when the fix can't be automated.
 
-The attribute does not suppress ``-Wunsafe-buffer-usage`` inside the function
-to which it is attached. These warnings still need to be addressed.
+* Attribute attached to functions: The attribute does not suppress 
+  ``-Wunsafe-buffer-usage`` inside the function to which it is attached. 
+  These warnings still need to be addressed.
 
-The attribute is warranted even if the only way a function can overflow
-the buffer is by violating the function's preconditions. For example, it
-would make sense to put the attribute on function ``foo()`` below because
-passing an incorrect size parameter would cause a buffer overflow:
+  The attribute is warranted even if the only way a function can overflow
+  the buffer is by violating the function's preconditions. For example, it
+  would make sense to put the attribute on function ``foo()`` below because
+  passing an incorrect size parameter would cause a buffer overflow:
 
-.. code-block:: c++
+  .. code-block:: c++
 
-  [[clang::unsafe_buffer_usage]]
-  void foo(int *buf, size_t size) {
-    for (size_t i = 0; i < size; ++i) {
-      buf[i] = i;
+    [[clang::unsafe_buffer_usage]]
+    void foo(int *buf, size_t size) {
+      for (size_t i = 0; i < size; ++i) {
+        buf[i] = i;
+      }
     }
-  }
 
-The attribute is NOT warranted when the function uses safe abstractions,
-assuming that these abstractions weren't misused outside the function.
-For example, function ``bar()`` below doesn't need the attribute,
-because assuming that the container ``buf`` is well-formed (has size that
-fits the original buffer it refers to), overflow cannot occur:
+  The attribute is NOT warranted when the function uses safe abstractions,
+  assuming that these abstractions weren't misused outside the function.
+  For example, function ``bar()`` below doesn't need the attribute,
+  because assuming that the container ``buf`` is well-formed (has size that
+  fits the original buffer it refers to), overflow cannot occur:
 
-.. code-block:: c++
+  .. code-block:: c++
 
-  void bar(std::span<int> buf) {
-    for (size_t i = 0; i < buf.size(); ++i) {
-      buf[i] = i;
+    void bar(std::span<int> buf) {
+      for (size_t i = 0; i < buf.size(); ++i) {
+        buf[i] = i;
+      }
     }
-  }
 
-In this case function ``bar()`` enables the user to keep the buffer
-"containerized" in a span for as long as possible. On the other hand,
-Function ``foo()`` in the previous example may have internal
-consistency, but by accepting a raw buffer it requires the user to unwrap
-their span, which is undesirable according to the programming model
-behind ``-Wunsafe-buffer-usage``.
+  In this case function ``bar()`` enables the user to keep the buffer
+  "containerized" in a span for as long as possible. On the other hand,
+  Function ``foo()`` in the previous example may have internal
+  consistency, but by accepting a raw buffer it requires the user to unwrap
+  their span, which is undesirable according to the programming model
+  behind ``-Wunsafe-buffer-usage``.
 
-The attribute is warranted when a function accepts a raw buffer only to
-immediately put it into a span:
+  The attribute is warranted when a function accepts a raw buffer only to
+  immediately put it into a span:
 
-.. code-block:: c++
+  .. code-block:: c++
 
-  [[clang::unsafe_buffer_usage]]
-  void baz(int *buf, size_t size) {
-    std::span<int> sp{ buf, size };
-    for (size_t i = 0; i < sp.size(); ++i) {
-      sp[i] = i;
+    [[clang::unsafe_buffer_usage]]
+    void baz(int *buf, size_t size) {
+      std::span<int> sp{ buf, size };
+      for (size_t i = 0; i < sp.size(); ++i) {
+        sp[i] = i;
+      }
     }
-  }
 
-In this case ``baz()`` does not contain any unsafe operations, but the awkward
-parameter type causes the caller to unwrap the span unnecessarily.
-Note that regardless of the attribute, code inside ``baz()`` isn't flagged
-by ``-Wunsafe-buffer-usage`` as unsafe. It is definitely undesirable,
-but if ``baz()`` is on an API surface, there is no way to improve it
-to make it as safe as ``bar()`` without breaking the source and binary
-compatibility with existing users of the function. In such cases
-the proper solution would be to create a 
diff erent function (possibly
-an overload of ``baz()``) that accepts a safe container like ``bar()``,
-and then use the attribute on the original ``baz()`` to help the users
-update their code to use the new function.
+  In this case ``baz()`` does not contain any unsafe operations, but the awkward
+  parameter type causes the caller to unwrap the span unnecessarily.
+  Note that regardless of the attribute, code inside ``baz()`` isn't flagged
+  by ``-Wunsafe-buffer-usage`` as unsafe. It is definitely undesirable,
+  but if ``baz()`` is on an API surface, there is no way to improve it
+  to make it as safe as ``bar()`` without breaking the source and binary
+  compatibility with existing users of the function. In such cases
+  the proper solution would be to create a 
diff erent function (possibly
+  an overload of ``baz()``) that accepts a safe container like ``bar()``,
+  and then use the attribute on the original ``baz()`` to help the users
+  update their code to use the new function.
+
+* Attribute attached to fields: The attribute should only be attached to 
+  struct fields, if the fields can not be updated to a safe type with bounds 
+  check, such as std::span. In other words, the buffers prone to unsafe accesses 
+  should always be updated to use safe containers/views and attaching the attribute 
+  must be last resort when such an update is infeasible.
+
+  The attribute can be placed on individual fields or a set of them as shown below.
+
+  .. code-block:: c++
+
+    struct A {
+      [[clang::unsafe_buffer_usage]]
+      int *ptr1;
+
+      [[clang::unsafe_buffer_usage]]
+      int *ptr2, buf[10];
+
+      [[clang::unsafe_buffer_usage]]
+      size_t sz;
+    };
+
+  Here, every read/write to the fields ptr1, ptr2, buf and sz will trigger a warning 
+  that the field has been explcitly marked as unsafe due to unsafe-buffer operations.
+
   }];
 }
 

diff  --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 8d4f8f36d8565b..ae4f89580ab2dd 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -12383,7 +12383,8 @@ def warn_unsafe_buffer_variable : Warning<
   InGroup<UnsafeBufferUsage>, DefaultIgnore;
 def warn_unsafe_buffer_operation : Warning<
   "%select{unsafe pointer operation|unsafe pointer arithmetic|"
-  "unsafe buffer access|function introduces unsafe buffer manipulation|unsafe invocation of span::data}0">,
+  "unsafe buffer access|function introduces unsafe buffer manipulation|unsafe invocation of span::data|"
+  "field %1 prone to unsafe buffer manipulation}0">,
   InGroup<UnsafeBufferUsage>, DefaultIgnore;
 def note_unsafe_buffer_operation : Note<
   "used%select{| in pointer arithmetic| in buffer access}0 here">;

diff  --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 866222380974b6..051381edabf0b2 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -926,22 +926,27 @@ class CArrayToPtrAssignmentGadget : public FixableGadget {
 /// A call of a function or method that performs unchecked buffer operations
 /// over one of its pointer parameters.
 class UnsafeBufferUsageAttrGadget : public WarningGadget {
-  constexpr static const char *const OpTag = "call_expr";
-  const CallExpr *Op;
+  constexpr static const char *const OpTag = "attr_expr";
+  const Expr *Op;
 
 public:
   UnsafeBufferUsageAttrGadget(const MatchFinder::MatchResult &Result)
       : WarningGadget(Kind::UnsafeBufferUsageAttr),
-        Op(Result.Nodes.getNodeAs<CallExpr>(OpTag)) {}
+        Op(Result.Nodes.getNodeAs<Expr>(OpTag)) {}
 
   static bool classof(const Gadget *G) {
     return G->getKind() == Kind::UnsafeBufferUsageAttr;
   }
 
   static Matcher matcher() {
+    auto HasUnsafeFieldDecl =
+        member(fieldDecl(hasAttr(attr::UnsafeBufferUsage)));
+
     auto HasUnsafeFnDecl =
         callee(functionDecl(hasAttr(attr::UnsafeBufferUsage)));
-    return stmt(callExpr(HasUnsafeFnDecl).bind(OpTag));
+
+    return stmt(anyOf(callExpr(HasUnsafeFnDecl).bind(OpTag),
+                      memberExpr(HasUnsafeFieldDecl).bind(OpTag)));
   }
 
   void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler,

diff  --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 0f604c61fa3af9..e6ce89dc7ec406 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2231,6 +2231,7 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
     SourceLocation Loc;
     SourceRange Range;
     unsigned MsgParam = 0;
+    NamedDecl *D = nullptr;
     if (const auto *ASE = dyn_cast<ArraySubscriptExpr>(Operation)) {
       Loc = ASE->getBase()->getExprLoc();
       Range = ASE->getBase()->getSourceRange();
@@ -2261,6 +2262,12 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
         // note_unsafe_buffer_operation doesn't have this mode yet.
         assert(!IsRelatedToDecl && "Not implemented yet!");
         MsgParam = 3;
+      } else if (isa<MemberExpr>(Operation)) {
+        // note_unsafe_buffer_operation doesn't have this mode yet.
+        assert(!IsRelatedToDecl && "Not implemented yet!");
+        auto ME = dyn_cast<MemberExpr>(Operation);
+        D = ME->getMemberDecl();
+        MsgParam = 5;
       } else if (const auto *ECE = dyn_cast<ExplicitCastExpr>(Operation)) {
         QualType destType = ECE->getType();
         if (!isa<PointerType>(destType))
@@ -2285,7 +2292,12 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
              "Variables blamed for unsafe buffer usage without suggestions!");
       S.Diag(Loc, diag::note_unsafe_buffer_operation) << MsgParam << Range;
     } else {
-      S.Diag(Loc, diag::warn_unsafe_buffer_operation) << MsgParam << Range;
+      if (D) {
+        S.Diag(Loc, diag::warn_unsafe_buffer_operation)
+            << MsgParam << D << Range;
+      } else {
+        S.Diag(Loc, diag::warn_unsafe_buffer_operation) << MsgParam << Range;
+      }
       if (SuggestSuggestions) {
         S.Diag(Loc, diag::note_safe_buffer_usage_suggestions_disabled);
       }

diff  --git a/clang/test/Misc/pragma-attribute-supported-attributes-list.test b/clang/test/Misc/pragma-attribute-supported-attributes-list.test
index 0f7dcab7c4248d..93620edce71ce2 100644
--- a/clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ b/clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -202,7 +202,7 @@
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
 // CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: Uninitialized (SubjectMatchRule_variable_is_local)
-// CHECK-NEXT: UnsafeBufferUsage (SubjectMatchRule_function)
+// CHECK-NEXT: UnsafeBufferUsage (SubjectMatchRule_function, SubjectMatchRule_field)
 // CHECK-NEXT: UseHandle (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: VTablePointerAuthentication (SubjectMatchRule_record)
 // CHECK-NEXT: VecReturn (SubjectMatchRule_record)

diff  --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp
new file mode 100644
index 00000000000000..0ba605475925b9
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp
@@ -0,0 +1,190 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN:            -fsafe-buffer-usage-suggestions -verify %s
+
+using size_t = __typeof(sizeof(int));
+
+namespace std {
+  class type_info;
+  class bad_cast;
+  class bad_typeid;
+
+  template <typename T> class span {
+
+  private:
+    T *elements;
+    size_t size_;
+
+  public:
+    span(T *, size_t){}
+
+    constexpr T* data() const noexcept {
+      return elements;
+    }
+
+    constexpr size_t size() const noexcept {
+      return size_;
+    }
+
+  };
+}
+
+struct A {
+    [[clang::unsafe_buffer_usage]]
+    int *ptr;
+
+    size_t sz;
+};
+
+struct B {
+   A a;
+ 
+   [[clang::unsafe_buffer_usage]]
+   int buf[];
+};
+
+struct D { 
+  [[clang::unsafe_buffer_usage]]
+  int *ptr, *ptr2;
+
+  [[clang::unsafe_buffer_usage]]
+  int buf[10];
+ 
+  size_t sz;
+  
+};
+
+void foo(int *ptr);
+
+void foo_safe(std::span<int> sp);
+
+int* test_atribute_struct(A a) {
+   int b = *(a.ptr); //expected-warning{{field 'ptr' prone to unsafe buffer manipulation}}
+   a.sz++;
+   // expected-warning at +1{{unsafe pointer arithmetic}}
+   return a.ptr++; //expected-warning{{field 'ptr' prone to unsafe buffer manipulation}}
+}
+
+void test_attribute_field_deref_chain(B b) {
+  int *ptr = b.a.ptr;//expected-warning{{field 'ptr' prone to unsafe buffer manipulation}}
+  foo(b.buf); //expected-warning{{field 'buf' prone to unsafe buffer manipulation}}
+}
+
+void test_writes_from_span(std::span<int> sp) {
+  A a;
+  a.ptr = sp.data(); //expected-warning{{field 'ptr' prone to unsafe buffer manipulation}}
+  a.sz = sp.size();
+
+  a.ptr = nullptr; // expected-warning{{field 'ptr' prone to unsafe buffer manipulation}}
+}
+
+void test_reads_to_span(A a, A b) {
+  //expected-warning at +1{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+  std::span<int> sp {a.ptr, a.sz}; //expected-warning{{field 'ptr' prone to unsafe buffer manipulation}}
+
+  // expected-warning at +1 3{{field 'ptr' prone to unsafe buffer manipulation}}
+  if(a.ptr != nullptr && a.ptr != b.ptr) {
+    foo_safe(sp);
+  }
+
+}
+
+void test_attribute_multiple_fields (D d) {
+   int *p =d.ptr; //expected-warning{{field 'ptr' prone to unsafe buffer manipulation}}
+   p = d.ptr2; //expected-warning{{field 'ptr2' prone to unsafe buffer manipulation}}
+
+   p = d.buf; //expected-warning{{field 'buf' prone to unsafe buffer manipulation}}
+
+   int v = d.buf[0]; //expected-warning{{field 'buf' prone to unsafe buffer manipulation}}
+
+   //expected-warning at +1{{unsafe buffer access}}
+   v = d.buf[5]; //expected-warning{{field 'buf' prone to unsafe buffer manipulation}}
+}
+
+template <typename T>
+struct TemplateArray {
+  [[clang::unsafe_buffer_usage]]
+  T *buf;
+
+  [[clang::unsafe_buffer_usage]]
+  size_t sz;
+};
+
+
+void test_struct_template (TemplateArray<int> t) {
+  int *p = t.buf; //expected-warning{{field 'buf' prone to unsafe buffer manipulation}}
+  size_t s = t.sz; //expected-warning{{field 'sz' prone to unsafe buffer manipulation}}
+}
+
+class R {
+  [[clang::unsafe_buffer_usage]]
+  int *array;
+
+  public:
+   int* getArray() {
+     return array; //expected-warning{{field 'array' prone to unsafe buffer manipulation}}
+   }
+ 
+   void setArray(int *arr) {
+     array = arr; //expected-warning{{field 'array' prone to unsafe buffer manipulation}}
+   }
+};
+
+template<class P>
+class Q {
+  [[clang::unsafe_buffer_usage]]
+  P *array;
+
+  public:
+   P* getArray() {
+     return array; //expected-warning{{field 'array' prone to unsafe buffer manipulation}}
+   }
+
+   void setArray(P *arr) {
+     array = arr; //expected-warning{{field 'array' prone to unsafe buffer manipulation}}
+   }
+};
+
+void test_class_template(Q<R> q) {
+   q.getArray();
+   q.setArray(nullptr);
+}
+
+struct AnonSFields {
+ struct {
+  [[clang::unsafe_buffer_usage]]
+  int a;
+ };
+};
+
+void test_anon_struct_fields(AnonSFields anon) {
+  int val = anon.a; //expected-warning{{field 'a' prone to unsafe buffer manipulation}}
+}
+
+union Union {
+  [[clang::unsafe_buffer_usage]]
+  int *ptr1;
+
+  int ptr2;
+};
+
+struct C {
+  Union ptr;
+};
+
+void test_attribute_union(C c) {
+  int *p = c.ptr.ptr1; //expected-warning{{field 'ptr1' prone to unsafe buffer manipulation}}
+
+  int address = c.ptr.ptr2;
+}
+
+struct AnonFields2 { 
+  [[clang::unsafe_buffer_usage]] 
+  struct { 
+    int a; 
+  }; 
+};
+
+void test_anon_struct(AnonFields2 af) {
+  int val = af.a; // No warning here, as the attribute is not explicitly attached to field 'a'
+  val++;
+}


        


More information about the cfe-commits mailing list