Page MenuHomePhabricator

[asan] Move instrumented null-terminated strings to a special section
ClosedPublic

Authored by kubamracek on Sep 28 2016, 8:11 AM.

Details

Summary

On Darwin, simple C null-terminated constant strings normally end up in the __TEXT,__cstring section of the resulting Mach-O binary. When instrumented with ASan, these strings are transformed in a way that they cannot be in __cstring (the linker unifies the content of this section and strips extra NUL bytes, which would break instrumentation), and are put into a generic __const section. This breaks some of the tools that we have: Some tools need to scan all C null-terminated strings in Mach-O binaries, and scanning all the contents of __const has a large performance penalty. This patch instead introduces a special section, __asan_cstring which will now hold the instrumented null-terminated strings.

Diff Detail

Repository
rL LLVM

Event Timeline

kubamracek updated this revision to Diff 72829.Sep 28 2016, 8:11 AM
kubamracek retitled this revision from to [asan] Move instrumented null-terminated strings to a special section.
kubamracek updated this object.
kubamracek added a project: Restricted Project.
kubamracek added a subscriber: llvm-commits.
m.ostapenko added inline comments.Sep 30 2016, 2:36 AM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
1512 ↗(On Diff #72829)

Oh, too many nested ifs here. Could you use && instead?

zaks.anna edited edge metadata.Sep 30 2016, 10:12 AM

LGTM

lib/Transforms/Instrumentation/AddressSanitizer.cpp
1516 ↗(On Diff #72829)

Should this have SectionKind::getReadOnly()?

zaks.anna accepted this revision.Sep 30 2016, 10:34 AM
zaks.anna edited edge metadata.
zaks.anna added inline comments.
lib/Transforms/Instrumentation/AddressSanitizer.cpp
1516 ↗(On Diff #72829)

Kuba pointed out that this API does not take the Kind. The section will be read-only because it is in the __TEXT segment.

This revision is now accepted and ready to land.Sep 30 2016, 10:34 AM
kcc edited edge metadata.Sep 30 2016, 11:11 AM

Is it possible to also add a runnable test to compiler-rt?
I expect to make some changes here soon: https://github.com/google/sanitizers/issues/260#issuecomment-250062528
And by doing that may break your test and your fix.

kubamracek updated this revision to Diff 74253.Oct 11 2016, 7:35 AM
kubamracek edited edge metadata.

Updating patch. Lowering the number of nested ifs. Adding a testcase for compiler-rt. Updating the odr-lto.cc testcase.

kubamracek requested a review of this revision.Oct 11 2016, 7:35 AM
kubamracek edited edge metadata.
filcab added inline comments.Oct 21 2016, 3:25 AM
projects/compiler-rt/test/asan/TestCases/Darwin/odr-lto.cc
6 ↗(On Diff #74253)

What happened here? There's no error any more? Do we still need to use -asan-use-private-alias, etc?

ping

projects/compiler-rt/test/asan/TestCases/Darwin/odr-lto.cc
6 ↗(On Diff #74253)

This tests checks that -asan-use-private-alias fixes a known bug when using LTO+ASan. Eventually, I wanted to turn -asan-use-private-alias on by default to get rid of this annoying bug causing many false positives. This patch fixes accidentally also this bug for a lot of cases (when strings are involved), but I believe there are other (less common) instances of the LTO+ASan bug that aren’t fixed. I would still like to keep the odr-lto.cc testcase, because it tests that -asan-use-private-alias doesn’t regress. If you want me to, I can try to create a new test that would trigger the LTO+ASan bug.

kubamracek updated this revision to Diff 75642.Oct 24 2016, 1:30 PM
kubamracek edited edge metadata.

Updating the ODR testcase. If the global is not a C string (e.g. it contains a NUL byte), it will not be moved to __asan_cstring section, but LTO still likes to merge such globals.

zaks.anna accepted this revision.Oct 28 2016, 6:14 PM
zaks.anna edited edge metadata.

Looks good. I do not think there are any outstanding issues here. Please, commit.

This revision is now accepted and ready to land.Oct 28 2016, 6:14 PM
This revision was automatically updated to reflect the committed changes.

Landed in LLVM r285619, compiler-rt r285620.