[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