xazax.hun added inline comments.
================
Comment at: clang/docs/SafeBuffers.rst:31
+ convert large amounts of old code to conform to the warning;
+ - Attribute ``[[unsafe_buffer_usage]]`` lets you annotate custom functions as
+ unsafe, while providing a safe alternative that can often be suggested by
----------------
aaron.ballman wrote:
> Hmmmm that attribute name looks problematic (more below in that section).
With C++ modules gaining some momentum, I wonder if it would be
possible/beneficial to be able to annotate module import/export declarations
with this attribute to signal that the APIs and the content in a module was not
yet hardened.
================
Comment at: clang/docs/SafeBuffers.rst:36-37
+ hardened mode where C++ classes such as ``std::vector`` and ``std::span``,
+ together with their respective ``iterator`` classes, are protected
+ at runtime against buffer overflows. This changes buffer overflows from
+ extremely dangerous undefined behavior to predictable runtime crash.
----------------
Only buffer overflows, so comparing cases like:
```
auto it1 = v1.begin();
auto it2 = v2.begin();
if (it1 == it2) { ... }
```
are out of scope, right?
The main reason I'm asking, because I am not sure how safe iterators are
implemented and whether the same implementation would give opportunities for
other safety checks without significant additional cost.
================
Comment at: clang/docs/SafeBuffers.rst:40-41
+ - Finally, in order to avoid bugs in newly converted code, the
+ Clang static analyzer provides a checker to find misconstructed
+ ``std::span`` objects.
+
----------------
Isn't this overly specific? What about `string_view`s?
================
Comment at: clang/docs/SafeBuffers.rst:93
+ buffers, either pass the original container – move it or pass by reference
or
+ use a smart pointer – or pass a view into that container, such as
+ ``std::span``, which helps you deal with different container classes
----------------
I think in many cases it is more desirable to accept a view as a view can be
constructed from multiple different containers. An API that accepts a span can
work with `std::vector`s and `std::array`s or even some 3rd party containers.
This makes me think maybe mentioning views first and the other options later
would result in a better guideline.
================
Comment at: clang/docs/SafeBuffers.rst:115
+ (TODO: Will automatic fixits be able to suggest custom containers or views?)
+ (TODO: Explain how to implement such checks in a custom container?)
+
----------------
jkorous wrote:
> aaron.ballman wrote:
> > I think this would be a good idea, especially if you can use an example of
> > a more involved (but still common) container. Like, how does someone harden
> > a ring buffer?
> I believe that we should stick to hardening low-level primitives - all that a
> ring buffer implementation has to do is to store data in `std::array` from
> hardened libc++ and it's all set.
I think it would be great to link to some other resources like how to annotate
your container such that address sanitizer can help you catch OOB accesses.
================
Comment at: clang/docs/SafeBuffers.rst:124
+ - Array subscript expression on raw arrays or raw pointers,
+ - unless the index is a compile-time constant ``0``,
+ - Increment and decrement of a raw pointer with operators ``++`` and ``--``;
----------------
Isn't this too restrictive? How about arrays where both the index and the size
of the array is known at compile time?
Also, what about subscripts in `consteval` code where the compiler should
diagnose OOB accesses at compile time?
I believe this model can be made more ergonomic without losing any of the
guarantees.
================
Comment at: clang/docs/SafeBuffers.rst:287-288
+
+The automatic fixit associated with the warning would transform this array
+into an ``std::array``:
+
----------------
I anticipate that doing these fixits for multi-dimensional arrays will be
confusing. Is that in scope?
================
Comment at: clang/docs/SafeBuffers.rst:324-326
+and binary compatibility. In order to avoid that, the fix will provide
+a compatibility overload to preserve the old functionality. The updated code
+produced by the fixit will look like this:
----------------
While this might be desirable for some projects, I can imagine other projects
want to avoid generating the overload (just generate the fixes in one pass, and
apply all of them in a second to avoid a "fixed" header render another TU
invalid). But that would require fixits for the call sites. Do you plan to
support those batch migration scenarios or is that out of scope?
================
Comment at: clang/docs/SafeBuffers.rst:352
+
+ - The compatibility overload contains a ``__placeholder__`` which needs
+ to be populated manually. In this case ``size`` is a good candidate.
----------------
Just a small concern. While users are not allowed to use identifiers like this,
there is always a chance someone has a global variable of this name and the
code ends up compiling. So, if we want to be absolutely safe, we should either
have logic to pick a unique name, or not generate fixits in that case.
Although, this is probably rare enough that we could just hand-wave.
================
Comment at: clang/docs/SafeBuffers.rst:354-357
+ - Despite accepting a ``std::span`` which carries size information,
+ the fixed function still accepts ``size`` separately. It can be removed
+ manually, or it can be preserved, if ``size`` and ``buf.size()``
+ actually need to be different in your case.
----------------
I see this part a bit confusing. While it is true that we cannot be sure
whether the size is actually the size of the buffer, I wonder if we should at
least generate something to call it out the user has further action items. It
could be a `/*revise me*/` comment after the size, or documentation, both, or
something else.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136811/new/
https://reviews.llvm.org/D136811
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
