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

[m.ostapenko]

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.

[vitalybuka]

In D55156#1316695, @m.ostapenko wrote:

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]

Rename->Split

[vitalybuka]

updated tests

[vitalybuka]

fixed typo in test

[vitalybuka]

compiler-rt/test/asan/TestCases/Linux/odr-violation.cc

28

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.

[m.ostapenko]

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]

LGTM

compiler-rt/test/asan/TestCases/Darwin/odr-lto.cc

-// Check that -asan-use-private-alias and use_odr_indicator=1 silence the false
+// Check that -asan-use-odr-indicator and use_odr_indicator=1 silence the false
 // positive ODR violation on Darwin with LTO.
 
 // REQUIRES: lto
 
-// RUN: %clangxx_asan -DPART=0 -c %s -o %t-1.o -flto -mllvm -asan-use-private-alias
-// RUN: %clangxx_asan -DPART=1 -c %s -o %t-2.o -flto -mllvm -asan-use-private-alias
+// RUN: %clangxx_asan -DPART=0 -c %s -o %t-1.o -flto -mllvm -asan-use-odr-indicator
+// RUN: %clangxx_asan -DPART=1 -c %s -o %t-2.o -flto -mllvm -asan-use-odr-indicator
 // RUN: %clangxx_asan %t-1.o %t-2.o -o %t -flto
 // RUN: %env_asan_opts=use_odr_indicator=1 %run %t 2>&1 | FileCheck %s
 
 #include <stdio.h>
 #include <stdlib.h>
 void putstest();
 
 #if PART == 1

compiler-rt/test/asan/TestCases/Linux/local_alias.cc

 // Test that mixing instrumented and non-instrumented code doesn't lead to crash.
 // Build two modules (one is instrumented, another is not) that have globals
 // with same names. Check, that ASan doesn't crash with CHECK failure or
 // false positive global-buffer-overflow due to sanitized library poisons
 // globals from non-sanitized one.
 //
 // FIXME: https://github.com/google/sanitizers/issues/316
 // XFAIL: android
 //
-// RUN: %clangxx_asan -DBUILD_INSTRUMENTED_DSO=1 -fPIC -shared -mllvm -asan-use-private-alias %s -o %t-INSTRUMENTED-SO.so
+// RUN: %clangxx_asan -DBUILD_INSTRUMENTED_DSO=1 -fPIC -shared -mllvm -asan-use-odr-indicator %s -o %t-INSTRUMENTED-SO.so
 // RUN: %clangxx -DBUILD_UNINSTRUMENTED_DSO=1 -fPIC -shared %s -o %t-UNINSTRUMENTED-SO.so
-// RUN: %clangxx %s -c -mllvm -asan-use-private-alias -o %t.o
+// RUN: %clangxx %s -c -mllvm -asan-use-odr-indicator -o %t.o
 // RUN: %clangxx_asan %t.o %t-UNINSTRUMENTED-SO.so %t-INSTRUMENTED-SO.so -o %t-EXE
 // RUN: %env_asan_opts=use_odr_indicator=true %run %t-EXE
 
 #if defined (BUILD_INSTRUMENTED_DSO)
 long h = 15;
 long f = 4;
 long foo(long *p) {
   return *p;

compiler-rt/test/asan/TestCases/Linux/odr-violation.cc

 // RUN: %env_asan_opts=fast_unwind_on_malloc=0                        not %run %t-ODR-EXE 2>&1 | FileCheck %s
 // RUN: echo "odr_violation:foo::ZZZ" > %t.supp
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0:detect_odr_violation=2:suppressions=%t.supp  not %run %t-ODR-EXE 2>&1 | FileCheck %s
 // RUN: echo "odr_violation:foo::G" > %t.supp
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0:detect_odr_violation=2:suppressions=%t.supp      %run %t-ODR-EXE 2>&1 | FileCheck %s --check-prefix=DISABLED
 // RUN: rm -f %t.supp
 //
 // Use private aliases for global variables: use indicator symbol to detect ODR violation.
-// RUN: %clangxx_asan -DBUILD_SO=1 -fPIC -shared -mllvm -asan-use-private-alias %s -o %t-ODR-SO.so -DSZ=100
-// RUN: %clangxx_asan -mllvm -asan-use-private-alias %s %t-ODR-SO.so -Wl,-R. -o %t-ODR-EXE
+// RUN: %clangxx_asan -DBUILD_SO=1 -fPIC -shared -mllvm -asan-use-odr-indicator %s -o %t-ODR-SO.so -DSZ=100
+// RUN: %clangxx_asan -mllvm -asan-use-odr-indicator %s %t-ODR-SO.so -Wl,-R. -o %t-ODR-EXE
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0                              %run %t-ODR-EXE 2>&1 | FileCheck %s --check-prefix=DISABLED
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0:use_odr_indicator=false      %run %t-ODR-EXE 2>&1 | FileCheck %s --check-prefix=DISABLED
 // RUN: %env_asan_opts=fast_unwind_on_malloc=0:use_odr_indicator=true   not %run %t-ODR-EXE 2>&1 | FileCheck %s
 
 // GNU driver doesn't handle .so files properly.
 // REQUIRES: Clang
 
 #ifndef SZ

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

static cl::opt<bool> ClDynamicAllocaStack(
     cl::desc("Use dynamic alloca to represent stack variables"), cl::Hidden,
     cl::init(true));
 
 static cl::opt<uint32_t> ClForceExperiment(
     "asan-force-experiment",
     cl::desc("Force optimization experiment (for testing)"), cl::Hidden,
     cl::init(0));
 
-static cl::opt<bool>
-    ClUsePrivateAliasForGlobals("asan-use-private-alias",
+static cl::opt<bool> ClUseOdrIndicator("asan-use-odr-indicator",
                                        cl::desc("Use private aliases for global"
                                                 " variables"),
                                        cl::Hidden, cl::init(false));
 
 static cl::opt<bool>
     ClUseGlobalsGC("asan-globals-live-support",
                    cl::desc("Use linker features to support dead "
                             "code stripping of globals"),
                    cl::Hidden, cl::init(true));
 
 // This is on by default even though there is a bug in gold:
for (size_t i = 0; i < n; i++) {
     }
 
     Constant *ODRIndicator = ConstantExpr::getNullValue(IRB.getInt8PtrTy());
     GlobalValue *InstrumentedGlobal = NewGlobal;
 
     bool CanUsePrivateAliases =
         TargetTriple.isOSBinFormatELF() || TargetTriple.isOSBinFormatMachO() ||
         TargetTriple.isOSBinFormatWasm();
-    if (CanUsePrivateAliases && ClUsePrivateAliasForGlobals) {
+    if (CanUsePrivateAliases && ClUseOdrIndicator) {
       // Create local alias for NewGlobal to avoid crash on ODR between
       // instrumented and non-instrumented libraries.
       auto *GA =
           GlobalAlias::create(GlobalValue::PrivateLinkage, "", NewGlobal);
 
       // With local aliases, we need to provide another externally visible
       // symbol __odr_asan_XXX to detect ODR violation.
       auto *ODRIndicatorSym =

llvm/test/Instrumentation/AddressSanitizer/local_alias.ll

-; RUN: opt < %s -asan -asan-module -asan-use-private-alias=1 -S | FileCheck %s
+; RUN: opt < %s -asan -asan-module -asan-use-odr-indicator=1 -S | FileCheck %s
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 @a = internal global [2 x i32] zeroinitializer, align 4
 
 ; Check that we generate internal alias and odr indicator symbols for global to be protected.
 ; CHECK: @__odr_asan_gen_a = internal global i8 0, align 1