This is an archive of the discontinued LLVM Phabricator instance.

[asan] Split -asan-use-private-alias to -asan-use-odr-indicator
ClosedPublic

Authored by vitalybuka on Nov 30 2018, 5:21 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

vitalybuka created this revision.Nov 30 2018, 5:21 PM

Hm, I'd rather separate private aliases and odr indicators because they are separate (although connected) features:

  • Private aliases are used to avoid ASan false positives when we mix instrumented and non-instrumented modules (also, AFAIR, FPs are possible when using LTO). They can be used with and without ODR indicators.
  • ODR indicators were introduced to improve ODR violation in presence of private aliases, but they can be omitted if desired.

Hm, I'd rather separate private aliases and odr indicators because they are separate (although connected) features:

I'll do this.

  • Private aliases are used to avoid ASan false positives when we mix instrumented and non-instrumented modules (also, AFAIR, FPs are possible when using LTO). They can be used with and without ODR indicators.
  • ODR indicators were introduced to improve ODR violation in presence of private aliases, but they can be omitted if desired.

Can you please elaborate on how it improves "use_odr_indicator"? Do we miss some violations if we "have aliases without indicators" comparing to default:"have no aliases and have no indicators"?

vitalybuka retitled this revision from [asan] Rename -asan-use-private-alias to -asan-use-odr-indicator to [asan] Split -asan-use-private-alias to -asan-use-odr-indicator.Dec 3 2018, 5:56 PM
vitalybuka updated this revision to Diff 176527.Dec 3 2018, 5:57 PM

Rename->Split

vitalybuka updated this revision to Diff 176530.Dec 3 2018, 6:37 PM

updated tests

vitalybuka updated this revision to Diff 176531.Dec 3 2018, 6:40 PM
vitalybuka marked an inline comment as done.

fixed typo in test

vitalybuka marked an inline comment as not done.Dec 3 2018, 6:59 PM
vitalybuka added inline comments.
compiler-rt/test/asan/TestCases/Linux/odr-violation.cc
29 ↗(On Diff #176530)

Here is a problem. If we just enable -asan-use-private-alias we miss reports which we had on line 10.
So I am not sure if there is any value in using aliases without indicators.

So I am not sure if there is any value in using aliases without indicators.

Oh, this is a long story... In short: if we don't use private aliases, we'll may have ASan false positives when mixing sanitized and non-sanitized code depending on ordering during linkage.
Consider the following example (from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68016#c8):

max@max:/tmp$ cat libfoo.c

long h = 15;
long f = 4;
long foo (long *p) {
  return *p;
}

max@max:/tmp$ cat libbar.c 

extern void abort (void);
long foo (long *);
long h = 12;
long i = 13;
long f = 5;

int bar () {
  if (foo (&f) != 5 || foo (&h) != 12 || foo (&i) != 13)
    abort ();
  return 0;
}

max@max:/tmp$ cat main.c

int bar ();

int main () {
  return bar ();
}
$ clang libfoo.c -shared -fpic -o libfoo.so  -g -fsanitize=address
$ clang libbar.c -shared -fpic -o libbar.so -g
$ clang main.c -c -o main.o
$ clang main.o  ./libbar.so ./libfoo.so -o main -fsanitize=address
$ ./main

==6425==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7efc5f2f2040 at pc 0x7efc5eeef8d1 bp 0x7ffddc2a2ff0 sp 0x7ffddc2a2fe8
READ of size 8 at 0x7efc5f2f2040 thread T0
    #0 0x7efc5eeef8d0 in foo /tmp/libfoo.c:4:10
    #1 0x7efc5f0f16cf in bar /tmp/libbar.c:8:7
    #2 0x4f1575 in main (/tmp/main+0x4f1575)
    #3 0x7efc5dffc82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #4 0x41a378 in _start (/tmp/main+0x41a378)

0x7efc5f2f2040 is located 0 bytes inside of global variable 'f' defined in 'libfoo.c:2:6' (0x7efc5f2f2040) of size 8
0x7efc5f2f2040 is located 8 bytes to the right of global variable 'h' defined in 'libfoo.c:1:6' (0x7efc5f2f2030) of size 8
SUMMARY: AddressSanitizer: global-buffer-overflow /tmp/libfoo.c:4:10 in foo
Shadow bytes around the buggy address:
  0x0fe00be563b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe00be563c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe00be563d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe00be563e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe00be563f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0fe00be56400: 00 00 00 00 00 00 00 f9[f9]f9 f9 f9 f9 f9 f9 f9
  0x0fe00be56410: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe00be56420: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe00be56430: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe00be56440: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe00be56450: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==6425==ABORTING

Here, global symbols 'f' and 'h' are resolved to libbar.so, that is not sanitized. However, when libfoo.so registers its "own" globals, it actually poisons libbar.so's 'f' and 'h' that are not properly padded.

There also was an issue with LTO, see https://github.com/google/sanitizers/issues/647

We can use private aliases w/o indicators if we don't care about ODR violation and don't want to pay for odr indicators size.

Can you please elaborate on how it improves "use_odr_indicator"? Do we miss some violations if we "have aliases without indicators" comparing to default:"have no aliases and have no indicators"?
Here is a problem. If we just enable -asan-use-private-alias we miss reports which we had on line 10.

That's correct. Right now detector relies on fact that if we have an ODR violation for some pair of globals, when we register the second one, its will be already poisoned (this will be true at least for ELF). So, effectively it checks that we try to poison the same global twice.
However, this won't be the case when we use private aliases, because we won't try to poison one global twice, we'll poison their private aliases that reside in their corresponding DSOs. In this case, even when we have actual ODR violation, we won't be able to catch it.
And here odr indicators come -- they mimic original globals and allow us to detect ODR violation in a pretty similar manner as before, but without poisoning innocent globals from non-instrumented DSOs.

eugenis accepted this revision.Dec 4 2018, 12:48 PM

LGTM

This revision is now accepted and ready to land.Dec 4 2018, 12:48 PM
This revision was automatically updated to reflect the committed changes.