[asan] Use private aliases for global variables (compiler-rt part).

[m.ostapenko]

Summary

This is a compiler-rt part of this http://reviews.llvm.org/D15642 patch. Here, we change the way how we detect ODR violation. Instead of using __asan_region_is_poisoned(g->beg, g->size_with_redzone) on global address (that would be false now due to using private alias), we use new globally visible indicator symbol to perform the check.

Does this look like a reasonable approach?

[kcc]

test/asan/TestCases/Linux/local_alias.cc

1 ↗

(On Diff #43231)

a comment, please

7 ↗

(On Diff #43231)

name this more descriptively, e.g. BUILD_INSTRUMENTED_DSO and BUILD_UNINSTRUMENTED_DSO

15 ↗

(On Diff #43231)

isn't this a real ODR which we now will fail to detect?

[m.ostapenko]

test/asan/TestCases/Linux/local_alias.cc

15 ↗

(On Diff #43231)

Yes, we can't detect ODR violation here, because libfoo.so is sanitized and libbar.so is not. But how could we do it with existing approach? And anyway, this particular testcase fails with CHECK on x86 and with false positive global buffer overflow on x86_64. I think it's better to miss ODR violation than just miserably die :).

[kcc]

test/asan/TestCases/Linux/local_alias.cc

15 ↗

(On Diff #43231)

With the current approach we do die, even though miserably. So, the bug is in fact detected.
Now we will miss it completely. Right?

[m.ostapenko]

test/asan/TestCases/Linux/local_alias.cc

15 ↗

(On Diff #43231)

Right. But still, there's C code with semantic interposition that would fail here unconditionally.

[ygribov]

test/asan/TestCases/Linux/local_alias.cc

15 ↗

(On Diff #43231)

Well, we'd really die accidentally. It could also lead to all sorts of misbehavior or even go undetected. So I'd rather not consider this a real existing feature.

On the contrary, we'll now detect errors in libraries dlopened with RTLD_DEEPBIND or linked with -Bsymbolic.

[kcc]

test/asan/TestCases/Linux/local_alias.cc

15 ↗

(On Diff #43231)

I don't consider this as a blocker, just want to check my understanding.

[m.ostapenko]

Distinguish between two methods of ODR violation checking in runtime: old one (use __asan_region_is_poisoned) and new one (use odr indicators). This code looks scary, but it manages to preserve default behavior and enable new one if desired.

[m.ostapenko]

This is a draft implementation, for further design discussion.

[ygribov]

I guess you need to update asan_init version.

lib/asan/asan_globals.cc

139 ↗

(On Diff #45281)

"in case compiler instruments global variables through their local aliases"?

143 ↗

(On Diff #45281)

s/registred/registered/, s/vioation/violation/

144 ↗

(On Diff #45281)

You could simplify a bit by doing the easy case first and then early-exiting.

247 ↗

(On Diff #45281)

What if indicator was set by a different module?

[m.ostapenko]

lib/asan/asan_globals.cc

247 ↗

(On Diff #45281)

Well, if not consider concurrently running dlopen and dclose, this should never happen, because we have caught ODR violation, that happened before, no? If consider concurrency issues - yes, your nit is totally reasonable. Since original code seems not be thread safe, I didn't implement my changes to be thread safe either. So I wonder, if I should consider races here.

[m.ostapenko]

Addressing Yura's nits (except the last one, that I want to discuss a bit more).

[ygribov]

lib/asan/asan_globals.cc

143 ↗

(On Diff #45407)

I think you forgot to set *odr_indicator to 1 here? Or rather to some descriptive enum value e.g. DEFINED. This would also simplify below description.

156 ↗

(On Diff #45407)

s/posoned/poisoned/

176 ↗

(On Diff #45407)

Some spelling suggestions: s/cheeply/cheaply/, s/try poison/try to poison an/

247 ↗

(On Diff #45407)

Got it.

[m.ostapenko]

lib/asan/asan_globals.cc

143 ↗

(On Diff #45407)

Yeah, thanks. Just remembered about this 5 minutes ago ☺.

176 ↗

(On Diff #45407)

Ouch, thanks.

[m.ostapenko]

Fix an error with setting odr_indicator to 1 in CheckODRViolationViaIndicator + fix some spelling issues. I've also updated existing odr-violation.cc testcase to cover new functionality.

[m.ostapenko]

Ping. Actually, I think ODR indicators are quite independent from ASan (well, they still need to be registered on the very early stage, but all poisoning stuff can be skipped), perhaps we could move them out to separate module in the future?

[samsonov]

Sorry for the delays. Overall, this looks good. However, please note that this is an ABI-breaking change: you will need to update asan_init_version.h

lib/asan/asan_interface_internal.h

49 ↗

(On Diff #45547)

Out of curiosity - why not make this new field last in the __asan_global structure?

[m.ostapenko]

However, please note that this is an ABI-breaking change: you will need to update asan_init_version.h

Right, thanks (though Yura had already pointed on this, but I forgot to make this change).

lib/asan/asan_interface_internal.h

49 ↗

(On Diff #45547)

Yeah, you are right, will fix this.

[m.ostapenko]

Updating according to Alexey's nits.

[samsonov]

LGTM

lib/asan/asan_globals.cc

139 ↗

(On Diff #47008)

Why not UNREGISTERED/REGISTERED?

[m.ostapenko]

Thanks! Uploading the diff to be committed.

compiler-rt/trunk/lib/asan/asan_flags.inc

ASAN_FLAG(int, detect_odr_violation, 2,
           "If ==1, detect ODR-violation only if the two variables "
           "have different sizes")
 ASAN_FLAG(bool, dump_instruction_bytes, false,
           "If true, dump 16 bytes starting at the instruction that caused SEGV")
 ASAN_FLAG(const char *, suppressions, "", "Suppressions file name.")
 ASAN_FLAG(bool, halt_on_error, true,
           "Crash the program after printing the first error report "
           "(WARNING: USE AT YOUR OWN RISK!)")
+ASAN_FLAG(bool, use_odr_indicator, false,
+          "Use special ODR indicator symbol for ODR violation detection")

compiler-rt/trunk/lib/asan/asan_globals.cc

if (GetGlobalsForAddress(addr, &g, nullptr, 1)) {
     descr->region_address = g.beg;
     descr->region_size = g.size;
     descr->region_kind = "global";
     return true;
   }
   return false;
 }
 
+enum GlobalSymbolState {
+  UNREGISTERED = 0,
+  REGISTERED = 1
+};
+
+// Check ODR violation for given global G via special ODR indicator. We use
+// this method in case compiler instruments global variables through their
+// local aliases.
+static void CheckODRViolationViaIndicator(const Global *g) {
+  u8 *odr_indicator = reinterpret_cast<u8 *>(g->odr_indicator);
+  if (*odr_indicator == UNREGISTERED) {
+    *odr_indicator = REGISTERED;
+    return;
+  }
+  // If *odr_indicator is DEFINED, some module have already registered
+  // externally visible symbol with the same name. This is an ODR violation.
+  for (ListOfGlobals *l = list_of_all_globals; l; l = l->next) {
+    if (g->odr_indicator == l->g->odr_indicator &&
+        (flags()->detect_odr_violation >= 2 || g->size != l->g->size) &&
+        !IsODRViolationSuppressed(g->name))
+      ReportODRViolation(g, FindRegistrationSite(g),
+                         l->g, FindRegistrationSite(l->g));
+  }
+}
+
+// Check ODR violation for given global G by checking if it's already poisoned.
+// We use this method in case compiler doesn't use private aliases for global
+// variables.
+static void CheckODRViolationViaPoisoning(const Global *g) {
+  if (__asan_region_is_poisoned(g->beg, g->size_with_redzone)) {
+    // This check may not be enough: if the first global is much larger
+    // the entire redzone of the second global may be within the first global.
+    for (ListOfGlobals *l = list_of_all_globals; l; l = l->next) {
+      if (g->beg == l->g->beg &&
+          (flags()->detect_odr_violation >= 2 || g->size != l->g->size) &&
+          !IsODRViolationSuppressed(g->name))
+        ReportODRViolation(g, FindRegistrationSite(g),
+                           l->g, FindRegistrationSite(l->g));
+    }
+  }
+}
+
+// Clang provides two different ways for global variables protection:
+// it can poison the global itself or it's private alias. In former
+// case we may poison same symbol multiple times, that can help us to
+// cheaply detect ODR violation: if we try to poison an already poisoned
+// global, we have ODR violation error.
+// In latter case, we poison each symbol exactly once, so we use special
+// indicator symbol to perform similar check.
+// In either case, compiler provides a special odr_indicator field to Global
+// structure, that can contain two kinds of values:
+//   1) Non-zero value. In this case, odr_indicator is an address of
+//      corresponding indicator variable for given global.
+//   2) Zero. This means that we don't use private aliases for global variables
+//      and can freely check ODR violation with the first method.
+//
+// This routine chooses between two different methods of ODR violation
+// detection.
+static inline bool UseODRIndicator(const Global *g) {
+  // Use ODR indicator method iff use_odr_indicator flag is set and
+  // indicator symbol address is not 0.
+  return flags()->use_odr_indicator && g->odr_indicator > 0;
+}
+
 // Register a global variable.
 // This function may be called more than once for every global
 // so we store the globals in a map.
 static void RegisterGlobal(const Global *g) {
   CHECK(asan_inited);
   if (flags()->report_globals >= 2)
     ReportGlobal(*g, "Added");
   CHECK(flags()->report_globals);
   CHECK(AddrIsInMem(g->beg));
   if (!AddrIsAlignedByGranularity(g->beg)) {
     Report("The following global variable is not properly aligned.\n");
     Report("This may happen if another global with the same name\n");
     Report("resides in another non-instrumented module.\n");
     Report("Or the global comes from a C file built w/o -fno-common.\n");
     Report("In either case this is likely an ODR violation bug,\n");
     Report("but AddressSanitizer can not provide more details.\n");
     ReportODRViolation(g, FindRegistrationSite(g), g, FindRegistrationSite(g));
     CHECK(AddrIsAlignedByGranularity(g->beg));
   }
   CHECK(AddrIsAlignedByGranularity(g->size_with_redzone));
   if (flags()->detect_odr_violation) {
     // Try detecting ODR (One Definition Rule) violation, i.e. the situation
     // where two globals with the same name are defined in different modules.
-    if (__asan_region_is_poisoned(g->beg, g->size_with_redzone)) {
-      // This check may not be enough: if the first global is much larger
-      // the entire redzone of the second global may be within the first global.
-      for (ListOfGlobals *l = list_of_all_globals; l; l = l->next) {
-        if (g->beg == l->g->beg &&
-            (flags()->detect_odr_violation >= 2 || g->size != l->g->size) &&
-            !IsODRViolationSuppressed(g->name))
-          ReportODRViolation(g, FindRegistrationSite(g),
-                             l->g, FindRegistrationSite(l->g));
-      }
-    }
+    if (UseODRIndicator(g))
+      CheckODRViolationViaIndicator(g);
+    else
+      CheckODRViolationViaPoisoning(g);
   }
   if (CanPoisonMemory())
     PoisonRedZones(*g);
   ListOfGlobals *l = new(allocator_for_globals) ListOfGlobals;
   l->g = g;
   l->next = list_of_all_globals;
   list_of_all_globals = l;
   if (g->has_dynamic_init) {
static void UnregisterGlobal(const Global *g) {
   CHECK(AddrIsInMem(g->beg));
   CHECK(AddrIsAlignedByGranularity(g->beg));
   CHECK(AddrIsAlignedByGranularity(g->size_with_redzone));
   if (CanPoisonMemory())
     PoisonShadowForGlobal(g, 0);
   // We unpoison the shadow memory for the global but we do not remove it from
   // the list because that would require O(n^2) time with the current list
   // implementation. It might not be worth doing anyway.
+
+  // Release ODR indicator.
+  if (UseODRIndicator(g)) {
+    u8 *odr_indicator = reinterpret_cast<u8 *>(g->odr_indicator);
+    *odr_indicator = UNREGISTERED;
+  }
 }
 
 void StopInitOrderChecking() {
   BlockingMutexLock lock(&mu_for_globals);
   if (!flags()->check_initialization_order || !dynamic_init_globals)
     return;
   flags()->check_initialization_order = false;
   for (uptr i = 0, n = dynamic_init_globals->size(); i < n; ++i) {

compiler-rt/trunk/lib/asan/asan_init_version.h

extern "C" {
   // v1=>v2: added 'module_name' to __asan_global
   // v2=>v3: stack frame description (created by the compiler)
   //         contains the function PC as the 3-rd field (see
   //         DescribeAddressIfStack).
   // v3=>v4: added '__asan_global_source_location' to __asan_global.
   // v4=>v5: changed the semantics and format of __asan_stack_malloc_ and
   //         __asan_stack_free_ functions.
   // v5=>v6: changed the name of the version check symbol
-  #define __asan_version_mismatch_check __asan_version_mismatch_check_v6
+  // v6=>v7: added 'odr_indicator' to __asan_global.
+  #define __asan_version_mismatch_check __asan_version_mismatch_check_v7
 }
 
 #endif  // ASAN_INIT_VERSION_H

compiler-rt/trunk/lib/asan/asan_interface_internal.h

struct __asan_global {
     uptr size;               // The original size of the global.
     uptr size_with_redzone;  // The size with the redzone.
     const char *name;        // Name as a C string.
     const char *module_name; // Module name as a C string. This pointer is a
                              // unique identifier of a module.
     uptr has_dynamic_init;   // Non-zero if the global has dynamic initializer.
     __asan_global_source_location *location;  // Source location of a global,
                                               // or NULL if it is unknown.
+    uptr odr_indicator;      // The address of the ODR indicator symbol.
   };
 
   // These two functions should be called by the instrumented code.
   // 'globals' is an array of structures describing 'n' globals.
   SANITIZER_INTERFACE_ATTRIBUTE
   void __asan_register_globals(__asan_global *globals, uptr n);
   SANITIZER_INTERFACE_ATTRIBUTE
   void __asan_unregister_globals(__asan_global *globals, uptr n);

compiler-rt/trunk/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.
+//
+// RUN: %clangxx_asan -DBUILD_INSTRUMENTED_DSO=1 -fPIC -shared -mllvm -asan-use-private-alias %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_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;
+}
+#elif defined (BUILD_UNINSTRUMENTED_DSO)
+long foo(long *);
+long h = 12;
+long i = 13;
+long f = 5;
+
+int bar() {
+  if (foo(&f) != 5 || foo(&h) != 12 || foo(&i) != 13)
+    return 1;
+  return 0;
+}
+#else
+extern int bar();
+
+int main() {
+  return bar();
+}
+#endif

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

 // RUN: %env_asan_opts=fast_unwind_on_malloc=0:detect_odr_violation=1     %run %t-ODR-EXE 2>&1 | FileCheck %s --check-prefix=DISABLED
 // 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.
+// 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: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