I think it makes sense on module export (the module author knows whether they are buffer-safe or not), but what do you have in mind for import where the user may not be in a position to make that assertion validly?

Wouldn't only_safe_buffers make sense on the import side to ensure that no unhardened code is being imported from a specific module?

Wouldn't only_safe_buffers make sense on the import side to ensure that no unhardened code is being imported from a specific module?

But the import side has no way to know that the module being imported has no unhardened code, only the module implementer knows that.

That said, I could imagine wanting to mark a module import as "will never be hardened" as a blanket way to say "don't complain about interfaces in this module I have no control over".

But now I'm questioning something from the docs:

Pragmas `unsafe_buffer_usage and only_safe_buffers` and allow you to
annotate sections of code as either already-hardened or not-to-be-hardened,

It now occurs to me there's two ways to read this.

unsafe_buffer_usage = will diagnose any unsafe buffer usage, it's already hardened.
only_safe_buffers == should only be used with known-safe buffers, it's not going to be hardened

or

unsafe_buffer_usage == this uses unsafe buffers, it's not to be hardened
only_safe_buffers == this uses only safe buffers, it's already been hardened

I originally read it the first way from the docs in the patch but the comment from @xazax.hun reads to me like he read it the second way.

Yes, this is out of scope for our diagnostics and code transformations. @ldionne Do you want to cover the safe iterator design part?

string_view is a bit harder to recommend automatically because whether a certain sequence of bytes constitutes a "string" is often a matter of opinion / user's point of view.

But why does that matter for string_view? It's not a null-terminated interface, so it explicitly encodes a pointer/length interface, same as span.

We can probably offer both variants in different note-fixits as soon as we teach the machine to recognize the different requirements for these strategies, and we can have string_view go first in the list?

I think that's reasonable enough; it doesn't have to do everything up front. So long as there's a plan (or at least an aspiration) to support multiple containers depending on context and preference, I'm happy enough. I had the (perhaps mistaken) impression this was heading towards *only* recommending span.

That's a fair point and if we don't find a good solution we should ignore arithmetic with this and a constant.

Fair enough -- it's worth documenting explicitly as an open question. FWIW, I agree that the construct is mostly used for accessing trailing objects as a buffer. The closest thing I've seen to a safe way to handle this is how LLVM does it with the TrailingObjects mixin.

Ah, C terminology is biting me here -- we use the term "constant" the same way C++ uses the term "literal", so when I read "constant 0" my brain interpreted that as literally just 0.

We discussed other options but generally prefer simple pragmas for their flexibility.
Our current thinking is that for readability purposes we won't allow nesting of pragmas and would have end of scope to be explicit.
While this might be more verbose it would be dead simple to reason about.

We discussed other options but generally prefer simple pragmas for their flexibility.

A single pragma with lexical scoping behaves like RAII, which is often a replacement for more complex acquire/release paired operations and is a pretty well-understood pattern in C++.

Our current thinking is that for readability purposes we won't allow nesting of pragmas and would have end of scope to be explicit.
While this might be more verbose it would be dead simple to reason about.

True, it is easy to reason about. But not allowing nesting for readability purposes removes some utility, especially for folks in C where macros are much more common. e.g.,

#define ONLY_SAFE_BUFFERS _Pragma("only_safe_buffers")

#define AWESOME_BUFFER_THING(ptr, size) ({ ONLY_SAFE_BUFFERS; (interesting work goes here) })

void func(int *ptr, size_t size) {
  ONLY_SAFE_BUFFERS;
  ... interesting code lives here ...
  AWESOME_BUFFER_THING(ptr, size); // Oops, this is nested.
 .. more interesting code lives here ...
}

(You can hit the same situation with paired pragmas.) Because C doesn't have constexpr or consteval support for functions or templates, function-like macros are a pretty common interface folks use in practice and macro expansion makes it somewhat hard to avoid potential surprising conflicts.

It is desirable to minimize the extent of code labeled as unsafe - if a single line of code is unsafe then it'd be suboptimal do label a whole scope as unsafe.
Since scopes affect objects lifetime in C++ we won't be able to generally recommend adding a scope around the unsafe line either as that might change behavior of the code.
In this sense associating the pragmas' semantics with scopes reduces their flexibility. Is there any benefit of doing that which would outweigh this?

Separately from that - in theory the users could just end the scope before the macro:

ONLY_SAFE_BUFFERS_BEGIN;
... interesting code lives here ...
ONLY_SAFE_BUFFERS_END;
AWESOME_BUFFER_THING(ptr, size); // Oops, this is nested.

But point taken - there might be options for better ergonomics.

In this sense associating the pragmas' semantics with scopes reduces their flexibility. Is there any benefit of doing that which would outweigh this?

FWIW, I wasn't suggesting we support *only* the scoped form, but that we *also* supported a scoped form. The benefit really boils down to more complex functions (generally ones in need of refactoring anyway) where you already have an appropriate scope handy and pairing the pragmas would be clutter. e.g.,

if (ptr) {
  #pragma clang unsafe_buffer_usage
  ptr += 12;
}

vs

if (ptr) {
  #pragma clang unsafe_buffer_usage begin
  ptr += 12;
  #pragma clang unsafe_buffer_usage end
}

I don't see a need to write two pragmas in these kinds of cases where the compound scope is already a clear delimiter. WDYT?

We'd love to arrive at a design that is very difficult to use incorrectly.

You are absolutely right that our preference for not using the diagnostic pragmas stems from the "untyped" pop operation.

Assuming this code is a starting point:

#pragma clang diagnostic push warning "-Wfoo"
// code
#pragma clang diagnostic pop //end of -Wfoo
// more code

if the user adds pragmas like this:

#pragma clang diagnostic push warning "-Wfoo"
#pragma clang diagnostic push warning "-Wsafe-buffers-usage"
// code
#pragma clang diagnostic pop //end of -Wfoo
// more code
#pragma clang diagnostic pop //end of -Wsafe-buffers-usage

they actually get this behavior:

#pragma clang diagnostic push warning "-Wfoo"
#pragma clang diagnostic push warning "-Wsafe-buffers-usage"
// code
#pragma clang diagnostic pop //end of -Wsafe-buffers-usage
// more code
#pragma clang diagnostic pop //end of -Wfoo

We should have the patch for pragmas ready to be put for review here in not too distant future. Maybe it'd be easier to discuss there?

We should have the patch for pragmas ready to be put for review here in not too distant future. Maybe it'd be easier to discuss there?

Certainly! I was mostly trying to reword what you had to keep the same intent but not make it sound like the basis for the pragmas is "we think these other pragmas, which do work, were not good enough" because that's not very technically compelling.

One thing I think is worth pointing out about these docs is that the first example effectively says "do add the attribute because the size passed in to the function could be wrong" and the second example says "don't add the attribute on the assumption that the container has the correct size information". The advice feels a bit conflicting to me because in one case we're assuming callers pass in incorrect values and in the other case we're assuming callers pass in correct values and the only distinction between them is a "container was used". But a significant portion of our users don't use libc++ (they use libstdc++ or MS STL for example).

Our model depends on Standard Library used implementing the bounds checks.
With that the "container was used" distinction is absolutely crucial - that is what adds the bounds checks and already mitigates certain classes of bugs (even if the span passed in still has the same wrong size).

Let's assume we're starting with this buggy code:

void caller() {
  int buffer[10];
  callee(buffer, 20);
}

void callee(int* ptr, unsigned size) {
  ptr[size-1] = 42;
  ptr[size] = 42;
  ptr[100] = 42;
}

and transform it to (this is just a part of what we actually plan but hopefully illustrates the point):

void caller() {
  int buffer[10];
  callee(std::span<int>(buffer, 20), 20);
}

void callee(std::span<int> ptr, unsigned size) {
  ptr[size-1] = 42; // still unmitigated
  ptr[size] = 42; // mitigated at runtime by std::span::operator[]() in hardened libc++
  ptr[100] = 42; // mitigated at runtime by std::span::operator[]() in hardened libc++
}

Our model depends on Standard Library used implementing the bounds checks.

So will the model consider things like libstdc++ that don't implement the bounds checks? Or MSVC STL with Safe Library Support (https://learn.microsoft.com/en-us/cpp/standard-library/safe-libraries-cpp-standard-library?view=msvc-170) which seems to have some of the hardening you're talking about, but not all of it?

void caller() {
  int buffer[10];
  callee(std::span<int>(buffer, 20), 20);
}

void callee(std::span<int> ptr, unsigned size) {
  ptr[size-1] = 42; // still unmitigated
  ptr[size] = 42; // mitigated at runtime by std::span::operator[]() in hardened libc++
  ptr[100] = 42; // mitigated at runtime by std::span::operator[]() in hardened libc++
}

Understood; my point was that the comments in callee() are incorrect in other STL implementations. The model seems to presume all STLs are hardened (or will be in the relatively near future) but I don't think that's a valid presumption (for example, STLs exist which are focused solely on performance, like EASTL). So that makes me question whether the model *should* presume that container state is valid when it comes from an untrusted source.

The warnings about unsafe buffer usage would be valid no matter what Standard Library implementation is used and I can imagine the FixIts to exist in different flavors.
We don't plan to target any other Standard Library implementation but the machinery we'll bring up should allow that to be implemented in the future.

Does that make sense?

It does, but the situation I'm thinking of is where the user is migrating their unsafe code to this new programming mode and aren't able to use libc++ to get the extra hardening checks. Consider this contrived example:

extern "C" void func(int *array, size_t count) {
  // Do buffer things with array and count
}

The user can't (easily) modify this code to accept a span directly because that could potentially break ABI. So they wrap the data in a span:

extern "C" void func(int *array, size_t count) {
  std::span<int> sp(array, count);
  // Do buffer things with sp
}

In both cases, the user is taking it on faith that the data passed into the function is correct. But you want the user to believe the second case is more trustworthy because it uses span -- but more trustworthy based on what? There's no hardened checks in the STL they're using and the potentially-wrong information is the same flavor of potentially-wrong in either case.

My fear is that we push users to make changes like that by telling them it's safer when it's actually functionally equivalent instead, but when they migrates their code they accidentally *introduce* a new bug where there wasn't one before.

I'm used to thinking about this sort of stuff as a kind of taint analysis. We need to know where that data comes from in order to know whether we can trust it or not. Did the "count" come from user input on the command line... or did it come from a call to .size() on some other container? These have different levels of trust (to me).

Btw, this brings up a different question about fix-it hints. Are you planning to add fix-it hints to interface boundaries? If so, are you planning to pay attention to things like extern "C" or the function linkage to try to reduce the amount of ABI breaks suggested?

In both cases, the user is taking it on faith that the data passed into the function is correct. But you want the user to believe the second case is more trustworthy because it uses span

No, that's definitely not our intention. That's why I'm explicitly specifying that this assumption only applies to containers constructed *outside* the function. In your example both functions should wear the attribute. And there's no way to produce a safe alternative (which wouldn't need [[unsafe_buffer_usage]]) under extern "C" linkage.

My point is that the assumption seems invalid to me -- it's a matter of garbage-in, garbage-out, but you're saying "this potentially garbage-in situation is assumed to not happen" and then building analysis checks around that assumption. I realize we have to make some heuristic decisions for the analysis, but this one seems dangerous for something attempting to improve buffer safety because nothing validates that assumption. Or do you have plans for the future to add an analysis to cover the situation of shoving garbage into the span?

To be clear, this is the situation I'm concerned about:

// SomeLibraryTU.cpp
void func(int *buffer, size_t count) { // Old interface for compatibility
  func(std::span<int>(buffer, count));
}

void func(std::span<int> sp) {  // New, safe interface
  if (!sp.empty()) {
    int *data = sp.data();
    // Do stuff on data (other than pointer arithmetic, only looking at first element)
  }
}

// SomeApplicationTU.cpp
int main() {
  func(0, 1); // Oops, wrong count!
}

My complaint is that the library code is perfectly safe but the application code is not, and it sounds like this situation is purposefully not considered as part of this analysis... but it seems like this situation is the common case we'd *want* to catch.

We do have plans for a static analyzer checker that warns about shoving garbage into the span! - that's the last bullet point in the intro.

Ah, perfect!

We also think it's much harder to shove garbage into span in the first place. The size parameter of the span is extremely unambiguous, there's nothing else you can think of putting there.

Hehe, oh to be this optimistic still. :-D The scenario I expect to see garbage in span is from data that can be user-controlled (the exact stuff you're worried about, to be honest!): pulling in size data from the network or a file format, etc. Something where it's not at all accidental that it's garbage going into the span, but is actually malicious input from an attacker.

To be clear, this is the situation I'm concerned about:

Hmm I'm not sure I understand this example, how does the lack of buffer operations in the new function kicks in here?

It doesn't have to lack buffer operations, I was just trying to make the simplest example I could and I probably obscured my point. My point is mostly that a library interface that takes buffer + size and then wraps it into a span to pass to a span interface is every bit as dangerous as not having the span interface at all. If the data going into the span is tainted, it's game over. So I worry about *assuming* the data in the span is valid and basing diagnostic decisions around that. However, because you're going to also have checkers specifically trying to catch the case where invalid data CAN be shoved into the span (like a taint analysis, I hope?), that basically resolves my concerns.

I was trying to say that `[[deprecated]] is probably appropriate if the vendor of the function expects *all* users of the function to move away from it.

Ahhhh that makes a lot more sense to me, thanks!

For better or worse the deprecated attribute is "overloaded" and this refers to the two-parameter version.

https://clang.llvm.org/docs/AttributeReference.html#deprecated

"When spelled as attribute((deprecated)), the deprecated attribute can have two optional string arguments. The first one is the message to display when emitting the warning; the second one enables the compiler to provide a Fix-It to replace the deprecated name with a new name."

Since it seems confusing even to you - maybe we should take this as a signal that our users would get just as confused and remove the reference to deprecated?
WDYT?

Ohhhh! I see now! What confused me is that the docs spell it as [[deprecated]] which does *not* have an overload that takes two arguments. If we keep this part of the docs, you should probably change it to __attribute__((deprecated)) and link to the attribute docs for it.

Since it seems confusing even to you - maybe we should take this as a signal that our users would get just as confused and remove the reference to deprecated?

Maybe we should, but because I'm still not quite clear on how this relates to the deprecated attribute yet, maybe you could reply with some code examples of deprecated and unsafe_buffer_usage and what diagnostics you get? If that helps clarify, maybe I can help reword the docs to improve clarity.

Do you mean that the dimensions get reversed if we replace int arr_3d[3][4][5] with array<array<array<int, 5>, 4>, 3> arr_3d_r?

Yeah, I can imagine some people wanting bounds safety but having strong opinions about the reversed dimensions in their code. Moreover, if there are multi-dimensional array typedefs in the nesting, that can make things more complicated.

We currently don't have a machinery that would allow us to analyze the whole project at once.

Two passes are generally not enough - that number is something like number of unique functions in the deepest call-stack.

Part of our fixits is attributing functions as unsafe and application of such fixits can create new warnings in callers.

Let's take an example:

foo(int* ptr) {
  ptr[5] = 5;
}

bar(int* ptr) {
  foo(ptr);
}

baz(int* ptr) {
  bar(ptr);
}

Initially it is not obvious that bar or baz might get transformed.

In the first iteration a fixit for foo is emitted and when applied we get:

foo(span<int> ptr) {
  ptr[5] = 5;
}

[[clang::unsafe_buffer_usage]] foo(int* ptr);

bar(int* ptr) {
  foo(ptr);
}

baz(int* ptr) {
  bar(ptr);
}

Only now we learn that bar (which can live in a diferent TU) should get transformed as well because it calls to unsafe function foo. But our analysis still won't see that baz will eventually have to be transformed too and it'll take another iteration.

I see, this explains it all, thanks! I'd love to have this motivating example in the design document why a 2-pass fix-it-all model does not work.

Agreed; we should not have a fixit that suggests use of a reserved identifier.

Our thinking is to actually use syntactically incorrect placeholder to make sure that users can't accidentally miss it.
This is related to:
https://reviews.llvm.org/D136811#inline-1324542

Our thinking is to actually use syntactically incorrect placeholder to make sure that users can't accidentally miss it.

So long as that happens on a note, that would be fine depending on how you name the identifier. e.g., remember that we have a feature flag to support dollar signs in identifiers, we need to protect against macros, that sort of thing.

We have accepted that our analysis will have some limits. In situations where we have a reasonable suspicion that a certain parameter has the semantics of size we can somehow communicate that to the user. But it is very likely that in some cases the analysis won't be able to tell even that.

Not being able to cover all the cases sounds fine to me, and I'm glad you plan to communicate potential size params to the user when there is reasonable certainty.

That's an extremely interesting question! Let me add our current thinking as a context for the discussion.

Our analysis will have its limits and will not be able to provide 100% complete and guaranteed correct code transformation in every case.

At the same time it sounds suboptimal both from our and our users perspective to not emit anything for such situations.
Imagine we have a code transformation that affects 20 lines of code and all we need is for the user to address a single function call argument.
The thought of bailing, producing just a warning and having user to manually change all 20 lines of code sounds very unsatisfactory to me.
That's why we are thinking to use placeholders.

The next question is - can we not break the code when using placeholders?
By definition whenever we'd emit a placeholder our analysis can't provide a correct solution. That means that whatever way we'd make the code to compile can introduce a logic bug in the program. That is simply unacceptable given the overall goal of our effort.
That pretty much leaves us with only using placeholders that would break the build.

WDYT?

I wasn't aware of the precedent related to fixits attached to notes not having to lead to compilable code and we should consider that path.

I wasn't aware of the precedent related to fixits attached to notes not having to lead to compilable code and we should consider that path.

We document the expectations here: https://clang.llvm.org/docs/InternalsManual.html#fix-it-hints and the interesting bit is:

"Fix-it hints on errors and warnings need to obey these rules:

• Since they are automatically applied if -Xclang -fixit is passed to the driver, they should only be used when it’s very likely they match the user’s intent.
• Clang must recover from errors as if the fix-it had been applied.
• Fix-it hints on a warning must not change the meaning of the code. However, a hint may clarify the meaning as intentional, for example by adding parentheses when the precedence of operators isn’t obvious.

If a fix-it can’t obey these rules, put the fix-it on a note. Fix-its on notes are not applied automatically."

I think using placeholders on warnings rather than notes will be an issue for both the first and second bullets.

Yes, we should look into attaching FixIts that contain placeholders to notes instead.