[PATCH] D136811: WIP: RFC: NFC: C++ Buffer Hardening user documentation.

[Gábor Horváth via Phabricator via cfe-commits]

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