Skip to content

[attributes][-Wunsafe-buffer-usage] Support adding unsafe_buffer_usage attribute to struct fields #101585

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 14, 2024
Merged

[attributes][-Wunsafe-buffer-usage] Support adding unsafe_buffer_usage attribute to struct fields #101585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clang/include/clang/Basic/Attr.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}

Expand Down
136 changes: 81 additions & 55 deletions clang/include/clang/Basic/AttrDocs.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6764,77 +6764,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 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.
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++

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel the need to add something constructive here, to give people a way to eliminate the warning.

"Together with adding the attribute, it is recommended to provide a safe way of accessing these structs. Under our assumption that the fields cannot be edited, or even made private, for compatibility reasons, one possible solution is to provide safe span-based accessor methods to these fields, then use the attribute to encourage users to use those methods, without breaking compatibility if they don't:

struct A {
    [[clang::unsafe_buffer_usage]]
    int *ptr;
    [[clang::unsafe_buffer_usage]]
    size_t sz;

    std::span<int> getPtrAsSpan() {
        #pragma clang unsafe_buffer_usage begin
        return std::span{ptr, sz};
        #pragma clang unsafe_buffer_usage end
    }

    void setPtrFromSpan(std::span<int> sp) {
        #pragma clang unsafe_buffer_usage begin
        ptr = sp.data();
        sz = sp.size();
        #pragma clang unsafe_buffer_usage begin
    }
}

".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline - what I proposed is "a" solution but very much not necessarily "the" solution so we probably shouldn't outright recommend it in a document as low-level as compiler documentation.

}];
}

Expand Down
3 changes: 2 additions & 1 deletion clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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">;
Expand Down
13 changes: 9 additions & 4 deletions clang/lib/Analysis/UnsafeBufferUsage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
14 changes: 13 additions & 1 deletion clang/lib/Sema/AnalysisBasedWarnings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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))
Expand All @@ -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);
}
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Misc/pragma-attribute-supported-attributes-list.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,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)
Expand Down
Loading
Loading