[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

29

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/Linux/odr-violation.cc

 // RUN: %env_asan_opts=fast_unwind_on_malloc=0:detect_odr_violation=2 not %run %t-ODR-EXE 2>&1 | FileCheck %s
 // 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.
+// Use private aliases for global variables without indicator symbol.
 // 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: %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                          %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   %run %t-ODR-EXE 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+// 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 -mllvm -asan-use-odr-indicator  %s -o %t-ODR-SO.so -DSZ=100
+// RUN: %clangxx_asan -mllvm -asan-use-private-alias -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
 # define SZ 4
 #endif

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

static cl::opt<bool> ClDynamicAllocaStack(
     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",
-                                cl::desc("Use private aliases for global"
-                                         " variables"),
-                                cl::Hidden, cl::init(false));
+    ClUsePrivateAlias("asan-use-private-alias",
+                      cl::desc("Use private aliases for global variables"),
+                      cl::Hidden, cl::init(false));
+
+static cl::opt<bool>
+    ClUseOdrIndicator("asan-use-odr-indicator",
+                      cl::desc("Use odr indicators to improve ODR reporting"),
 
 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 && ClUsePrivateAlias) {
       // Create local alias for NewGlobal to avoid crash on ODR between
       // instrumented and non-instrumented libraries.
-      auto *GA =
+      InstrumentedGlobal =
           GlobalAlias::create(GlobalValue::PrivateLinkage, "", NewGlobal);
+    }
 
+    if (ClUseOdrIndicator) {
       // With local aliases, we need to provide another externally visible
       // symbol __odr_asan_XXX to detect ODR violation.
       auto *ODRIndicatorSym =
           new GlobalVariable(M, IRB.getInt8Ty(), false, Linkage,
                              Constant::getNullValue(IRB.getInt8Ty()),
                              kODRGenPrefix + NameForGlobal, nullptr,
                              NewGlobal->getThreadLocalMode());
 
       // Set meaningful attributes for indicator symbol.
       ODRIndicatorSym->setVisibility(NewGlobal->getVisibility());
       ODRIndicatorSym->setDLLStorageClass(NewGlobal->getDLLStorageClass());
       ODRIndicatorSym->setAlignment(1);
       ODRIndicator = ODRIndicatorSym;
-      InstrumentedGlobal = GA;
     }
 
     Constant *Initializer = ConstantStruct::get(
         GlobalStructTy,
         ConstantExpr::getPointerCast(InstrumentedGlobal, IntptrTy),
         ConstantInt::get(IntptrTy, SizeInBytes),
         ConstantInt::get(IntptrTy, SizeInBytes + RightRedzoneSize),
         ConstantExpr::getPointerCast(Name, IntptrTy),

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 -S | FileCheck %s --check-prefixes=CHECK-NOALIAS,CHECK-NOINDICATOR
+; RUN: opt < %s -asan -asan-module -asan-use-private-alias=1 -S | FileCheck %s --check-prefixes=CHECK-ALIAS,CHECK-NOINDICATOR
+; RUN: opt < %s -asan -asan-module -asan-use-odr-indicator=1 -S | FileCheck %s --check-prefixes=CHECK-INDICATOR,CHECK-NOALIAS
+; RUN: opt < %s -asan -asan-module -asan-use-private-alias=1 -asan-use-odr-indicator=1 -S | FileCheck %s --check-prefixes=CHECK-ALIAS,CHECK-INDICATOR
 
 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
-; CHECK: @0 = private alias { [2 x i32], [56 x i8] }, { [2 x i32], [56 x i8] }* @a
+; CHECK-NOINDICATOR-NOT: __odr_asan_gen_a
+; CHECK-noALIAS: private alias
+; CHECK-INDICATOR: @__odr_asan_gen_a = internal global i8 0, align 1
+; CHECK-ALIAS: @0 = private alias { [2 x i32], [56 x i8] }, { [2 x i32], [56 x i8] }* @a
 
 ; Function Attrs: nounwind sanitize_address uwtable
 define i32 @foo(i32 %M) #0 {
 entry:
   %M.addr = alloca i32, align 4
   store i32 %M, i32* %M.addr, align 4
   store volatile i32 6, i32* getelementptr inbounds ([2 x i32], [2 x i32]* @a, i64 2, i64 0), align 4
   %0 = load i32, i32* %M.addr, align 4
   %idxprom = sext i32 %0 to i64
   %arrayidx = getelementptr inbounds [2 x i32], [2 x i32]* @a, i64 0, i64 %idxprom
   %1 = load volatile i32, i32* %arrayidx, align 4
   ret i32 %1
 }