This is an archive of the discontinued LLVM Phabricator instance.

[GWP-ASan] Add public-facing documentation [6].
ClosedPublic

Authored by hctim on Jun 4 2019, 1:19 PM.

Details

Summary

Note: Do not submit this documentation until Scudo support is reviewed and submitted (should be #[5]).

See D60593 for further information.

This patch introduces the public-facing documentation for GWP-ASan, as well as updating the definition of one of the options, which wasn't properly merged. The document describes the design and features of GWP-ASan, as well as how to use GWP-ASan from both a user's standpoint, and development documentation for supporting allocators.

Diff Detail

Repository
rL LLVM

Event Timeline

hctim created this revision.Jun 4 2019, 1:19 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, Restricted Project, jfb and 2 others. · View Herald Transcript
hctim removed a reviewer: jfb.Jun 4 2019, 1:20 PM
kcc added a subscriber: kcc.Jul 9 2019, 2:46 PM

Maybe add an example?
something like my favorite

#include <iostream>
#include <string>
#include <string_view>

int main() {
  std::string s = "Hellooooooooooooooo ";
  std::string_view sv = s + "World\n";
  std::cout << sv;
}


for((i=0;i<10000; i++)); do GWP_ASAN_OPTIONS=Enabled=1:SampleRate=500  ./a.out > /dev/null; done

Segmentation fault
*** GWP-ASan detected a memory error ***
Use after free at 0x7f43300df000 (0 bytes into a 41-byte allocation at 0x7f43300df000) by thread 26228 here:
#0 ./a.out(_ZN8gwp_asan20GuardedPoolAllocator19reportErrorInternalEmNS0_5ErrorE+0x25a) [0x56454249ecea]
#1 ./a.out(+0x1bede) [0x56454249dede]
#2 /lib/x86_64-linux-gnu/libpthread.so.0(+0x123a0) [0x7ffbb526f3a0]
#3 /lib/x86_64-linux-gnu/libc.so.6(+0x15c27e) [0x7ffbb49d627e]
#4 /lib/x86_64-linux-gnu/libc.so.6(_IO_default_xsputn+0xa8) [0x7ffbb48f7058]
#5 /lib/x86_64-linux-gnu/libc.so.6(_IO_file_xsputn+0x151) [0x7ffbb48f52b1]
#6 /lib/x86_64-linux-gnu/libc.so.6(fwrite+0xd8) [0x7ffbb48ea1f8]
#7 /lib/x86_64-linux-gnu/libstdc++.so.6(_ZSt16__ostream_insertIcSt11char_traitsIcEERSt13basic_ostreamIT_T0_ES6_PKS3_l+0x124) [0x7ffbb590d3a4]
#8 ./a.out(_ZStlsIcSt11char_traitsIcEERSt13basic_ostreamIT_T0_ES6_St17basic_string_viewIS3_S4_E+0x45) [0x5645424b3bc5]
#9 ./a.out(main+0x9f) [0x5645424b393f]

Also, please provide some guidance about symbolizing the reports.

kcc added a comment.Jul 9 2019, 2:47 PM

With a build command line, of course

clang++ -g -std=c++17 string-view-use-after-free.cc  -fsanitize=scudo
hctim updated this revision to Diff 211223.Jul 22 2019, 4:04 PM
  • Added GWP-ASan symbolization example and script.
morehouse added inline comments.Jul 22 2019, 5:32 PM
compiler-rt/lib/gwp_asan/scripts/symbolize.sh
14 ↗(On Diff #211223)

Can reduce indentation by putting a continue here and removing else below.

llvm/docs/GwpAsan.rst
88 ↗(On Diff #211223)

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*.

94 ↗(On Diff #211223)

insert "detection"

99 ↗(On Diff #211223)

"helpful" is redundant.

102 ↗(On Diff #211223)

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".

107 ↗(On Diff #211223)

provides

108 ↗(On Diff #211223)

s/the guard page/its guarded slot

116 ↗(On Diff #211223)

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".

121 ↗(On Diff #211223)

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..."

133 ↗(On Diff #211223)

I'm confused. Is this per-allocator or per-process?

136 ↗(On Diff #211223)

Is this a CMake variable, a preprocessor macro, or something else?

196 ↗(On Diff #211223)

ephemeral + temporary seems redundant

196 ↗(On Diff #211223)

"inappropriately assigned" - Can we be more precise?

hctim marked 15 inline comments as done.Jul 22 2019, 6:27 PM
hctim added inline comments.
llvm/docs/GwpAsan.rst
121 ↗(On Diff #211223)

I think the "for other allocators..." part is covered above in the "Allocator Support" section, but otherwise SGTM+done.

133 ↗(On Diff #211223)

Hopefully the rewording adds some clarity.

hctim updated this revision to Diff 211239.Jul 22 2019, 6:27 PM
hctim marked 2 inline comments as done.
  • Updated from Matt's comments. Thanks Matt for taking a look on short notice :)
morehouse accepted this revision.Jul 23 2019, 9:25 AM
morehouse added inline comments.
llvm/docs/GwpAsan.rst
136 ↗(On Diff #211223)

This helps, but it's still not clear whether this is a cmake variable or a preprocessor variable.

Do I do this with:

cmake ... -DGWP_ASAN_DEFAULT_OPTIONS="..."

or

clang++ -fsanitize=scudo ... -DGWP_ASAN_DEFAULT_OPTIONS="..."
This revision is now accepted and ready to land.Jul 23 2019, 9:25 AM
vlad.tsyrklevich accepted this revision.Jul 23 2019, 4:53 PM
vlad.tsyrklevich added inline comments.
compiler-rt/lib/gwp_asan/scripts/symbolize.sh
3 ↗(On Diff #211239)

s/looks/look/

llvm/docs/GwpAsan.rst
50 ↗(On Diff #211239)

s/reasonble/reasonable/

hctim updated this revision to Diff 215512.Aug 15 2019, 5:08 PM
hctim marked 2 inline comments as done.
  • Addressed Matt's and Vlad's comments.
  • Updated memory overhead description in the docs.

Updated memory overhead description in the docs.

Assumes 8KiB for allocation/deallocation stack traces (256B compressed *
2 [alloc + dealloc] * 16 slots).
Assumes a full 4KiB of wasted memory (if we have 1-byte allocations) *
8 [half of the 16 possible slots].

Note that freed guarded allocations take no space, as the pages are
released back to the OS.

llvm/docs/GwpAsan.rst
136 ↗(On Diff #211223)

Added some clarifying comments, let me know if they LGTY.

hctim marked 2 inline comments as done.Aug 16 2019, 10:49 AM
morehouse added inline comments.Aug 19 2019, 9:06 AM
llvm/docs/GwpAsan.rst
134 ↗(On Diff #215512)

Shouldn't this be -DGWP_ASAN_DEFAULT_OPTIONS?

135 ↗(On Diff #215512)

What manual?

138 ↗(On Diff #215512)

Do we expect/want-to-support building outside of compiler-rt? If we encourage this, we may end up with many forks of GWP-ASan.

hctim marked 4 inline comments as done.Aug 19 2019, 4:14 PM
hctim added inline comments.
llvm/docs/GwpAsan.rst
135 ↗(On Diff #215512)

Was supposed to be the manual for the supporting allocator, but I've removed some words here to try and clarify.

138 ↗(On Diff #215512)

I think that supporting out-of-RT builds is necessary. We've got an Android repository slice up and going that will definitely be out-of-tree builds.

My goal is basically to have the core **/*.h and **/*.cpp. If you want to use GWP-ASan, simply pull in the slice of the repository and add it to your build.

Maybe I should copybara slice this out of CRT and keep a makefile-independent (read: only cpp and header files) version so that it can be mirrored everywhere... WDYT?

hctim updated this revision to Diff 216019.Aug 19 2019, 4:14 PM
hctim marked 2 inline comments as done.
  • Update from Matt's comments.
morehouse accepted this revision.Aug 19 2019, 4:53 PM
morehouse added inline comments.
llvm/docs/GwpAsan.rst
138 ↗(On Diff #215512)

Not sure if we have a need/demand for this. I'm inclined to not do this unless we have a good reason.

hctim marked an inline comment as done.Aug 20 2019, 1:09 PM
hctim added inline comments.
llvm/docs/GwpAsan.rst
138 ↗(On Diff #215512)

I forsee Chrome on Android using this configuration method to start-time disable platform GWP-ASan, as they'll use their own variant.

@vlad.tsyrklevich, do you have any thoughts?

llvm/docs/GwpAsan.rst
138 ↗(On Diff #215512)

We probably do want to disable GWP-ASan for Android Chrome, though it doesn't have to be this method specifically.

hctim marked an inline comment as done.Aug 20 2019, 4:48 PM
hctim added inline comments.
llvm/docs/GwpAsan.rst
138 ↗(On Diff #215512)

Ways to configure:

  • Compile time (for GWP-ASan) configuration options: Use -DGWP_ASAN_OPTIONS as described above.
  • Compile time (for application) configuration options: Use __gwp_asan_default_options here.
  • Program start time: Use GWP_ASAN_OPTIONS environment as per below.

We could possibly do detection of Chrome in the initialisation of platform GWP-ASan on Android - but I don't think this is generally suitable.

@morehouse are you okay with providing this config option? I think that #2 is probably the best way for applications to "always off" GWP-ASan, unless we have a better solution in mind.

morehouse added inline comments.Aug 20 2019, 6:02 PM
llvm/docs/GwpAsan.rst
138 ↗(On Diff #215512)

These three options are fine with me.

This revision was automatically updated to reflect the committed changes.

The docs don't build and rL369554 doesn't fully fix it

$ ninja docs-llvm-html
[1/1] Generating html Sphinx documentation
FAILED: docs/CMakeFiles/docs-llvm-html

Warning, treated as error:
llvm/docs/GwpAsan.rst:document isn't included in any toctree
ninja: build stopped: subcommand failed.

Submitted rL369556 to temporarily make docs build again. However that basically suppresses the warning; you likely want to put it in a toctree instead of making it an orphan doc, e.g. https://reviews.llvm.org/D66183#change-D4KqPHFormwF

The docs don't build and rL369554 doesn't fully fix it

$ ninja docs-llvm-html
[1/1] Generating html Sphinx documentation
FAILED: docs/CMakeFiles/docs-llvm-html

Warning, treated as error:
llvm/docs/GwpAsan.rst:document isn't included in any toctree
ninja: build stopped: subcommand failed.

Submitted rL369556 to temporarily make docs build again. However that basically suppresses the warning; you likely want to put it in a toctree instead of making it an orphan doc, e.g. https://reviews.llvm.org/D66183#change-D4KqPHFormwF

Solved in rL369560. Thanks!