[clang] [-Wunsafe-buffer-usage] Support adding unsafe_buffer_usage attribute to struct fields (PR #101585)
Malavika Samak via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 1 16:46:55 PDT 2024
https://github.com/malavikasamak created https://github.com/llvm/llvm-project/pull/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.
>From 1ecd91c03f6de92153809402b10f99e5f649787f Mon Sep 17 00:00:00 2001
From: MalavikaSamak <malavika2 at apple.com>
Date: Thu, 1 Aug 2024 11:01:36 -0700
Subject: [PATCH] [-Wunsafe-buffer-usage] Support adding unsafe_buffer_usage
aatribute to struct fields.
---
clang/include/clang/Basic/Attr.td | 2 +-
clang/include/clang/Basic/AttrDocs.td | 45 +++++--
.../clang/Basic/DiagnosticSemaKinds.td | 3 +-
clang/lib/Analysis/UnsafeBufferUsage.cpp | 17 ++-
clang/lib/Sema/AnalysisBasedWarnings.cpp | 13 +-
.../warn-unsafe-buffer-usage-field-attr.cpp | 113 ++++++++++++++++++
6 files changed, 176 insertions(+), 17 deletions(-)
create mode 100644 clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp
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 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_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 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}}
+ 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}}
+}
More information about the cfe-commits
mailing list