[-Wunsafe-buffer-usage] Introduce the `unsafe_buffer_usage` attribute

[t-rasmud]

Summary

• Introduces the unsafe_buffer_usage function attribute.
• Issues a warning at call sites to functions wearing this attribute.

[jkorous]

clang/include/clang/Basic/AttrDocs.td

6214

We should keep this in-sync with:
https://reviews.llvm.org/D136811

clang/lib/Sema/SemaDeclAttr.cpp

8451

I think we should remove the commented out code.

[NoQ]

So here's what we roughly think about the future of this attribute.

In this initial patch the idea is that we don't restrict the applicability of the attribute in any way. It can be put on functions that don't have any pointer parameters, or any parameters at all, or any return values, they can be plain functions or methods, whatever. The attribute simply says "This function isn't compatible with the Safe Buffers programming model" without specifying where the buffers are or where the bounds are or how to mitigate the problem.

Then later we're going to expand the attribute to offer automatic fixes. Then the default mode of this attribute should probably mean "There's an automatic fix, and it's *obvious*", eg.:

[[clang::unsafe_buffer_usage]]
void foo(int *buf, size_t size);

void foo(std::span<int> buf);

foo(buf, size); // obvious automatic fix: foo(std::span(buf, __placeholder__));

And we're thinking of attribute misuse warning that informs the user that "The fix isn't obvious enough" (eg., couldn't figure out how to match parameters):

[[clang::unsafe_buffer_usage]] // warning: cannot figure out the automatic fix
void foo(int *buf, size_t size);

void foo(std::span<int> sp); // note: no parameter named 'buf'

foo(buf, size); // warn about unsafe operation but no fixit

And, separately, there will be a "mode" of this attribute that says "There's no automatic fix, please fix all call sites manually" which corresponds to the current patch's implementation (maybe includes a human-readable explanation of how to mitigate the problem) (exact syntax TBD):

[[clang::unsafe_buffer_usage(__nofix, "use the bar() interface instead")]] // no warning, the attribute is fine
void foo(int *buf, size_t size);

foo(buf, size); // warning: use the bar() interface instead

Why do I think the default mode should provide a fix? Imagine you're a library vendor, and you're updating your APIs to conform to the Safe Buffers programming model. Then you may add an attribute, but you don't necessarily have an easy way to test whether the compiler will emit an automatic fix or not. And if you forget to test that, it silently breaks our entire tool, because any user code can no longer be modernized automatically. Or if the function definitions diverge and the original automatic fix can no longer be applied, that'd be another silent breakage scenario.

Which is why I think __nofix *cannot* be the default behavior. If folks are breaking our tool (that they clearly intended to support, given that they added the attribute in the first place), they should know about that and explicitly opt in. And the good news is, the compiler can fully figure out whether an automatic fix is theoretically possible, by looking only at the prototypes. So it has enough information to notify people if they're breaking our tool.

Then, there's a question of how smart the default behavior should be. It's a matter of discussion how deep do we want to go, but I believe quality of life is important there. If our hypothetical library vendors can't figure out how to satisfy the attribute's prerequisites, they'll give up and go for __nofix which, again, diminishes the usefulness of our tool.

Then later we'll probably add attribute arguments through which the user can provide more hints to the compiler, eg. if it couldn't match parameters (exact syntax TBD):

[[clang::unsafe_buffer_usage(buf=sp)]]
void foo(int *buf, size_t size);

void foo(std::span<int> sp);

foo(buf, size); // assisted automatic fix: foo(std::span(buf, __placeholder__));

Or if the safe function isn't an overload:

[[clang::unsafe_buffer_usage(foo=foo2)]]
void foo(int *buf, size_t size);

void foo2(std::span<int> buf);

foo(buf, size); // assisted automatic fix: foo2(std::span(buf, __placeholder__));

We'll see how deep do we want to dig into this. Obviously, for all these cornercases, we'll have to teach the fixit machine to handle them, otherwise it's pointless to support them. It might be beneficial to consider a "full control" mode in which no automatic name matching of any kind takes place, for all the people who don't want to rely on the compiler to do magic for them.

clang/include/clang/Basic/AttrDocs.td

6212

Looks like this part is an accidental duplicate of the one below.

clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp

2

Do you still need -Wno-deprecated-declarations? I think the test file doesn't have any deprecated declarations anyway.

[NoQ]

clang/include/clang/Basic/Attr.td

3971

In this initial patch the idea is that we don't restrict the applicability of the attribute in any way.

Well, we restrict it to *functions*, but I'm not even sure this is necessary. It might make sense in some cases to have this attribute on, say, entire *classes*. But we can extend that later.

Another good target for the attribute is Objective-C methods (which aren't FunctionDecls). Because Objective-C++ is a thing and folks who use it might be interested. But, again, we don't have any concrete data on that.

[NoQ]

+ Erich!

[erichkeane]

I need a way better commit message... I have no idea what this is doing on first read. NoQ's comment is perhaps a good start.

That said, I don't like this form of incremental approach. Doing it this way means the additions you want to do could end up being breaking changes for folks. We should limit where/how this can be called, and with what parameters down to ONLY what functionality we're implementing with it, and add 1 of those functionalities, fully tested, at a time.

But allowing it to fully appertain to anything means that we are going to have a harder time changing what that menas in the future.

clang/include/clang/Basic/AttrDocs.td

6280

This doc is woefully unspecified here.

[NoQ]

That said, I don't like this form of incremental approach. Doing it this way means the additions you want to do could end up being breaking changes for folks. We should limit where/how this can be called, and with what parameters down to ONLY what functionality we're implementing with it, and add 1 of those functionalities, fully tested, at a time.

Fair enough, we should probably avoid intentionally breaking people. So I guess starting with a

[[clang::unsafe_buffer_usage(__nofix, "use the bar() interface instead")]]

variant (where the hint is mandatory) would make sure we're starting with a very strict model that only gets more relaxed over time without breaking existing uses.

But we may also run into hypothetical situations when, say, more unforeseen data comes in from our users, and the data would indicate that we have to change something in a breaking way. Would such breakage also be completely unacceptable, or can we do that under certain conditions? Should we consider this possibility and prepare for it by, say, telling the users in documentation that the attribute is experimental and may break? (Of course, we'll do our best to eliminate that clause and stabilize the syntax as soon as possible.)

[erichkeane]

In D138940#3976154, @NoQ wrote:

That said, I don't like this form of incremental approach. Doing it this way means the additions you want to do could end up being breaking changes for folks. We should limit where/how this can be called, and with what parameters down to ONLY what functionality we're implementing with it, and add 1 of those functionalities, fully tested, at a time.

Fair enough, we should probably avoid intentionally breaking people. So I guess starting with a

[[clang::unsafe_buffer_usage(__nofix, "use the bar() interface instead")]]

variant (where the hint is mandatory) would make sure we're starting with a very strict model that only gets more relaxed over time without breaking existing uses.

But we may also run into hypothetical situations when, say, more unforeseen data comes in from our users, and the data would indicate that we have to change something in a breaking way. Would such breakage also be completely unacceptable, or can we do that under certain conditions? Should we consider this possibility and prepare for it by, say, telling the users in documentation that the attribute is experimental and may break? (Of course, we'll do our best to eliminate that clause and stabilize the syntax as soon as possible.)

Telling users the attribute is experimental is an acceptable way to get around 'unforseen' issues, but we should still strive to make as few breaking changes as possible. That is, don't use said documentation as an excuse to break users arbitrarily, else you'll have a difficult time getting folks to use this/try this during its 'experimental' time. That is, every time you break it, you're giving existing users an opportunity to just say "eh, lets just remove all of these from our code, not worth the extra effort", and future users a reason to say, "eh, lets just wait until later (read: never) and see if it is worth using when it becomes more popular".

[tschuett]

The summary says usage_buffer_usage, which really confused me. For such an invasive feature, I would expect a more detailed summary.

[NoQ]

In D138940#3978039, @erichkeane wrote:

Telling users the attribute is experimental is an acceptable way to get around 'unforseen' issues, but we should still strive to make as few breaking changes as possible. That is, don't use said documentation as an excuse to break users arbitrarily, else you'll have a difficult time getting folks to use this/try this during its 'experimental' time. That is, every time you break it, you're giving existing users an opportunity to just say "eh, lets just remove all of these from our code, not worth the extra effort", and future users a reason to say, "eh, lets just wait until later (read: never) and see if it is worth using when it becomes more popular".

Yes, absolutely! We'll definitely avoid foresee-able breakages as much as possible.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

323–338

In a nearby patch we've agreed to unhardcode/reuse the .bind tags like this!

[jkorous]

clang/lib/Sema/SemaDeclAttr.cpp

9335

Nit: git just complained we should not have whitespace at the end of lines.

[t-rasmud]

Here's an update on how we plan to implement the attribute after considering feedback from everyone:

The semantics of our function attribute was originally "calls to this function are unsafe".
Now we are thinking about it more like "calls to this function are unsafe AND automatically fixable".

We propose a design where the attribute has parameters and the above is the default. The valid forms and how clang responds to them would be:

• Implementation Stage 1: [[clang::unsafe_buffer_usage]]
  • Clang emits -Wunsafe_buffer_usage warning at call-sites, fixit emitted on best-effort basis.
• Implementation Stage 2: [[clang::unsafe_buffer_usage(arguments for fixit generation...)]]
  • Example of the parameter use-cases: foo/bar (allows to emit a fixit that calls function bar instead of foo, another example is when the user changes parameters of the safe overload (e. g. remove a “size” parameter).
  • Fixits are still on best-effort basis.
• Implementation Stage 3: [[clang::unsafe_buffer_usage(__no_autofix)]]
  • Clang emits a different warning (TBD) at the attribute if we think we won’t be able to emit a fixit for call-sites (when we can’t find an overload with exactly 1 different parameter type changed from T* to std::span<T>).
  • __no_autofix parameter suppresses that warning.

Note that the TBD warning is disabled by default. In order to see these warnings the users not only have to opt in into using the attribute (by turning on -Wunsafe_buffer_usage), but also opt in into seeing this specific (TBD) warning group.
With this strategy, we think we can avoid the situation where we introduce changes that will break code for people who adopted a previous version of the attribute.
We're curious to see what everyone thinks of this incremental strategy.

[erichkeane]

In D138940#4030158, @t-rasmud wrote:

Here's an update on how we plan to implement the attribute after considering feedback from everyone:

The semantics of our function attribute was originally "calls to this function are unsafe".
Now we are thinking about it more like "calls to this function are unsafe AND automatically fixable".

We propose a design where the attribute has parameters and the above is the default. The valid forms and how clang responds to them would be:

• Implementation Stage 1: [[clang::unsafe_buffer_usage]]
  • Clang emits -Wunsafe_buffer_usage warning at call-sites, fixit emitted on best-effort basis.
• Implementation Stage 2: [[clang::unsafe_buffer_usage(arguments for fixit generation...)]]
  • Example of the parameter use-cases: foo/bar (allows to emit a fixit that calls function bar instead of foo, another example is when the user changes parameters of the safe overload (e. g. remove a “size” parameter).
  • Fixits are still on best-effort basis.
• Implementation Stage 3: [[clang::unsafe_buffer_usage(__no_autofix)]]
  • Clang emits a different warning (TBD) at the attribute if we think we won’t be able to emit a fixit for call-sites (when we can’t find an overload with exactly 1 different parameter type changed from T* to std::span<T>).
  • __no_autofix parameter suppresses that warning.

Note that the TBD warning is disabled by default. In order to see these warnings the users not only have to opt in into using the attribute (by turning on -Wunsafe_buffer_usage), but also opt in into seeing this specific (TBD) warning group.
With this strategy, we think we can avoid the situation where we introduce changes that will break code for people who adopted a previous version of the attribute.
We're curious to see what everyone thinks of this incremental strategy.

Can you add more details to that proposal? This is a 'devil in the details' sorta thing for me. Else, feel free to start implementing stage-1 (I suspect you've proposing what I commented on earlier), and I can comment on it there.

[NoQ]

In D138940#4031366, @erichkeane wrote:

Can you add more details to that proposal? This is a 'devil in the details' sorta thing for me. Else, feel free to start implementing stage-1 (I suspect you've proposing what I commented on earlier), and I can comment on it there.

Yeah so the change of plans compared to my initial take mostly boils down to this part:

Why do I think the default mode should provide a fix? Imagine you're a library vendor, and you're updating your APIs to conform to the Safe Buffers programming model. Then you may add an attribute, but you don't necessarily have an easy way to test whether the compiler will emit an automatic fix or not. And if you forget to test that, it silently breaks our entire tool, because any user code can no longer be modernized automatically. Or if the function definitions diverge and the original automatic fix can no longer be applied, that'd be another silent breakage scenario.

Which is why I think __nofix *cannot* be the default behavior. If folks are breaking our tool (that they clearly intended to support, given that they added the attribute in the first place), they should know about that and explicitly opt in. And the good news is, the compiler can fully figure out whether an automatic fix is theoretically possible, by looking only at the prototypes. So it has enough information to notify people if they're breaking our tool.

So we no longer believe this is true. We think that if somebody is *not* a library vendor, then for them the requirement to annotate everything that isn't autofixable may be overly tedious. So now we're thinking that the default, argument-less syntax

[[clang::unsafe_buffer_usage]]

should be always allowed and always mean only "this function is unsafe", regardless of whether the automatic fix is possible or sufficiently obvious from context.

Then eventually we'll implement a separate warning to verify that the automatic fix is possible and recommends adding extra information into the attribute if it's not sufficient as is:

Implementation Stage 3: [[clang::unsafe_buffer_usage(__no_autofix)]]

• Clang emits a different warning (TBD) at the attribute if we think we won’t be able to emit a fixit for call-sites
• __no_autofix parameter suppresses that warning.

Note that the TBD warning is disabled by default. In order to see these warnings the users not only have to opt in into using the attribute (by turning on -Wunsafe_buffer_usage), but also opt in into seeing this specific (TBD) warning group.

But it is going to be a separate warning that requires separate opt-in from the user. So existing code that uses [[clang::unsafe_buffer_usage]] will not be "broken" by the introduction of this new warning (but only by the users opting into the extra thing), which (partially?) addresses the incremental development concerns that you voiced earlier. WDYT, would such landing strategy be acceptable?

Also, yeah, I think we should start properly documenting these things. It's arguably clear enough as of today, we can already write something down.

[erichkeane]

In D138940#4032836, @NoQ wrote:

In D138940#4031366, @erichkeane wrote:

Can you add more details to that proposal? This is a 'devil in the details' sorta thing for me. Else, feel free to start implementing stage-1 (I suspect you've proposing what I commented on earlier), and I can comment on it there.

Yeah so the change of plans compared to my initial take mostly boils down to this part:

Why do I think the default mode should provide a fix? Imagine you're a library vendor, and you're updating your APIs to conform to the Safe Buffers programming model. Then you may add an attribute, but you don't necessarily have an easy way to test whether the compiler will emit an automatic fix or not. And if you forget to test that, it silently breaks our entire tool, because any user code can no longer be modernized automatically. Or if the function definitions diverge and the original automatic fix can no longer be applied, that'd be another silent breakage scenario.

Which is why I think __nofix *cannot* be the default behavior. If folks are breaking our tool (that they clearly intended to support, given that they added the attribute in the first place), they should know about that and explicitly opt in. And the good news is, the compiler can fully figure out whether an automatic fix is theoretically possible, by looking only at the prototypes. So it has enough information to notify people if they're breaking our tool.

So we no longer believe this is true. We think that if somebody is *not* a library vendor, then for them the requirement to annotate everything that isn't autofixable may be overly tedious. So now we're thinking that the default, argument-less syntax

[[clang::unsafe_buffer_usage]]

should be always allowed and always mean only "this function is unsafe", regardless of whether the automatic fix is possible or sufficiently obvious from context.

Then eventually we'll implement a separate warning to verify that the automatic fix is possible and recommends adding extra information into the attribute if it's not sufficient as is:

Implementation Stage 3: [[clang::unsafe_buffer_usage(__no_autofix)]]

• Clang emits a different warning (TBD) at the attribute if we think we won’t be able to emit a fixit for call-sites
• __no_autofix parameter suppresses that warning.

Note that the TBD warning is disabled by default. In order to see these warnings the users not only have to opt in into using the attribute (by turning on -Wunsafe_buffer_usage), but also opt in into seeing this specific (TBD) warning group.

But it is going to be a separate warning that requires separate opt-in from the user. So existing code that uses [[clang::unsafe_buffer_usage]] will not be "broken" by the introduction of this new warning (but only by the users opting into the extra thing), which (partially?) addresses the incremental development concerns that you voiced earlier. WDYT, would such landing strategy be acceptable?

Also, yeah, I think we should start properly documenting these things. It's arguably clear enough as of today, we can already write something down.

I'm unconcerned about people being broken by a new warning. I suggest that the overall 'Wunsafe-buffer-usage' (or whatever the top-level warning group is) ALWAYS include every warning (with sub-warning-classes to disable individuals if you'd like). The whole point of new warnings is to fire on existing code as QoI improves, so that is expected.

What is NOT acceptable to me is that once an attribute form is written, that it becomes ill-formed in some way. So allowing the attribute in the empty-form on every function (limited to functions, right?), then it cannot EVER become not-allowed/diagnosed for that. If you allow a parameter to it, that form can never be disallowed, etc.

My suggestion was to only add it to functions that you KNOW you can diagnose in the exact form you know you can diagnose, so you don't get stuck in a place where you cannot make something illegal. I believe this is WORSE in cases where you allow parameters without giving immediate meaning to them, but is worth making sure you DO want to someday allow this in the same form on every function. So, are you ok with this being on C variadics, template variadics, prototypeless functions, etc?

[NoQ]

In D138940#4036211, @erichkeane wrote:

What is NOT acceptable to me is that once an attribute form is written, that it becomes ill-formed in some way. So allowing the attribute in the empty-form on every function (limited to functions, right?), then it cannot EVER become not-allowed/diagnosed for that. If you allow a parameter to it, that form can never be disallowed, etc.

My suggestion was to only add it to functions that you KNOW you can diagnose in the exact form you know you can diagnose, so you don't get stuck in a place where you cannot make something illegal. I believe this is WORSE in cases where you allow parameters without giving immediate meaning to them, but is worth making sure you DO want to someday allow this in the same form on every function. So, are you ok with this being on C variadics, template variadics, prototypeless functions, etc?

I think we're on the same page here, with our updated proposal. We're going to permanently allow the attribute in its current form as proposed by this patch, on arbitrary functions. Some code will have it written this way and that's perfectly ok for that code to not be concerned about automatic fixability at all. We're never going to make it illegal.

I also don't see any problems with the special function kinds you've listed. It's perfectly reasonable to let the developer annotate the function as "Well, this thing is using some unsafe buffers here and there, and I refuse to specify how or where exactly", no matter how the function is declared and what information about the function is available or how much flexibility it offers. Such annotation is perfectly acceptable and sufficient in many cases. Then anybody who uses this function will know that they have to avoid it if they're opting into the safe buffers programming model (but not if they aren't opting into it), but it's likely that they'll have to figure out themselves what to replace it with (which is in many cases inevitable anyway).

With templates, it probably makes sense to test the behavior of the attribute on explicit instantiations. Eg., https://godbolt.org/z/djbTqbYET but with [[clang::unsafe_buffer_usage]] instead of [[deprecated]] - see how it only warns about the second call, we probably want the same behavior. Though it's probably the default behavior anyway.

In D138940#4036211, @erichkeane wrote:

I suggest that the overall 'Wunsafe-buffer-usage' (or whatever the top-level warning group is) ALWAYS include every warning (with sub-warning-classes to disable individuals if you'd like).

Yeah, I think this is reasonable, maybe it's not that important to keep them completely separate 🤔 Even if the new warning gets enabled unnecessarily on some code, the user will immediately know how to disable it without disabling the entire machine. I think we'll indeed go with a subgroup.

[t-rasmud]

Improves documentation and addresses feedback.

[NoQ]

Aha, great, you reused a chunk of D136811 to document the attribute. I have a few suggestions how to make it make sense to the reader who didn't read the documentation in D136811, because even when D136811 lands there will still be people who learn about the attribute for the first time from https://clang.llvm.org/docs/AttributeReference.html and not from the document in D136811.

clang/include/clang/Basic/AttrDocs.td

6279–6282

(the last part is carried over from below because it sounds important enough to say up front)

6297–6302

6312–6315

6324

Looks like a formatting typo (those pesky review comments probably got in the way).

6332–6336

Hmm, this part is hard to understand even in the full context of D136811, so that's on me. Let me try to do a better job here.

I moved the part about escape hatch to the beginning of the document because it directly describes what the attribute does, which is important to say up front in this kind of documentation.

6338–6347

I think we can drop this section entirely. It's mostly motivational, doesn't add much.

6349

This TODO can be removed, because there are no parameters yet. When we implement the parameters, we'll still need to document them.

[t-rasmud]

Improves documentation for unsafe_buffer_usage attribute

[NoQ]

Ok with the new documentation I think this patch is mostly good to go! @erichkeane WDYT, are we missing anything?

At a glance it looks like we're short on tests. We might be happy about the default behavior of everything, but it's important to confirm that we actually understand them, by adding tests that document the intended behavior and protect it from regressing. So I think it's worth it to add the cornercases Erich mentioned:

In D138940#4036211, @erichkeane wrote:

So, are you ok with this being on C variadics, template variadics, prototypeless functions, etc?

as well as the cornercase I brought up in response:

In D138940#4041591, @NoQ wrote:

With templates, it probably makes sense to test the behavior of the attribute on explicit instantiations. Eg., https://godbolt.org/z/djbTqbYET but with [[clang::unsafe_buffer_usage]] instead of [[deprecated]] - see how it only warns about the second call, we probably want the same behavior. Though it's probably the default behavior anyway.

and probably interactions with C++ inheritance (if the attribute is on the base class method, is the overriding method also warned about?, etc.).

clang/lib/Analysis/UnsafeBufferUsage.cpp

392

Formatting ^.^

393–395

This FIXME is a bit stale because after D140062 WarningGadgets aren't responsible for emitting fixits.

[NoQ]

At some point we'd want to add this to release notes just like we did with D135851. But we probably want to do that for the entire -Wunsafe-buffer-usage feature, not for individual elements like this attribute. I think I'll add a release note to D136811 so that it landed at the same time as we start considering the feature is "usable" (for that we need to have an initial implementation of all the enforcement and opt-out elements - warnings, pragmas (D140179), the attribute, but not necessarily fixits).

[t-rasmud]

Adds tests for variadic functions, templates.

[NoQ]

Ok, let's add a virtual method / inheritance test and IMHO we're good to go!

clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp

22–23

Hmm, maybe we should have a specialized warning/note text for these functions? Passing a pointer into these functions isn't "pointer operation" in and of itself. Maybe we should teach the handler class to say something like

warning: function 'deprecatedFunction3' introduces unsafe buffer manipulation

or when we have a variable:

warning: 'z' is an unsafe pointer used for buffer access
note: passed into 'deprecatedFunction4' here, which uses the buffer unsafely

? (in this case 'z' isn't a pointer but you get the point) (could be a follow-up patch)

[t-rasmud]

Adds test cases for the unsafe_buffer_usage attribute on virtual functions.

[t-rasmud]

@erichkeane We'd love to get your feedback on the latest revision, thanks!

[NoQ]

As for me, this looks like it's good to go! Thanks Rashmi for addressing all the comments!

[erichkeane]

Needs a release note, otherwise looks fine to me.

[t-rasmud]

Adds a release note for unsafe_buffer_usage attribute.

clang/docs/ReleaseNotes.rst

 
 New Pragmas in Clang
 --------------------
 - ...
 
 Attribute Changes in Clang
 --------------------------
 
+Introduced a new function attribute ``__attribute__((unsafe_buffer_usage))``
+to be worn by functions containing buffer operations that could cause out of
+bounds memory accesses. It emits warnings at call sites to such functions when
+the flag ``-Wunsafe-buffer-usage`` is enabled.
+
 Windows Support
 ---------------
 
 AIX Support
 -----------
 
 C Language Changes in Clang
 ---------------------------

clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def

 #ifndef FIXABLE_GADGET
 #define FIXABLE_GADGET(name) GADGET(name)
 #endif
 
 WARNING_GADGET(Increment)
 WARNING_GADGET(Decrement)
 WARNING_GADGET(ArraySubscript)
 WARNING_GADGET(PointerArithmetic)
+WARNING_GADGET(UnsafeBufferUsageAttr)
 
 #undef FIXABLE_GADGET
 #undef WARNING_GADGET
 #undef GADGET

clang/include/clang/Basic/Attr.td

 
 def ReleaseHandle : InheritableParamAttr {
   let Spellings = [Clang<"release_handle">];
   let Args = [StringArgument<"HandleType">];
   let Subjects = SubjectList<[ParmVar]>;
   let Documentation = [ReleaseHandleDocs];
 }
 
+def UnsafeBufferUsage : InheritableAttr {
+  let Spellings = [Clang<"unsafe_buffer_usage">];
+  let Subjects = SubjectList<[Function]>;
+  let Documentation = [UnsafeBufferUsageDocs];
+}
+
 def DiagnoseAsBuiltin : InheritableAttr {
   let Spellings = [Clang<"diagnose_as_builtin">];
   let Args = [DeclArgument<Function, "Function">,
               VariadicUnsignedArgument<"ArgIndices">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [DiagnoseAsBuiltinDocs];
 }
 

clang/include/clang/Basic/AttrDocs.td

 position. If the entity referred to by the using-declaration is found by name
 lookup, the attribute has no effect. This attribute is useful for libraries
 (primarily, libc++) that wish to redeclare a set of declarations in another
 namespace, when the availability of those declarations is difficult or
 impossible to detect at compile time with the preprocessor.
   }];
 }
 
 def HandleDocs : DocumentationCategory<"Handle Attributes"> {
   let Content = [{
 Handles are a way to identify resources like files, sockets, and processes.
 They are more opaque than pointers and widely used in system programming. They
 have similar risks such as never releasing a resource associated with a handle,
 attempting to use a handle that was already released, or trying to release a
 handle twice. Using the annotations below it is possible to make the ownership
 of the handles clear: whose responsibility is to release them. They can also
 aid static analysis tools to find bugs.
   }];
 }

 attribute requires a string literal argument to identify the handle being released.
 
 .. code-block:: c++
 
   zx_status_t zx_handle_close(zx_handle_t handle [[clang::release_handle("tag")]]);
   }];
 }
 
+def UnsafeBufferUsageDocs : Documentation {
+  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 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:
+
+.. 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;
+    }
+  }
+
+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++
+
+  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``.
+
+The attribute is warranted when a function accepts a raw buffer only to
+immediately put it into a span:
+
+.. 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;
+    }
+  }
+
+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.
+  }];
+}
+
 def DiagnoseAsBuiltinDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 The ``diagnose_as_builtin`` attribute indicates that Fortify diagnostics are to
 be applied to the declared function as if it were the function specified by the
 attribute. The builtin function whose diagnostics are to be mimicked should be
 given. In addition, the order in which arguments should be applied must also
 be given.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 
 // Unsafe buffer usage diagnostics.
 def warn_unsafe_buffer_variable : Warning<
   "%0 is an %select{unsafe pointer used for buffer access|unsafe buffer that "
   "does not perform bounds checks}1">,
   InGroup<UnsafeBufferUsage>, DefaultIgnore;
 def warn_unsafe_buffer_operation : Warning<
   "%select{unsafe pointer operation|unsafe pointer arithmetic|"
-  "unsafe buffer access}0">,
+  "unsafe buffer access|function introduces unsafe buffer manipulation}0">,
   InGroup<UnsafeBufferUsage>, DefaultIgnore;
 def note_unsafe_buffer_operation : Note<
   "used%select{| in pointer arithmetic| in buffer access}0 here">;
 def err_loongarch_builtin_requires_la32 : Error<
   "this builtin requires target: loongarch32">;
 
 // WebAssembly reference type and table diagnostics.
 def err_wasm_reference_pr : Error<
   "%select{pointer|reference}0 to WebAssembly reference type is not allowed">;
 def err_wasm_ca_reference : Error<
   "cannot %select{capture|take address of}0 WebAssembly reference">;
 } // end of sema component.

clang/lib/Analysis/UnsafeBufferUsage.cpp

DeclUseList getClaimedVarUseSites() const override {
     }
 
     return {};
   }
 };
 
 /// A pointer arithmetic expression of one of the forms:
 ///  \code
 ///  ptr + n | n + ptr | ptr - n | ptr += n | ptr -= n
 ///  \endcode
 class PointerArithmeticGadget : public WarningGadget {
   static constexpr const char *const PointerArithmeticTag = "ptrAdd";
   static constexpr const char *const PointerArithmeticPointerTag = "ptrAddPtr";
   const BinaryOperator *PA; // pointer arithmetic expression
   const Expr * Ptr;         // the pointer expression in `PA`
 
 public:
     PointerArithmeticGadget(const MatchFinder::MatchResult &Result)
       : WarningGadget(Kind::PointerArithmetic),
         PA(Result.Nodes.getNodeAs<BinaryOperator>(PointerArithmeticTag)),
         Ptr(Result.Nodes.getNodeAs<Expr>(PointerArithmeticPointerTag)) {}
 
   static bool classof(const Gadget *G) {
     return G->getKind() == Kind::PointerArithmetic;
   }
 
   static Matcher matcher() {
     auto HasIntegerType = anyOf(
           hasType(isInteger()), hasType(enumType()));
     auto PtrAtRight = allOf(hasOperatorName("+"),
                             hasRHS(expr(hasPointerType()).bind(PointerArithmeticPointerTag)),
                             hasLHS(HasIntegerType));
if (const auto *DRE =
       return {DRE};
     }
 
     return {};
   }
   // FIXME: pointer adding zero should be fine
   //FIXME: this gadge will need a fix-it
 };
+
+
+/// 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;
+
+public:
+    UnsafeBufferUsageAttrGadget(const MatchFinder::MatchResult &Result)
+      : WarningGadget(Kind::UnsafeBufferUsageAttr),
+        Op(Result.Nodes.getNodeAs<CallExpr>(OpTag)) {}
+
+  static bool classof(const Gadget *G) {
+    return G->getKind() == Kind::UnsafeBufferUsageAttr;
+  }
+
+  static Matcher matcher() {
+    return stmt(callExpr(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage))))
+                          .bind(OpTag));
+  }
+  const Stmt *getBaseStmt() const override { return Op; }
+
+  DeclUseList getClaimedVarUseSites() const override {
+    return {};
+  }
+};
 } // namespace
 
 namespace {
 // An auxiliary tracking facility for the fixit analysis. It helps connect
 // declarations to its and make sure we've covered all uses with our analysis
 // before we try to fix the declaration.
 class DeclUseTracker {
   using UseSetTy = SmallSet<const DeclRefExpr *, 16>;

clang/lib/Sema/AnalysisBasedWarnings.cpp

if (const auto *ASE = dyn_cast<ArraySubscriptExpr>(Operation)) {
       UnaryOperator::Opcode Op = UO->getOpcode();
       if (Op == UO_PreInc || Op == UO_PreDec || Op == UO_PostInc ||
           Op == UO_PostDec) {
         Loc = UO->getSubExpr()->getExprLoc();
         Range = UO->getSubExpr()->getSourceRange();
         MsgParam = 1;
       }
     } else {
+      if (const auto *FC = dyn_cast<CallExpr>(Operation)) {
+        MsgParam = 3;
+      }
       Loc = Operation->getBeginLoc();
       Range = Operation->getSourceRange();
     }
     if (IsRelatedToDecl)
       S.Diag(Loc, diag::note_unsafe_buffer_operation) << MsgParam << Range;
     else
       S.Diag(Loc, diag::warn_unsafe_buffer_operation) << MsgParam << Range;
   }

clang/lib/Sema/SemaDeclAttr.cpp

 template<typename Attr>
 static void handleHandleAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   StringRef Argument;
   if (!S.checkStringLiteralArgumentAttr(AL, 0, Argument))
     return;
   D->addAttr(Attr::Create(S.Context, Argument, AL));
 }
 
+template<typename Attr>
+static void handleUnsafeBufferUsage(Sema &S, Decl *D, const ParsedAttr &AL) {
+  D->addAttr(Attr::Create(S.Context, AL));
+}
+
 static void handleCFGuardAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   // The guard attribute takes a single identifier argument.
 
   if (!AL.isArgIdent(0)) {
     S.Diag(AL.getLoc(), diag::err_attribute_argument_type)
         << AL << AANT_ArgumentIdentifier;
     return;
   }
ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
 
   case ParsedAttr::AT_AcquireHandle:
     handleAcquireHandleAttr(S, D, AL);
     break;
 
   case ParsedAttr::AT_ReleaseHandle:
     handleHandleAttr<ReleaseHandleAttr>(S, D, AL);
     break;
 
+  case ParsedAttr::AT_UnsafeBufferUsage:
+    handleUnsafeBufferUsage<UnsafeBufferUsageAttr>(S, D, AL);
+    break;
+
   case ParsedAttr::AT_UseHandle:
     handleHandleAttr<UseHandleAttr>(S, D, AL);
     break;
 
   case ParsedAttr::AT_EnforceTCB:
     handleEnforceTCBAttr<EnforceTCBAttr, EnforceTCBLeafAttr>(S, D, AL);
     break;
 

clang/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: SwiftObjCMembers (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TargetClones (SubjectMatchRule_function)
 // CHECK-NEXT: TargetVersion (SubjectMatchRule_function)
 // 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: UseHandle (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: VecReturn (SubjectMatchRule_record)
 // CHECK-NEXT: VecTypeHint (SubjectMatchRule_function)
 // CHECK-NEXT: WarnUnused (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType, SubjectMatchRule_type_alias)
 // CHECK-NEXT: Weak (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: WeakRef (SubjectMatchRule_variable, SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyExportName (SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyImportModule (SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyImportName (SubjectMatchRule_function)
 // CHECK-NEXT: WorkGroupSizeHint (SubjectMatchRule_function)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: ZeroCallUsedRegs (SubjectMatchRule_function)
 // CHECK-NEXT: End of supported attributes.

clang/test/SemaCXX/attr-unsafe-buffer-usage.cpp

+// RUN: %clang_cc1  -fsyntax-only -verify %s
+
+// Function annotations.
+[[clang::unsafe_buffer_usage]]
+void f(int *buf, int size);
+void g(int *buffer [[clang::unsafe_buffer_usage("buffer")]], int size); // expected-warning {{'unsafe_buffer_usage' attribute only applies to functions}}

clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp

+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify %s
+
+[[clang::unsafe_buffer_usage]]
+void deprecatedFunction3();
+
+void deprecatedFunction4(int z);
+
+void someFunction();
+
+[[clang::unsafe_buffer_usage]]
+void overloading(int* x);
+
+void overloading(char c[]);
+
+void overloading(int* x, int size);
+
+[[clang::unsafe_buffer_usage]]
+void deprecatedFunction4(int z);
+
+void caller(int z, int* x, int size, char c[]) {
+    deprecatedFunction3(); // expected-warning{{function introduces unsafe buffer manipulation}}
+    deprecatedFunction4(z); // expected-warning{{function introduces unsafe buffer manipulation}}
+    someFunction();
+
+    overloading(x); // expected-warning{{function introduces unsafe buffer manipulation}}
+    overloading(x, size);
+    overloading(c);
+}
+
+[[clang::unsafe_buffer_usage]]
+void overloading(char c[]);
+
+// Test variadics
+[[clang::unsafe_buffer_usage]]
+void testVariadics(int *ptr, ...);
+
+template<typename T, typename... Args>
+[[clang::unsafe_buffer_usage]]
+T adder(T first, Args... args);
+
+template <typename T>
+void foo(T t) {}
+
+template<>
+[[clang::unsafe_buffer_usage]]
+void foo<int *>(int *t) {}
+
+void caller1(int *p, int *q) {
+    testVariadics(p, q);  // expected-warning{{function introduces unsafe buffer manipulation}}
+    adder(p, q);  // expected-warning{{function introduces unsafe buffer manipulation}}
+    
+    int x;
+    foo(x);
+    foo(&x);  // expected-warning{{function introduces unsafe buffer manipulation}}
+}
+
+// Test virtual functions
+class BaseClass {
+public:
+    [[clang::unsafe_buffer_usage]]
+    virtual void func() {}
+    
+    virtual void func1() {}
+};
+
+class DerivedClass : public BaseClass {
+public:
+    void func() {}
+    
+    [[clang::unsafe_buffer_usage]]
+    void func1() {}
+};
+
+void testInheritance() {
+    DerivedClass DC;
+    DC.func();
+    DC.func1();  // expected-warning{{function introduces unsafe buffer manipulation}}
+    
+    BaseClass *BC;
+    BC->func();  // expected-warning{{function introduces unsafe buffer manipulation}}
+    BC->func1();
+    
+    BC = &DC;
+    BC->func();  // expected-warning{{function introduces unsafe buffer manipulation}}
+    BC->func1();
+}