llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Malavika Samak (malavikasamak) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/101585.diff 6 Files Affected: - (modified) clang/include/clang/Basic/Attr.td (+1-1) - (modified) clang/include/clang/Basic/AttrDocs.td (+36-9) - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+2-1) - (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+12-5) - (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+12-1) - (added) clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp (+113) ``````````diff diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td index 4825979a974d2..2cc21d67ddffb 100644 --- a/clang/include/clang/Basic/Attr.td +++ b/clang/include/clang/Basic/Attr.td @@ -4447,7 +4447,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 99738812c8157..a52e3dd68a0ce 100644 --- a/clang/include/clang/Basic/AttrDocs.td +++ b/clang/include/clang/Basic/AttrDocs.td @@ -6763,15 +6763,18 @@ attribute requires a string literal argument to identify the handle being releas 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 -alternatives, though the attribute can be used even when the fix can't be automated. +The attribute ``[[clang::unsafe_buffer_usage]]`` should be placed on functions or +struct fields that are buffers, that must 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 or +the field it is attached to, and it may enable ``-Wunsafe-buffer-usage`` to emit +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. + +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. @@ -6835,6 +6838,30 @@ the proper solution would be to create a different 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]; + + size_t sz; + }; + +Here, every read/write to the fields ptr1, ptr2 and buf will trigger a warning that the +field is marked unsafe due to unsafe-buffer operations on it. + }]; } diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 810abe4f23e31..b0428d28667a5 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -12371,7 +12371,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 866222380974b..b2645c814c3f4 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -927,21 +927,28 @@ class CArrayToPtrAssignmentGadget : public FixableGadget { /// over one of its pointer parameters. class UnsafeBufferUsageAttrGadget : public WarningGadget { constexpr static const char *const OpTag = "call_expr"; - const CallExpr *Op; + 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 HasUnsafeFielDecl = + member(fieldDecl(allOf( + anyOf(hasPointerType(), hasArrayType()), + hasAttr(attr::UnsafeBufferUsage)))); + auto HasUnsafeFnDecl = callee(functionDecl(hasAttr(attr::UnsafeBufferUsage))); - return stmt(callExpr(HasUnsafeFnDecl).bind(OpTag)); + + return stmt(expr(anyOf(callExpr(HasUnsafeFnDecl).bind(OpTag), + memberExpr(HasUnsafeFielDecl).bind(OpTag)))); } void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler, @@ -959,12 +966,12 @@ class UnsafeBufferUsageAttrGadget : public WarningGadget { /// perform buffer operations that depend on the correctness of the parameters. class UnsafeBufferUsageCtorAttrGadget : public WarningGadget { constexpr static const char *const OpTag = "cxx_construct_expr"; - const CXXConstructExpr *Op; + const Expr *Op; public: UnsafeBufferUsageCtorAttrGadget(const MatchFinder::MatchResult &Result) : WarningGadget(Kind::UnsafeBufferUsageCtorAttr), - Op(Result.Nodes.getNodeAs<CXXConstructExpr>(OpTag)) {} + Op(Result.Nodes.getNodeAs<Expr>(OpTag)) {} static bool classof(const Gadget *G) { return G->getKind() == Kind::UnsafeBufferUsageCtorAttr; diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 0f604c61fa3af..687b8df4fa262 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; + StringRef name = ""; 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); + name = ME->getMemberDecl()->getName(); + MsgParam = 5; } else if (const auto *ECE = dyn_cast<ExplicitCastExpr>(Operation)) { QualType destType = ECE->getType(); if (!isa<PointerType>(destType)) @@ -2285,7 +2292,11 @@ 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(!name.empty()) { + S.Diag(Loc, diag::warn_unsafe_buffer_operation) << MsgParam <<name << 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/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp new file mode 100644 index 0000000000000..7650af804df9b --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp @@ -0,0 +1,113 @@ +// 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[]; +}; + +union Union { + [[clang::unsafe_buffer_usage]] + int *ptr1; + + int ptr2; +}; + +struct C { + Union ptr; +}; + +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); + +void test_attribute_union(C c) { + int *p = c.ptr.ptr1; //expected-warning{{field ptr1 prone to unsafe buffer manipulation}} + + // TODO: Warn here about the field + int address = c.ptr.ptr2; +} + +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_safe_writes(std::span<int> sp) { + A a; + // TODO: We should not warn for safe assignments from hardened types + a.ptr = sp.data(); //expected-warning{{field ptr prone to unsafe buffer manipulation}} + a.sz = sp.size(); +} + +void test_safe_reads(A a) { + //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}} + 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}} +} `````````` </details> https://github.com/llvm/llvm-project/pull/101585 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
