Page MenuHomePhabricator

[asan] Use private aliases for global variables (LLVM part).
ClosedPublic

Authored by m.ostapenko on Dec 18 2015, 8:11 AM.

Details

Summary

As discussed in https://github.com/google/sanitizers/issues/398, with current implementation of poisoning globals we can have some CHECK failures or false positives in case of mixing instrumented and non-instrumented code due to ASan poisons innocent globals from non-sanitized binary/library. We can use private aliases to avoid such errors. In addition, to preserve ODR violation detection, we introduce new _asan_genXXX symbol for each instrumented global that indicates if this global was already registered. To detect ODR violation in runtime, we should only check the value of indicator and report an error if it's not equal to zero.

Diff Detail

Repository
rL LLVM

Event Timeline

m.ostapenko retitled this revision from to [asan] Use private aliases for global variables (LLVM part)..
m.ostapenko updated this object.
m.ostapenko added reviewers: kcc, eugenis, samsonov.
m.ostapenko set the repository for this revision to rL LLVM.
m.ostapenko added subscribers: ygribov, llvm-commits.
kcc edited edge metadata.Dec 18 2015, 11:10 AM
kcc added subscribers: samsonov, eugenis.

The change in general makes sense, but is potentially very intrusive, and hard to test on the small set of unit tests.
We'll need to test it on a larger code base. Did you test it on something large?
I afraid Chromium tests are not an adequate test target for this functionality, but it would be a good start.

Alexey, WDYT?
One way I can see is to put this functionality under a combination of a compile-time and a run-time flags.
That'll bloat the code, but I can't see a better way to test it (other than applying the patch locally).
__asan_global will need to be changed unconditionally, but that's ok.

Also, please don't mention the bugs in the source -- it's enough to mention them in the commit message.

In D15642#313943, @kcc wrote:

The change in general makes sense, but is potentially very intrusive, and hard to test on the small set of unit tests.
We'll need to test it on a larger code base. Did you test it on something large?
I afraid Chromium tests are not an adequate test target for this functionality, but it would be a good start.

Yeah, you are right, the change seems to be intrusive. I ran Chromium tests with the patch and hit on -fvisibililty=hidden stuff (we should not export _asan_genXXX symbols for hidden globals), so we should be very careful here. I'm updating the diff.

As for something really big, I thought Android would be a good candidate (AFAIK, Eugeni hit on ODR fiasco there), but I'm not sure I can build and run it without help (at least I don't have a target device). It would be nice if you could help me here somehow.

m.ostapenko edited edge metadata.
m.ostapenko removed rL LLVM as the repository for this revision.

Updating the diff. Don't export _asan_genXXX symbols in -fvisibility=hidden case.

Changing symbol linkage basing on visibility attribute is not intuitive and error prone. Instead, we should just copy attributes from NewGlobal to new indicator symbol. However, if we use asan_* pattern, we end up with indicator to be always exported because of --dynamic-list=/usr/local/bin/../lib/clang/3.8.0/lib/linux/libclang_rt.asan-x86_64.a.syms link option and this file contains asan_*. So, just add new __odr_gen* pattern and use it for ODR indicators.

m.ostapenko set the repository for this revision to rL LLVM.

Back here. Hide using private aliases for globals behind -asan-use-private-alias compile option. Generate ODR indicator symbol if use aliases and pass it's address to Global structure, pass 0 otherwise. Tested on Chromium (build chrome itself, build and run unit tests).

ygribov added inline comments.Jan 19 2016, 12:51 PM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
1399 ↗(On Diff #45279)

Would it make sense to disable this on platforms that don't have aliases (Windows and maybe OSX)?

1408 ↗(On Diff #45279)

Perhaps just reuse ODRIndicator here?

m.ostapenko added inline comments.Jan 20 2016, 6:16 AM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
1399 ↗(On Diff #45279)

Yes, you are right, perhaps something like this:

bool CanUsePrivateAliases = TargetTriple.isOSBinFormatELF();
if (CanUsePrivateAliases && ClUsePrivateAliasForGlobals) {
  ...

?

1408 ↗(On Diff #45279)

I use ODRIndicatorSym->copyAttributesFrom() that would be inaccessible if use ODRIndicator (that is Constant *). Perhaps we can change it's type to support copyAttributesFrom() interface, but I failed to find reasonable default value for it. If use Constant *, we can just use zero.

Updating according to Yura's feedback.

ygribov added inline comments.Jan 20 2016, 12:49 PM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
1399 ↗(On Diff #45405)

Yeah.

1402 ↗(On Diff #45405)

May also mention support for RTLD_DEEPBIND.

1408 ↗(On Diff #45405)

Got it.

Guys, what do you think? This is complex but we see no other way to robustly support globals.

m.ostapenko marked 2 inline comments as done.Feb 1 2016, 7:33 AM

Ping. Kostya, could you please take a look?

samsonov edited edge metadata.Feb 2 2016, 10:37 AM

Sorry for delayed response. Please add an LLVM test for this instrumentation.

lib/Transforms/Instrumentation/AddressSanitizer.cpp
104 ↗(On Diff #45405)

Consider naming this to __asan_odr_gen, so that we could know that these new globals are smth. added by ASan.

m.ostapenko added inline comments.Feb 2 2016, 11:40 AM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
104 ↗(On Diff #45405)

I'd like to, but I hit on this in Firefox: ASan explicitly exports all __asan_* symbols and if we compile our code with e.g -fvisibility=hidden, we can have false positive ODR violation errors.

samsonov added inline comments.Feb 2 2016, 2:23 PM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
104 ↗(On Diff #45405)

Oh, I see... __odr_asan_gen_ ? :)

m.ostapenko updated this revision to Diff 47006.Feb 5 2016, 3:30 AM
m.ostapenko updated this object.
m.ostapenko edited edge metadata.

Updating according to Alexey's review.

I've also realized, that we should not copy all attributes from NewGlobal (e.g. we don't need it's alignment and section name) to ODR indicator symbol (thanks Yura for pointing to this!), we just need some of them (Visibility, DLLStorageClass and ThreadLocalMode). Full list of attributes available for copying is here:

LinkageType               (set in constructor)
VisibilityType
DLLStorageClassType
Section
Alignment
ThreadLocalMode      (set in constructor)
isExternallyInitialized  (set in constructor)

Does this look better now?

m.ostapenko marked 8 inline comments as done.Feb 5 2016, 3:31 AM
samsonov accepted this revision.Feb 5 2016, 2:26 PM
samsonov edited edge metadata.

LGTM

lib/Transforms/Instrumentation/AddressSanitizer.cpp
1412 ↗(On Diff #47006)

Side note: M.getName() might not be unique when you compile/link together a bunch of source files. It shouldn't be relevant here, as global has internal linkage.

test/Instrumentation/AddressSanitizer/local_alias.ll
26 ↗(On Diff #47006)

Do you actually need two functions in this test?

This revision is now accepted and ready to land.Feb 5 2016, 2:26 PM
m.ostapenko edited edge metadata.

Alexey, thank you! Uploading the patch to be committed.

This revision was automatically updated to reflect the committed changes.