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.

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
15

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

llvm/docs/GwpAsan.rst
89

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

95

insert "detection"

100

"helpful" is redundant.

103

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

108

provides

109

s/the guard page/its guarded slot

117

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

122

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

134

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

137

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

197

ephemeral + temporary seems redundant

197

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

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

134

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
137

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

s/looks/look/

llvm/docs/GwpAsan.rst
50

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
137

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
135

Shouldn't this be -DGWP_ASAN_DEFAULT_OPTIONS?

136

What manual?

139

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
136

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

139

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
139

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
139

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
139

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
139

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
139

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!