[asan] Move instrumented null-terminated strings to a special section

[kubamracek]

Summary

On Darwin, simple C null-terminated constant strings normally end up in the __TEXT,__cstring section of the resulting Mach-O binary. When instrumented with ASan, these strings are transformed in a way that they cannot be in __cstring (the linker unifies the content of this section and strips extra NUL bytes, which would break instrumentation), and are put into a generic __const section. This breaks some of the tools that we have: Some tools need to scan all C null-terminated strings in Mach-O binaries, and scanning all the contents of __const has a large performance penalty. This patch instead introduces a special section, __asan_cstring which will now hold the instrumented null-terminated strings.

[m.ostapenko]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

1539

Oh, too many nested ifs here. Could you use && instead?

[zaks.anna]

LGTM

lib/Transforms/Instrumentation/AddressSanitizer.cpp

1543

Should this have SectionKind::getReadOnly()?

[zaks.anna]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

1543

Kuba pointed out that this API does not take the Kind. The section will be read-only because it is in the __TEXT segment.

[kcc]

Is it possible to also add a runnable test to compiler-rt?
I expect to make some changes here soon: https://github.com/google/sanitizers/issues/260#issuecomment-250062528
And by doing that may break your test and your fix.

[kubamracek]

Updating patch. Lowering the number of nested ifs. Adding a testcase for compiler-rt. Updating the odr-lto.cc testcase.

[kubamracek]

ping

[filcab]

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

11

What happened here? There's no error any more? Do we still need to use -asan-use-private-alias, etc?

[kubamracek]

ping

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

11

This tests checks that -asan-use-private-alias fixes a known bug when using LTO+ASan. Eventually, I wanted to turn -asan-use-private-alias on by default to get rid of this annoying bug causing many false positives. This patch fixes accidentally also this bug for a lot of cases (when strings are involved), but I believe there are other (less common) instances of the LTO+ASan bug that aren’t fixed. I would still like to keep the odr-lto.cc testcase, because it tests that -asan-use-private-alias doesn’t regress. If you want me to, I can try to create a new test that would trigger the LTO+ASan bug.

[kubamracek]

Updating the ODR testcase. If the global is not a C string (e.g. it contains a NUL byte), it will not be moved to __asan_cstring section, but LTO still likes to merge such globals.

[zaks.anna]

Looks good. I do not think there are any outstanding issues here. Please, commit.

[kubamracek]

Landed in LLVM r285619, compiler-rt r285620.

lib/Transforms/Instrumentation/AddressSanitizer.cpp

for (size_t i = 0; i < n; i++) {
     if (G->isConstant() && Linkage == GlobalValue::PrivateLinkage)
       Linkage = GlobalValue::InternalLinkage;
     GlobalVariable *NewGlobal =
         new GlobalVariable(M, NewTy, G->isConstant(), Linkage, NewInitializer,
                            "", G, G->getThreadLocalMode());
     NewGlobal->copyAttributesFrom(G);
     NewGlobal->setAlignment(MinRZ);
 
+    // Move null-terminated C strings to "__asan_cstring" section on Darwin.
+    if (TargetTriple.isOSBinFormatMachO() && !G->hasSection() &&
+        G->isConstant()) {
+      auto Seq = dyn_cast<ConstantDataSequential>(G->getInitializer());
+      if (Seq && Seq->isCString())
+        NewGlobal->setSection("__TEXT,__asan_cstring,regular");
+    }
+
     // Transfer the debug info.  The payload starts at offset zero so we can
     // copy the debug info over as is.
     SmallVector<DIGlobalVariable *, 1> GVs;
     G->getDebugInfo(GVs);
     for (auto *GV : GVs)
       NewGlobal->addDebugInfo(GV);
 
     Value *Indices2[2];

projects/compiler-rt/test/asan/TestCases/Darwin/cstring_section.c

+// Test that AddressSanitizer moves constant strings into a separate section.
+
+// RUN: %clang_asan -c -o %t %s
+// RUN: llvm-objdump -s %t | FileCheck %s
+
+// Check that "Hello.\n" is in __asan_cstring and not in __cstring.
+// CHECK: Contents of section __asan_cstring:
+// CHECK: 48656c6c {{.*}} Hello.
+// CHECK: Contents of section __const:
+// CHECK-NOT: 48656c6c {{.*}} Hello.
+// CHECK: Contents of section __cstring:
+// CHECK-NOT: 48656c6c {{.*}} Hello.
+
+int main(int argc, char *argv[]) {
+  argv[0] = "Hello.\n";
+  return 0;
+}

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

 // Check that -asan-use-private-alias 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
 // RUN: %clangxx_asan -DPART=1 -c %s -o %t-2.o -flto
 // RUN: %clangxx_asan %t-1.o %t-2.o -o %t -flto
 // RUN: not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-ODR
 
 // 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 %t-1.o %t-2.o -o %t -flto
 // RUN: %env_asan_opts=use_odr_indicator=1 %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-NO-ODR
 
 #include <stdio.h>
 #include <stdlib.h>
 void putstest();
 
 #if PART == 1
 
+static const char *my_global = "test\n\00abc";
+
 int main()
 {
-  fputs("test\n", stderr);
+  fputs(my_global, stderr);
   putstest();
   fprintf(stderr, "Done.\n");
   return 0;
 }
 
 #else // PART == 1
 
+static const char *my_other_global = "test\n\00abc";
+
 void putstest()
 {
-  fputs("test\n", stderr);
+  fputs(my_other_global, stderr);
 }
 
 #endif // PART == 1
 
 // CHECK-ODR: ERROR: AddressSanitizer: odr-violation
 // CHECK-NO-ODR-NOT: ERROR: AddressSanitizer: odr-violation
 // CHECK-NO-ODR: Done.

test/Instrumentation/AddressSanitizer/global_cstring_darwin.ll

+; This test checks that instrumented global C (null terminated) strings are put into a special section on Darwin.
+; RUN: opt < %s -asan -asan-module -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.10.0"
+
+; Should be put into __asan_cstring section:
+@.str.1 = private unnamed_addr constant [13 x i8] c"Hello world.\00", align 1
+@.str.2 = private unnamed_addr constant [4 x i8] c"%s\0A\00", align 1
+
+; CHECK: @.str.1 = internal unnamed_addr constant { [13 x i8], [51 x i8] } { [13 x i8] c"Hello world.\00", [51 x i8] zeroinitializer }, section "__TEXT,__asan_cstring,regular", align 32
+; CHECK: @.str.2 = internal unnamed_addr constant { [4 x i8], [60 x i8] } { [4 x i8] c"%s\0A\00", [60 x i8] zeroinitializer }, section "__TEXT,__asan_cstring,regular", align 32
+
+; Shouldn't be put into special section:
+@.str.3 = private unnamed_addr constant [4 x i8] c"\00\01\02\03", align 1
+@.str.4 = private unnamed_addr global [7 x i8] c"Hello.\00", align 1
+@.str.5 = private unnamed_addr constant [8 x i8] c"Hello.\00\00", align 1
+
+; CHECK: @.str.3 = internal unnamed_addr constant { [4 x i8], [60 x i8] } { [4 x i8] c"\00\01\02\03", [60 x i8] zeroinitializer }, align 32
+; CHECK: @.str.4 = private unnamed_addr global { [7 x i8], [57 x i8] } { [7 x i8] c"Hello.\00", [57 x i8] zeroinitializer }, align 32
+; CHECK: @.str.5 = internal unnamed_addr constant { [8 x i8], [56 x i8] } { [8 x i8] c"Hello.\00\00", [56 x i8] zeroinitializer }, align 32