[PATCH] D62875: [GWP-ASan] Add public-facing documentation [6].

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/scripts/symbolize.sh:14
+  if [ -z "$should_symbolize" ]; then
+    echo "$line"
+  else
----------------
Can reduce indentation by putting a continue here and removing else below.


================
Comment at: llvm/docs/GwpAsan.rst:88
+pages. Each guarded slot is surrounded by two *guard* pages, which are mapped as
+inaccessible. We create a contiguous buffer of this ``guard_page | guarded_slot
+| guard_page`` pattern, which we call the *guarded allocation pool*.
----------------
I find this explanation confusing.  Maybe we don't need to mention that it's a contiguous buffer, since that detail isn't key to the approach.

Example wording:  The collection of all guarded slots make up the *guarded allocation pool*.


================
Comment at: llvm/docs/GwpAsan.rst:94
+
+We gain buffer-overflow and buffer-underflow through these guard pages. When a
+memory access overruns the allocated buffer, it will touch the inaccessible
----------------
insert "detection"


================
Comment at: llvm/docs/GwpAsan.rst:99
+about where (and by what thread) it was allocated and deallocated, we can
+provide helpful information that will help identify the root cause of the bug.
+
----------------
"helpful" is redundant.


================
Comment at: llvm/docs/GwpAsan.rst:102
+In order to increase our detection of overflows, we randomly align half of the
+allocations to the right hand side of the guarded slot.
+
----------------
Increase versus what?  Maybe this would be clearer if we say something like "allocations are randomly selected to be either left- or right-aligned to provide equal detection of both underflows and overflows".


================
Comment at: llvm/docs/GwpAsan.rst:107
+
+The guarded allocation pool also provide use-after-free detection. Whenever a
+sampled allocation is deallocated, we map the guard page as inaccessible. Any
----------------
provides


================
Comment at: llvm/docs/GwpAsan.rst:108
+The guarded allocation pool also provide use-after-free detection. Whenever a
+sampled allocation is deallocated, we map the guard page as inaccessible. Any
+memory accesses after deallocation will thus trigger the crash handler, and we
----------------
s/the guard page/its guarded slot


================
Comment at: llvm/docs/GwpAsan.rst:116
+use-after-frees. We ranomly choose an inaccessible slot to reuse in order to
+provide a chance of detecting long-lived use-after-frees.
+
----------------
Above two sentences could be combined and made more succinct.

e.g,, "To keep memory overhead fixed while still detecting bugs, deallocated slots are randomly reused to guard future allocations".


================
Comment at: llvm/docs/GwpAsan.rst:121
+
+Currently, the only allocator that supports GWP-ASan is the
+`Scudo Hardened Allocator <https://llvm.org/docs/ScudoHardenedAllocator.html>`_.
----------------
It's a small difference, but I'd prefer a more positive (think advertising) wording here.

e.g.,

"GWP-ASan already ships by default in Scudo, so building with `-fsanitize=scudo` is the quickest and easiest way to try out GWP-ASan.  For other allocators, using GWP-ASan is as easy as..."


================
Comment at: llvm/docs/GwpAsan.rst:133
+implementation of configuration that is used by Scudo. Several aspects of
+GWP-ASan can be configured on a per process basis through the following ways:
+
----------------
I'm confused.  Is this per-allocator or per-process?


================
Comment at: llvm/docs/GwpAsan.rst:136
+- At compile time, by defining ``GWP_ASAN_DEFAULT_OPTIONS`` to the options
+  string you want set by default;
+
----------------
Is this a CMake variable, a preprocessor macro, or something else?


================
Comment at: llvm/docs/GwpAsan.rst:196
+The below code has a use-after-free bug, where the ``string_view`` is
+inappropriately assigned to the ephemeral temporary result of the ``string+``
+operator. The use-after-free occurs when ``sv`` is dereferenced on line 8.
----------------
ephemeral + temporary seems redundant


================
Comment at: llvm/docs/GwpAsan.rst:196
+The below code has a use-after-free bug, where the ``string_view`` is
+inappropriately assigned to the ephemeral temporary result of the ``string+``
+operator. The use-after-free occurs when ``sv`` is dereferenced on line 8.
----------------
morehouse wrote:
> ephemeral + temporary seems redundant
"inappropriately assigned" - Can we be more precise?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62875/new/

https://reviews.llvm.org/D62875