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

[m.ostapenko]

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.

[kcc]

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.

[m.ostapenko]

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]

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

[m.ostapenko]

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]

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]

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]

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.

[m.ostapenko]

Updating according to Yura's feedback.

[ygribov]

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.

[ygribov]

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

[m.ostapenko]

Ping. Kostya, could you please take a look?

[samsonov]

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]

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]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

104 ↗

(On Diff #45405)

Oh, I see... __odr_asan_gen_ ? :)

[m.ostapenko]

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?

[samsonov]

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?

[m.ostapenko]

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

llvm/trunk/lib/Transforms/Instrumentation/AddressSanitizer.cpp

 static const char *const kAsanReportErrorTemplate = "__asan_report_";
 static const char *const kAsanRegisterGlobalsName = "__asan_register_globals";
 static const char *const kAsanUnregisterGlobalsName =
     "__asan_unregister_globals";
 static const char *const kAsanPoisonGlobalsName = "__asan_before_dynamic_init";
 static const char *const kAsanUnpoisonGlobalsName = "__asan_after_dynamic_init";
 static const char *const kAsanInitName = "__asan_init";
 static const char *const kAsanVersionCheckName =
-    "__asan_version_mismatch_check_v6";
+    "__asan_version_mismatch_check_v7";
 static const char *const kAsanPtrCmp = "__sanitizer_ptr_cmp";
 static const char *const kAsanPtrSub = "__sanitizer_ptr_sub";
 static const char *const kAsanHandleNoReturnName = "__asan_handle_no_return";
 static const int kMaxAsanStackMallocSizeClass = 10;
 static const char *const kAsanStackMallocNameTemplate = "__asan_stack_malloc_";
 static const char *const kAsanStackFreeNameTemplate = "__asan_stack_free_";
 static const char *const kAsanGenPrefix = "__asan_gen_";
+static const char *const kODRGenPrefix = "__odr_asan_gen_";
 static const char *const kSanCovGenPrefix = "__sancov_gen_";
 static const char *const kAsanPoisonStackMemoryName =
     "__asan_poison_stack_memory";
 static const char *const kAsanUnpoisonStackMemoryName =
     "__asan_unpoison_stack_memory";
 
 static const char *const kAsanOptionDetectUAR =
     "__asan_option_detect_stack_use_after_return";
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",
+                                cl::desc("Use private aliases for global"
+                                         " variables"),
+                                cl::Hidden, cl::init(false));
+
 // Debug flags.
 static cl::opt<int> ClDebug("asan-debug", cl::desc("debug"), cl::Hidden,
                             cl::init(0));
 static cl::opt<int> ClDebugStack("asan-debug-stack", cl::desc("debug stack"),
                                  cl::Hidden, cl::init(0));
 static cl::opt<std::string> ClDebugFunc("asan-debug-func", cl::Hidden,
                                         cl::desc("Debug func"));
 static cl::opt<int> ClDebugMin("asan-debug-min", cl::desc("Debug min inst"),
auto GV = new GlobalVariable(M, LocStruct->getType(), true,
                                GlobalValue::PrivateLinkage, LocStruct,
                                kAsanGenPrefix);
   GV->setUnnamedAddr(true);
   return GV;
 }
 
 static bool GlobalWasGeneratedByAsan(GlobalVariable *G) {
   return G->getName().find(kAsanGenPrefix) == 0 ||
-         G->getName().find(kSanCovGenPrefix) == 0;
+         G->getName().find(kSanCovGenPrefix) == 0 ||
+         G->getName().find(kODRGenPrefix) == 0;
 }
 
 Value *AddressSanitizer::memToShadow(Value *Shadow, IRBuilder<> &IRB) {
   // Shadow >> scale
   Shadow = IRB.CreateLShr(Shadow, Mapping.Scale);
   if (Mapping.Offset == 0) return Shadow;
   // (Shadow >> scale) | offset
   if (Mapping.OrShadowOffset)
bool AddressSanitizerModule::InstrumentGlobals(IRBuilder<> &IRB, Module &M) {
   // A global is described by a structure
   //   size_t beg;
   //   size_t size;
   //   size_t size_with_redzone;
   //   const char *name;
   //   const char *module_name;
   //   size_t has_dynamic_init;
   //   void *source_location;
+  //   size_t odr_indicator;
   // We initialize an array of such structures and pass it to a run-time call.
   StructType *GlobalStructTy =
       StructType::get(IntptrTy, IntptrTy, IntptrTy, IntptrTy, IntptrTy,
-                      IntptrTy, IntptrTy, nullptr);
+                      IntptrTy, IntptrTy, IntptrTy, nullptr);
   SmallVector<Constant *, 16> Initializers(n);
 
   bool HasDynamicallyInitializedGlobals = false;
 
   // We shouldn't merge same module names, as this string serves as unique
   // module ID in runtime.
   GlobalVariable *ModuleName = createPrivateGlobalForString(
       M, M.getModuleIdentifier(), /*AllowMerging*/ false);
 
   auto &DL = M.getDataLayout();
   for (size_t i = 0; i < n; i++) {
     static const uint64_t kMaxGlobalRedzone = 1 << 18;
     GlobalVariable *G = GlobalsToChange[i];
 
     auto MD = GlobalsMD.get(G);
+    StringRef NameForGlobal = G->getName();
     // Create string holding the global name (use global name from metadata
     // if it's available, otherwise just write the name of global variable).
     GlobalVariable *Name = createPrivateGlobalForString(
-        M, MD.Name.empty() ? G->getName() : MD.Name,
+        M, MD.Name.empty() ? NameForGlobal : MD.Name,
         /*AllowMerging*/ true);
 
     Type *Ty = G->getValueType();
     uint64_t SizeInBytes = DL.getTypeAllocSize(Ty);
     uint64_t MinRZ = MinRedzoneSizeForGlobal();
     // MinRZ <= RZ <= kMaxGlobalRedzone
     // and trying to make RZ to be ~ 1/4 of SizeInBytes.
     uint64_t RZ = std::max(
for (size_t i = 0; i < n; i++) {
     Constant *SourceLoc;
     if (!MD.SourceLoc.empty()) {
       auto SourceLocGlobal = createPrivateGlobalForSourceLoc(M, MD.SourceLoc);
       SourceLoc = ConstantExpr::getPointerCast(SourceLocGlobal, IntptrTy);
     } else {
       SourceLoc = ConstantInt::get(IntptrTy, 0);
     }
 
+    Constant *ODRIndicator = ConstantExpr::getNullValue(IRB.getInt8PtrTy());
+    GlobalValue *InstrumentedGlobal = NewGlobal;
+
+    bool CanUsePrivateAliases = TargetTriple.isOSBinFormatELF();
+    if (CanUsePrivateAliases && ClUsePrivateAliasForGlobals) {
+      // Create local alias for NewGlobal to avoid crash on ODR between
+      // instrumented and non-instrumented libraries.
+      auto *GA = GlobalAlias::create(GlobalValue::InternalLinkage,
+                                     NameForGlobal + M.getName(), NewGlobal);
+
+      // 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;
+    }
+
     Initializers[i] = ConstantStruct::get(
-        GlobalStructTy, ConstantExpr::getPointerCast(NewGlobal, IntptrTy),
+        GlobalStructTy,
+        ConstantExpr::getPointerCast(InstrumentedGlobal, IntptrTy),
         ConstantInt::get(IntptrTy, SizeInBytes),
         ConstantInt::get(IntptrTy, SizeInBytes + RightRedzoneSize),
         ConstantExpr::getPointerCast(Name, IntptrTy),
         ConstantExpr::getPointerCast(ModuleName, IntptrTy),
-        ConstantInt::get(IntptrTy, MD.IsDynInit), SourceLoc, nullptr);
+        ConstantInt::get(IntptrTy, MD.IsDynInit), SourceLoc,
+        ConstantExpr::getPointerCast(ODRIndicator, IntptrTy), nullptr);
 
     if (ClInitializers && MD.IsDynInit) HasDynamicallyInitializedGlobals = true;
 
     DEBUG(dbgs() << "NEW GLOBAL: " << *NewGlobal << "\n");
   }
 
   ArrayType *ArrayOfGlobalStructTy = ArrayType::get(GlobalStructTy, n);
   GlobalVariable *AllGlobals = new GlobalVariable(

llvm/trunk/test/Instrumentation/AddressSanitizer/local_alias.ll

+; RUN: opt < %s -asan -asan-module -asan-use-private-alias=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
+; CHECK: @"a<stdin>" = internal 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
+}