https://github.com/malavikasamak updated
https://github.com/llvm/llvm-project/pull/101585
>From 1ecd91c03f6de92153809402b10f99e5f649787f Mon Sep 17 00:00:00 2001
From: MalavikaSamak <malavi...@apple.com>
Date: Thu, 1 Aug 2024 11:01:36 -0700
Subject: [PATCH 1/4] [-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 4825979a974d22..2cc21d67ddffb2 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 99738812c81579..a52e3dd68a0ce2 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 810abe4f23e31e..b0428d28667a51 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 866222380974b6..b2645c814c3f46 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 0f604c61fa3af9..687b8df4fa2626 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 00000000000000..7650af804df9b4
--- /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}}
+}
>From ffef08e561eaf9b0cf8a138d683e5683b4c0f212 Mon Sep 17 00:00:00 2001
From: MalavikaSamak <malavi...@apple.com>
Date: Mon, 5 Aug 2024 13:11:27 -0700
Subject: [PATCH 2/4] [-Wunsafe-buffer-usage] Extend unsafe_buffer_usage attr
support for struct fields of all types
---
clang/include/clang/Basic/AttrDocs.td | 30 ++--
clang/lib/Analysis/UnsafeBufferUsage.cpp | 18 ++-
clang/lib/Sema/AnalysisBasedWarnings.cpp | 11 +-
.../warn-unsafe-buffer-usage-field-attr.cpp | 129 +++++++++++++-----
4 files changed, 127 insertions(+), 61 deletions(-)
diff --git a/clang/include/clang/Basic/AttrDocs.td
b/clang/include/clang/Basic/AttrDocs.td
index a52e3dd68a0ce2..5c6cdd0d4be3d6 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -6763,16 +6763,15 @@ 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
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.
+The attribute ``[[clang::unsafe_buffer_usage]]`` should be placed on functions
+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.
Attribute attached to functions:
@@ -6844,23 +6843,24 @@ 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.
+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.
+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 and buf will trigger a warning
that the
-field is marked unsafe due to unsafe-buffer operations on it.
+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/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index b2645c814c3f46..b28f46aef9dfcd 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -926,7 +926,7 @@ 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";
+ constexpr static const char *const OpTag = "attr_expr";
const Expr *Op;
public:
@@ -939,16 +939,14 @@ class UnsafeBufferUsageAttrGadget : public WarningGadget {
}
static Matcher matcher() {
- auto HasUnsafeFielDecl =
- member(fieldDecl(allOf(
- anyOf(hasPointerType(), hasArrayType()),
- hasAttr(attr::UnsafeBufferUsage))));
-
+ auto HasUnsafeFielDecl =
+ member(fieldDecl(hasAttr(attr::UnsafeBufferUsage)));
+
auto HasUnsafeFnDecl =
callee(functionDecl(hasAttr(attr::UnsafeBufferUsage)));
- return stmt(expr(anyOf(callExpr(HasUnsafeFnDecl).bind(OpTag),
- memberExpr(HasUnsafeFielDecl).bind(OpTag))));
+ return stmt(expr(anyOf(callExpr(HasUnsafeFnDecl).bind(OpTag),
+ memberExpr(HasUnsafeFielDecl).bind(OpTag))));
}
void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler,
@@ -966,12 +964,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 Expr *Op;
+ const CXXConstructExpr *Op;
public:
UnsafeBufferUsageCtorAttrGadget(const MatchFinder::MatchResult &Result)
: WarningGadget(Kind::UnsafeBufferUsageCtorAttr),
- Op(Result.Nodes.getNodeAs<Expr>(OpTag)) {}
+ Op(Result.Nodes.getNodeAs<CXXConstructExpr>(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 687b8df4fa2626..e6ce89dc7ec406 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2231,7 +2231,7 @@ class UnsafeBufferUsageReporter : public
UnsafeBufferUsageHandler {
SourceLocation Loc;
SourceRange Range;
unsigned MsgParam = 0;
- StringRef name = "";
+ NamedDecl *D = nullptr;
if (const auto *ASE = dyn_cast<ArraySubscriptExpr>(Operation)) {
Loc = ASE->getBase()->getExprLoc();
Range = ASE->getBase()->getSourceRange();
@@ -2266,7 +2266,7 @@ class UnsafeBufferUsageReporter : public
UnsafeBufferUsageHandler {
// 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();
+ D = ME->getMemberDecl();
MsgParam = 5;
} else if (const auto *ECE = dyn_cast<ExplicitCastExpr>(Operation)) {
QualType destType = ECE->getType();
@@ -2292,11 +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 {
- if(!name.empty()) {
- S.Diag(Loc, diag::warn_unsafe_buffer_operation) << MsgParam <<name <<
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/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp
index 7650af804df9b4..64d5e1e3fad7ff 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp
@@ -42,17 +42,6 @@ struct B {
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;
@@ -68,46 +57,124 @@ 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}}
+ 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}}
+ 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}}
+ 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.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_safe_reads(A a) {
+void test_safe_reads(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}}
- foo_safe(sp);
+ 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}}
+ 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}}
+ 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}}
+ 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}}
+ 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 AnonFields {
+ struct {
+ [[clang::unsafe_buffer_usage]]
+ int a;
+ };
+};
+
+void test_anon_fields(AnonFields 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}}
+
+ // TODO: Warn here about the field
+ int address = c.ptr.ptr2;
}
>From 9fe652bd8d5d263e6e10840de76bd8cf5f95ea27 Mon Sep 17 00:00:00 2001
From: MalavikaSamak <malavi...@apple.com>
Date: Thu, 8 Aug 2024 14:36:24 -0700
Subject: [PATCH 3/4] Addressing recent comments
---
clang/include/clang/Basic/AttrDocs.td | 139 +++++++++---------
clang/lib/Analysis/UnsafeBufferUsage.cpp | 4 +-
.../warn-unsafe-buffer-usage-field-attr.cpp | 6 +-
3 files changed, 73 insertions(+), 76 deletions(-)
diff --git a/clang/include/clang/Basic/AttrDocs.td
b/clang/include/clang/Basic/AttrDocs.td
index 5c6cdd0d4be3d6..3cc6edad98599d 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -6773,94 +6773,93 @@ the field it is attached to, and it may also lead to
emission of automatic fix-i
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:
+* 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 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 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++
+ 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 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.
- struct A {
- [[clang::unsafe_buffer_usage]]
- int *ptr1;
+ .. code-block:: c++
- [[clang::unsafe_buffer_usage]]
- int *ptr2, buf[10];
+ struct A {
+ [[clang::unsafe_buffer_usage]]
+ int *ptr1;
- [[clang::unsafe_buffer_usage]]
- size_t sz;
- };
+ [[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.
+ 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/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index b28f46aef9dfcd..b1be0ea4d27b0a 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -945,8 +945,8 @@ class UnsafeBufferUsageAttrGadget : public WarningGadget {
auto HasUnsafeFnDecl =
callee(functionDecl(hasAttr(attr::UnsafeBufferUsage)));
- return stmt(expr(anyOf(callExpr(HasUnsafeFnDecl).bind(OpTag),
- memberExpr(HasUnsafeFielDecl).bind(OpTag))));
+ return stmt(anyOf(callExpr(HasUnsafeFnDecl).bind(OpTag),
+ memberExpr(HasUnsafeFielDecl).bind(OpTag)));
}
void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler,
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp
b/clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp
index 64d5e1e3fad7ff..2551d8e9a4db42 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp
@@ -69,16 +69,15 @@ void test_attribute_field_deref_chain(B b) {
foo(b.buf); //expected-warning{{field 'buf' prone to unsafe buffer
manipulation}}
}
-void test_safe_writes(std::span<int> sp) {
+void test_writes_from_span(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();
a.ptr = nullptr; // expected-warning{{field 'ptr' prone to unsafe buffer
manipulation}}
}
-void test_safe_reads(A a, A b) {
+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}}
@@ -175,6 +174,5 @@ struct C {
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;
}
>From a214f888e19c70d507b25c14ac43a63be3516ddb Mon Sep 17 00:00:00 2001
From: MalavikaSamak <malavi...@apple.com>
Date: Thu, 8 Aug 2024 15:50:39 -0700
Subject: [PATCH 4/4] Fixing clang-format issue
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index b1be0ea4d27b0a..e343c2c48105c1 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -946,7 +946,7 @@ class UnsafeBufferUsageAttrGadget : public WarningGadget {
callee(functionDecl(hasAttr(attr::UnsafeBufferUsage)));
return stmt(anyOf(callExpr(HasUnsafeFnDecl).bind(OpTag),
- memberExpr(HasUnsafeFielDecl).bind(OpTag)));
+ memberExpr(HasUnsafeFielDecl).bind(OpTag)));
}
void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
