[compiler-rt] Don't instrument globals from __objc_classname

[kubamracek]

Summary

Fixes https://code.google.com/p/address-sanitizer/issues/detail?id=360 – Using Obj-C blocks triggers a false-positive ODR violation

The change in r221451 (http://llvm.org/viewvc/llvm-project?view=revision&revision=221451) caused Obj-C related global constants to be instrumented, but the linker rearranges the sections, so the redzones break. We already added a check to skip objc_methname section from being instrumented because of this, see r221480 (http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/AddressSanitizer.cpp?view=diff&r1=221479&r2=221480&pathrev=221480). The same issue still exists for class names in objc_classname, this patch skips the __objc_classname section from being instrumented, as well.

[samsonov]

LGTM, but let's see if glider@ has anything to add.

[kledzik]

Can you just generalize ShouldInstrumentGlobal() to return false for all cstring_literals sections? The linker is always going to coalesce sections of that type.

[kubamracek]

Changing the patch to disable instrumentation of all cstring_literals sections.

[majnemer]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

1047

This is not reliable, it could end with ", cstring_literals".

Can you please use MCSectionMachO::ParseSectionSpecifier to parse the section string instead?

[kubamracek]

Modified the patch to use MCSectionMachO::ParseSectionSpecifier to correctly parse the section name, segment name and attributes.

I had to modify a couple of function prototypes to pass the llvm::Triple in order to avoid creating the Triple object for each global. Not sure if there's an easier way.

[glider]

LGTM

lib/Transforms/Instrumentation/AddressSanitizer.cpp

1271–1272

How about making TargetTriple a class member?

[majnemer]

Why does ShouldInstrumentGlobal need a triple argument? GlobalVariable has a getParent method which will return the Module.

[kubamracek]

Making TargetTriple a class member.

[kubamracek]

In D6488#17, @majnemer wrote:

Why does ShouldInstrumentGlobal need a triple argument? GlobalVariable has a getParent method which will return the Module.

Because Module only stores the triple as a string, and I wanted to avoid parsing the string for every single global.

[ygribov]

Could someone comment on issues raised in GCC Bugzilla about related bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63888 ?

[kubamracek]

Is it okay to commit this patch?

[majnemer]

I'm cool with it and @glider and @samsonov seem to be as well.

[kubamracek]

Landed in r223513 and r223514.

lib/Transforms/Instrumentation/AddressSanitizer.cpp

 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/InstVisitor.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"
+#include "llvm/MC/MCSectionMachO.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/DataTypes.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/SwapByteOrder.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Utils/ASanStackFrameLayout.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"

 /// This struct defines the shadow mapping using the rule:
 ///   shadow = (mem >> Scale) ADD-or-OR Offset.
 struct ShadowMapping {
   int Scale;
   uint64_t Offset;
   bool OrShadowOffset;
 };
 
-static ShadowMapping getShadowMapping(const Module &M, int LongSize) {
-  llvm::Triple TargetTriple(M.getTargetTriple());
+static ShadowMapping getShadowMapping(Triple &TargetTriple, int LongSize) {
   bool IsAndroid = TargetTriple.getEnvironment() == llvm::Triple::Android;
   bool IsIOS = TargetTriple.isiOS();
   bool IsFreeBSD = TargetTriple.isOSFreeBSD();
   bool IsLinux = TargetTriple.isOSLinux();
   bool IsPPC64 = TargetTriple.getArch() == llvm::Triple::ppc64 ||
                  TargetTriple.getArch() == llvm::Triple::ppc64le;
   bool IsX86_64 = TargetTriple.getArch() == llvm::Triple::x86_64;
   bool IsMIPS32 = TargetTriple.getArch() == llvm::Triple::mips ||

  private:
   void initializeCallbacks(Module &M);
 
   bool LooksLikeCodeInBug11395(Instruction *I);
   bool GlobalIsLinkerInitialized(GlobalVariable *G);
 
   LLVMContext *C;
   const DataLayout *DL;
+  Triple TargetTriple;
   int LongSize;
   Type *IntptrTy;
   ShadowMapping Mapping;
   DominatorTree *DT;
   Function *AsanCtorFunction;
   Function *AsanInitFunction;
   Function *AsanHandleNoReturnFunc;
   Function *AsanPtrCmpFunction, *AsanPtrSubFunction;
private:
   size_t MinRedzoneSizeForGlobal() const {
     return RedzoneSizeForScale(Mapping.Scale);
   }
 
   GlobalsMetadata GlobalsMD;
   Type *IntptrTy;
   LLVMContext *C;
   const DataLayout *DL;
+  Triple TargetTriple;
   ShadowMapping Mapping;
   Function *AsanPoisonGlobals;
   Function *AsanUnpoisonGlobals;
   Function *AsanRegisterGlobals;
   Function *AsanUnregisterGlobals;
 };
 
 // Stack poisoning does not play well with exception handling.
bool AddressSanitizerModule::ShouldInstrumentGlobal(GlobalVariable *G) {
   //   - Need to poison all copies, not just the main thread's one.
   if (G->isThreadLocal())
     return false;
   // For now, just ignore this Global if the alignment is large.
   if (G->getAlignment() > MinRedzoneSizeForGlobal()) return false;
 
   if (G->hasSection()) {
     StringRef Section(G->getSection());
+
+    if (TargetTriple.isOSBinFormatMachO()) {
+      StringRef ParsedSegment, ParsedSection;
+      unsigned TAA = 0, StubSize = 0;
+      bool TAAParsed;
+      std::string ErrorCode =
+        MCSectionMachO::ParseSectionSpecifier(Section, ParsedSegment,
+                                              ParsedSection, TAA, TAAParsed,
+                                              StubSize);
+      if (!ErrorCode.empty()) {
+        report_fatal_error("Invalid section specifier '" + ParsedSection +
+                           "': " + ErrorCode + ".");
+      }
+
       // Ignore the globals from the __OBJC section. The ObjC runtime assumes
       // those conform to /usr/lib/objc/runtime.h, so we can't add redzones to
       // them.
-    if (Section.startswith("__OBJC,") ||
-        Section.startswith("__DATA, __objc_")) {
+      if (ParsedSegment == "__OBJC" ||
+          (ParsedSegment == "__DATA" && ParsedSection.startswith("__objc_"))) {
         DEBUG(dbgs() << "Ignoring ObjC runtime global: " << *G << "\n");
         return false;
       }
       // See http://code.google.com/p/address-sanitizer/issues/detail?id=32
       // Constant CFString instances are compiled in the following way:
       //  -- the string buffer is emitted into
       //     __TEXT,__cstring,cstring_literals
       //  -- the constant NSConstantString structure referencing that buffer
       //     is placed into __DATA,__cfstring
       // Therefore there's no point in placing redzones into __DATA,__cfstring.
       // Moreover, it causes the linker to crash on OS X 10.7
-    if (Section.startswith("__DATA,__cfstring")) {
+      if (ParsedSegment == "__DATA" && ParsedSection == "__cfstring") {
         DEBUG(dbgs() << "Ignoring CFString: " << *G << "\n");
         return false;
       }
       // The linker merges the contents of cstring_literals and removes the
       // trailing zeroes.
-    if (Section.startswith("__TEXT,__cstring,cstring_literals")) {
+      if (ParsedSegment == "__TEXT" && (TAA & MachO::S_CSTRING_LITERALS)) {
         DEBUG(dbgs() << "Ignoring a cstring literal: " << *G << "\n");
         return false;
       }
-    if (Section.startswith("__TEXT,__objc_methname,cstring_literals")) {
-      DEBUG(dbgs() << "Ignoring objc_methname cstring global: " << *G << "\n");
-      return false;
     }
 
-
     // Callbacks put into the CRT initializer/terminator sections
     // should not be instrumented.
     // See https://code.google.com/p/address-sanitizer/issues/detail?id=305
     // and http://msdn.microsoft.com/en-US/en-en/library/bb918180(v=vs.120).aspx
     if (Section.startswith(".CRT")) {
       DEBUG(dbgs() << "Ignoring a global initializer callback: " << *G << "\n");
       return false;
     }

 bool AddressSanitizerModule::runOnModule(Module &M) {
   DataLayoutPass *DLP = getAnalysisIfAvailable<DataLayoutPass>();
   if (!DLP)
     return false;
   DL = &DLP->getDataLayout();
   C = &(M.getContext());
   int LongSize = DL->getPointerSizeInBits();
   IntptrTy = Type::getIntNTy(*C, LongSize);
-  Mapping = getShadowMapping(M, LongSize);
+  TargetTriple = Triple(M.getTargetTriple());
+  Mapping = getShadowMapping(TargetTriple, LongSize);
   initializeCallbacks(M);
 
   bool Changed = false;
 
   Function *CtorFunc = M.getFunction(kAsanModuleCtorName);
   assert(CtorFunc);
   IRBuilder<> IRB(CtorFunc->getEntryBlock().getTerminator());
 
if (!DLP)
     report_fatal_error("data layout missing");
   DL = &DLP->getDataLayout();
 
   GlobalsMD.init(M);
 
   C = &(M.getContext());
   LongSize = DL->getPointerSizeInBits();
   IntptrTy = Type::getIntNTy(*C, LongSize);
+  TargetTriple = Triple(M.getTargetTriple());
 
   AsanCtorFunction = Function::Create(
       FunctionType::get(Type::getVoidTy(*C), false),
       GlobalValue::InternalLinkage, kAsanModuleCtorName, &M);
   BasicBlock *AsanCtorBB = BasicBlock::Create(*C, "", AsanCtorFunction);
   // call __asan_init in the module ctor.
   IRBuilder<> IRB(ReturnInst::Create(*C, AsanCtorBB));
   AsanInitFunction = checkInterfaceFunction(
       M.getOrInsertFunction(kAsanInitName, IRB.getVoidTy(), nullptr));
   AsanInitFunction->setLinkage(Function::ExternalLinkage);
   IRB.CreateCall(AsanInitFunction);
 
-  Mapping = getShadowMapping(M, LongSize);
+  Mapping = getShadowMapping(TargetTriple, LongSize);
 
   appendToGlobalCtors(M, AsanCtorFunction, kAsanCtorAndDtorPriority);
   return true;
 }
 
 bool AddressSanitizer::maybeInsertAsanInitAtFunctionEntry(Function &F) {
   // For each NSObject descendant having a +load method, this method is invoked
   // by the ObjC runtime before any of the static constructors is called.

projects/compiler-rt/test/asan/TestCases/Darwin/objc-odr.mm

+// Regression test for
+// https://code.google.com/p/address-sanitizer/issues/detail?id=360.
+
+// RUN: %clang_asan %s -o %t -framework Foundation
+// RUN: %run %t 2>&1 | FileCheck %s
+
+#import <Foundation/Foundation.h>
+
+void f() {
+    int y = 7;
+    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_BACKGROUND, 0), ^{
+        dispatch_sync(dispatch_get_main_queue(), ^{
+            printf("num = %d\n", y);
+        });
+    });
+}
+
+int main() {
+  NSLog(@"Hello world");
+}
+
+// CHECK-NOT: AddressSanitizer: odr-violation
+// CHECK: Hello world