[[clang::unsafe_buffer_usage]] in libc++

[danakj]

There are many methods in libc++ which can cause out-of-bounds issues when given incorrect inputs, such as any method that takes one or more iterators as its inputs, or that takes a pointer input.

Will libc++ be annotating such methods with [[clang::unsafe_buffer_usage]]? Is the project open to adding such annotations on methods that receive iterators (instead of ranges)?

Concrete example: std::ranges::subrange::subrange(iterator, sentinel) if given invalid inputs will create a subrange that goes out of bounds. This is similar to std::span(first, size), which is currently hard-coded in the compiler as-if it were marked with [[clang::unsafe_buffer_usage]]. Other examples: std::span::span(first, last), std::vector::insert(pos, first, last), std::memcpy(dest, src, count).

Putting such annotations in libc++ will help callers avoid unsafe APIs and transition to safer ones.

We would need need all [[clang::unsafe_buffer_usage]] to live behind a config define to allow enabling it separately from rolling libc++ though.

Thoughts? Is this something we could do now? At some future time? Explicitly undesirable?

cc: @haoNoQ @ziqingluo-90 @jkorous-apple @ldionne

[pkasting]

I'd like to see this happen so that:

• We can simplify compiler-side implementation, hopefully making it more robust
• There's a clearer and more achievable path to getting the warning in other compilers
• Library-side usage of the attributes is battle-tested at scale, so they're known-suitable for usage on authors' own types because pain points are fixed
• libc++ sources serve as an example of how and where to use (much harder to understand usage by reading compiler source)
• If anyone ever wants to propose something like this to WG21, it's clear in practice what it would imply

[ldionne]

CC @var-const

I could see this being useful, however we'd have to agree on what needs to be marked. For example, we support an ABI configuration under which iterators are bounded and will trap on OOB access. Would we consider an iterator-based API to be "unsafe" under that configuration?

[philnik777]

IMO this would need an RFC, since it's a major change within libc++. That RFC should mention

• what the benefit of doing that is
• what the changes inside libc++ would look like
• how this interacts with the hardening effort

Also some things that will probably come up:

• How do we handle cases that are sometimes perfectly safe, like output iterators (keyword: back_insert_iterator)
• What about interfaces where there is no safe alternative - there are probably some

[haoNoQ]

A few things here:

• [[clang::unsafe_buffer_usage]] in the standard library is arguably an option (unless @ldionne objects), but it's also likely that with standard functions we can do better by simply hardcoding them in the compiler. This includes things like false positive suppression for known-safe idioms, as well as better diagnostic messages.
• C functions such as std::memcpy() are now hardcoded in the compiler as unsafe: [-Wunsafe-buffer-usage] Warning Libc functions #101583 - and it showcases a few benefits of hardcoding those in the compiler.
• Iterator and range based functions are as unsafe as their underlying iterator implementations:
  • We're currently focusing on the situation where the ABI-defining option _LIBCPP_ABI_BOUNDED_ITERATORS is enabled on your platform, which makes these functions safe as long as bounded iterators are used.
  • If raw pointers are passed to these functions, we should arguably warn unconditionally; but even then, the compiler might do a better job than the attribute.
  • With custom containers iterators, we should probably give up and emit no warnings and that's actually perfectly fine. The model simply assumes that eventually you'll get to address the warning inside the implementation of your container, which will make you either harden the iterators, or annotate your .begin()/.end() as unsafe with the attribute.
  • This leaves us with standard iterators in the situation where _LIBCPP_ABI_BOUNDED_ITERATORS cannot be enabled. IIUC we can't promise a solution in this case but we are exploring a few ideas (@ldionne can probably elaborate a bit better).
• Finally, there's a few other standard functions that also need to be considered unsafe. In particular, std::unique_ptr<T[]>::operator[] should probably be considered unsafe despite the clever hardening trick implemented by [libc++] Add an ABI setting to harden unique_ptr<T[]>::operator[] #91798; same with other smart pointers. We'll probably cover those in a moment.

[danakj]

Is iterator hardening always sufficient to prevent UB?

void f(std::random_access_iterator auto begin, std::random_access_iterator auto end) {
  if (std::distance(begin, end) < 10) { ... }
}

In this example if the iterators come from different containers (point to different allocations), we get UB, and the iterator checks don't help, right?

llvm-project/libcxx/include/__iterator/bounded_iter.h

Lines 191 to 194 in b3d2d50

	_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 friend difference_type
	operator-(__bounded_iter const& __x, __bounded_iter const& __y) _NOEXCEPT {
	return __x.__current_ - __y.__current_;
	}

[haoNoQ]

In this example if the iterators come from different containers (point to different allocations), we get UB, and the iterator checks don't help, right?

They actually help quite a bit because an iterator from a different container will naturally be out-of-bounds for the other container. It's just a matter of remembering to check that. The only way this doesn't work is when it's two overlapping spans of the same buffer. But even then, it's more of a technical UB than a practical UB, because the underlying operation is kind of valid anyway given that it's the same buffer(?)

In any case, looks like there's a TODO about that specifically: https://libcxx.llvm.org/Hardening.html#assertion-categories

valid-input-range – checks that ranges (whether expressed as an iterator pair, an iterator and a sentinel, an iterator and a count, or a std::range) given as input to library functions are valid: - the sentinel is reachable from the begin iterator; - TODO(hardening): both iterators refer to the same container.

[davidben]

Keep in mind that there are still some gaps in bounded iterators. They're fixable gaps, but someone needs to go through and fix them. #78771 comes to mind. But they're also simply bugs in to fix in bounded iterators, not a problem in the approach.

To that end, I think the issue @danakj points out is simply a bug in bounded iterators, not a reason to reject the idea. operator- can just check that the two iterators are compatible by checking for a match in __begin_ and __end_. In fact, I'd already flagged that is a prerequisite to fixing #78771. See step 5 in #78771 (comment)

[davidben]

TBH I can probably just cite all the bugs I've opened, as almost of them have been motivated in some form by bounded iterators. 😄 (But I don't have all that much time to hack on stuff, so I file more bugs than I have time to send patches for.)
https://github.com/llvm/llvm-project/issues/created_by/davidben

[davidben]

@danakj pointed out a case where we're kinda stuck with unbounded iterators. std::begin(T(&)[N]) is defined to return `T*, not an iterator. So even if we manage to get 100% bounded iterator coverage across an entire project's containers, there would need to be some answer for what happens when you pass pointers into an STL function.

(Or perhaps we get -fbounds-safety working in C++ so that T* is also bounded. 😄 )

[danakj]

If raw pointers are passed to these functions, we should arguably warn unconditionally; but even then, the compiler might do a better job than the attribute.

With the standard library, it's possible for the compiler to do this in a nice way, I agree. What about for other library authors?

From what I can see with current APIs, this would require every method that takes an iterator to have an overload taking a pointer, so that the latter can be annotated [[clang::unsafe_buffer_usage]].

For example:

template <std::input_iterator I>
void insert(I begin, I end);

Would need to become something like:

template <std::input_iterator I>
void insert(I begin, I end);

[[clang::unsafe_buffer_usage]]
void insert(value_type* begin, value_type* end);

Having to double the number of functions in APIs like this to get the unsafe-buffers warning only when working with a pointer is really unfortunate. And it seems like the compiler could do better here for other libraries beyond the stdlib.

One possibility, an optional boolean on the attribute.

template <std::input_iterator I>
[[clang::unsafe_buffer_usage(std::is_pointer_v<I>)]]
void insert(I begin, I end);

How else could we get warnings for pointers in these cases, in a consistent way between std and elsewhere?

[haoNoQ]

Would need to become something like:

template <std::input_iterator I>
void insert(I begin, I end);

[[clang::unsafe_buffer_usage]]
void insert(value_type* begin, value_type* end);

It'd actually need to become something like:

template <std::input_iterator I>
void insert(I begin, I end) {
    // body
}

void insert(value_type* begin, value_type* end) {
#pragma clang unsafe_buffer_usage begin // or address some of those warnings
    // body
#pragma clang unsafe_buffer_usage end
}

because the attribute doesn't suppress the warning inside the function. It only notifies the caller that this function should not be used at all. Addressing the warnings inside the function may still make your code strictly safer, even if it doesn't eliminate the unsafety entirely.

Side note, -Wunsafe-buffer-usage currently has a tricky false negative when it comes to templates in headers on project/library boundaries. When all we see is a piece of uninstantiated template code, we often can't notice any unsafe operations in it. A lot of the time we can't even notice that there are any pointers in it. We have no idea what types may go in. In such situations we can't emit a warning to the author of the library that their template is unsafe. And without that, the author is less likely to add [[clang::unsafe_buffer_usage]] to their template voluntarily.

On the other hand, if the user of the library includes the library through -isystem (which is the intended way to include the code you don't control), we aren't allowed to emit warnings inside the template either. Because the compiler is simply not allowed to emit warnings against -isystem headers - given that users can't fix them anyway. So the users will not be able to encourage the author to add [[clang::unsafe_buffer_usage]] to their template, given that they don't see any warnings either.

So in order to display any warning at all in this scenario, we'll need to do something very unusual: emit the warning in user code and unwrap the instantiation stack in reverse in order to put a note where the actual unsafe operation is. (It's probably not enough to simply put a note at the unsafe operation. Without the instantiation stack it may become incomprehensible.)

Once we implement that, it may be true that the header doesn't need #pragma clang unsafe_buffer_usage anymore. The pragma will go to the call site instead (if at all). They may not need the attribute either. At least not until they instantiate the template this way in their own library (so we never had that problem to begin with).

But the point still stands: if the function is unsafe, there are often a few ways to make it slightly safer, even if it continues to be unsafe. In this sense, a separate instantiation is often necessary.

Not always though. So:

One possibility, an optional boolean on the attribute.

template <std::input_iterator I>
[[clang::unsafe_buffer_usage(std::is_pointer_v<I>)]]
void insert(I begin, I end);

I still really like this. It may be exactly what we need in some cases. We could make the pragma conditional too?

#pragma clang unsafe_buffer_usage begin(std::is_pointer_v<I>)
template <std::input_iterator I>
[[clang::unsafe_buffer_usage(std::is_pointer_v<I>)]]
void insert(I begin, I end) {
    // body
}
#pragma clang unsafe_buffer_usage end

[davidben]

I like the boolean on the attribute too. Should it be a type trait or something, so we can distinguish __wrap_iter (unsafe) from __bounded_iter (safe)?

I mean really what we want is to say something like "this is safe as long as I::operator*, I::operator++, etc., are safe, but I suspect that would be more challenging than a trait.

[philnik777]

Closing, since the only actionable task here is to create an RFC, which isn't the task of libc++ contributors (necessarily).