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 <malavi...@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@+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@+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@+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@+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++; +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
