[asan] Rename the ABI versioning symbol to '__asan_version_mismatch_check' instead abusing '__asan_init'

[kubamracek]

Summary

Hi everyone,

I'd like to propose a change in the way we version the ASan libraries (and other sanitizers as well), and I'd like to get feedback on this proposal. We currently have the "ABI version" number embedded into the ASan initializer symbol name (__asan_init_v5), which is occasionally bumped when there is a significant ABI-breaking change. My understanding is that this number doesn't guarantee compatibility. It indicates that the ABI changed, but there might have been smaller incompatible changes when the version is unchanged. This can easily cause issues especially when using dynamic libraries (.dylib, .so), because they are (and need to be) distributed with applications and libraries, so users can easily end up using a stale version of the ASan library.

I'd like to include the LLVM release number into that symbol name so users are actually forced to use the library that was distributed with the compiler they used. We should also provide a way for vendors who make their own releases to change this version to indicate that it's a different build than the open-source one. Local (development) builds should also have a different version. This could reuse a version-string schema already used by Clang.

Secondly, I'd like to make the error message about a version mismatch clearer by changing the name of the undefined symbol to be something like __asan_version_mismatch_check_xxx (following by the version string). We obviously don't want the initializer to be named like that, so it's a separate symbol.

Attached is an incomplete (work in progress, llvm backend part missing) patch which outlines how this could be done.

Any feedback is welcome!

Kuba

[kcc]

I don't like it. While it may solve your problem, it will create new problems for us.
asan run-time is build by several different build systems and your patch modifies one of them (llvm cmake),
but it requires to change all others (gcc's makefiles and our internal build system at least, probably more).

I would prefer a solution that does not change the build files, or a solution that changes *only* build files (thus being llvm-specific)

[kubamracek]

Can I also have your opinion on the renaming of the symbol, e.g.:

... make the error message about a version mismatch clearer by changing the name of the undefined symbol to be something like __asan_version_mismatch_check_xxx (followed by the version string). We obviously don't want the initializer to be named like that, so it's a separate symbol.

Thanks!

[kcc]

In D11004#201766, @kubabrecka wrote:

Can I also have your opinion on the renaming of the symbol, e.g.:

... make the error message about a version mismatch clearer by changing the name of the undefined symbol to be something like __asan_version_mismatch_check_xxx (followed by the version string). We obviously don't want the initializer to be named like that, so it's a separate symbol.

Yes, this part clearly makes sense.

Thanks!

[kubamracek]

Hi, I've removed the build system changes, and now this patch is only about renaming the symbol.

We currently version __asan_init and when the ABI version doesn't match, the linker gives a undefined reference to '__asan_init_v5' message. From this, it might not be obvious that it's actually a version mismatch error. This patch makes the error message much clearer by changing the name of the undefined symbol to be __asan_version_mismatch_check_xxx (followed by the version string). We obviously don't want the initializer to be named like that, so it's a separate symbol that is used only for the purpose of version checking.

[kcc]

LGTM
Please watch the bots once you commit, this is a potentially disrupting change.

[kubamracek]

Landed in r243003 (llvm part) and r243004 (compiler-rt part). Thanks for the review!

[kubamracek]

Windows bot was failing some tests, so I had to add the new interface function into asan_win_dll_thunk.cc in r243005 and r243007.

include/llvm/Transforms/Utils/ModuleUtils.h

 // This family of functions perform manipulations on Modules.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TRANSFORMS_UTILS_MODULEUTILS_H
 #define LLVM_TRANSFORMS_UTILS_MODULEUTILS_H
 
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringRef.h"
 #include <utility> // for std::pair
 
 namespace llvm {
 
 class Module;
 class Function;
 class GlobalValue;
 class GlobalVariable;

 Function *checkSanitizerInterfaceFunction(Constant *FuncOrBitcast);
 
 /// \brief Creates sanitizer constructor function, and calls sanitizer's init
 /// function from it.
 /// \return Returns pair of pointers to constructor, and init functions
 /// respectively.
 std::pair<Function *, Function *> createSanitizerCtorAndInitFunctions(
     Module &M, StringRef CtorName, StringRef InitName,
-    ArrayRef<Type *> InitArgTypes, ArrayRef<Value *> InitArgs);
+    ArrayRef<Type *> InitArgTypes, ArrayRef<Value *> InitArgs,
+    StringRef VersionCheckName = StringRef());
 } // End llvm namespace
 
 #endif //  LLVM_TRANSFORMS_UTILS_MODULEUTILS_H

lib/Transforms/Instrumentation/AddressSanitizer.cpp

 static const char *const kAsanModuleDtorName = "asan.module_dtor";
 static const uint64_t kAsanCtorAndDtorPriority = 1;
 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_v5";
+static const char *const kAsanInitName = "__asan_init";
+static const char *const kAsanVersionCheckName =
+    "__asan_version_mismatch_check_v6";
 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 kSanCovGenPrefix = "__sancov_gen_";
bool AddressSanitizer::doInitialization(Module &M) {
 
   C = &(M.getContext());
   LongSize = M.getDataLayout().getPointerSizeInBits();
   IntptrTy = Type::getIntNTy(*C, LongSize);
   TargetTriple = Triple(M.getTargetTriple());
 
   if (!CompileKernel) {
     std::tie(AsanCtorFunction, AsanInitFunction) =
-        createSanitizerCtorAndInitFunctions(M, kAsanModuleCtorName, kAsanInitName,
-                                            /*InitArgTypes=*/{},
-                                            /*InitArgs=*/{});
+        createSanitizerCtorAndInitFunctions(
+            M, kAsanModuleCtorName, kAsanInitName,
+            /*InitArgTypes=*/{}, /*InitArgs=*/{}, kAsanVersionCheckName);
     appendToGlobalCtors(M, AsanCtorFunction, kAsanCtorAndDtorPriority);
   }
   Mapping = getShadowMapping(TargetTriple, LongSize, CompileKernel);
   return true;
 }
 
 bool AddressSanitizer::maybeInsertAsanInitAtFunctionEntry(Function &F) {
   // For each NSObject descendant having a +load method, this method is invoked

lib/Transforms/Utils/ModuleUtils.cpp

Function *llvm::checkSanitizerInterfaceFunction(Constant *FuncOrBitcast) {
   std::string Err;
   raw_string_ostream Stream(Err);
   Stream << "Sanitizer interface function redefined: " << *FuncOrBitcast;
   report_fatal_error(Err);
 }
 
 std::pair<Function *, Function *> llvm::createSanitizerCtorAndInitFunctions(
     Module &M, StringRef CtorName, StringRef InitName,
-    ArrayRef<Type *> InitArgTypes, ArrayRef<Value *> InitArgs) {
+    ArrayRef<Type *> InitArgTypes, ArrayRef<Value *> InitArgs,
+    StringRef VersionCheckName) {
   assert(!InitName.empty() && "Expected init function name");
   assert(InitArgTypes.size() == InitArgTypes.size() &&
          "Sanitizer's init function expects different number of arguments");
   Function *Ctor = Function::Create(
       FunctionType::get(Type::getVoidTy(M.getContext()), false),
       GlobalValue::InternalLinkage, CtorName, &M);
   BasicBlock *CtorBB = BasicBlock::Create(M.getContext(), "", Ctor);
   IRBuilder<> IRB(ReturnInst::Create(M.getContext(), CtorBB));
   Function *InitFunction =
       checkSanitizerInterfaceFunction(M.getOrInsertFunction(
           InitName, FunctionType::get(IRB.getVoidTy(), InitArgTypes, false),
           AttributeSet()));
   InitFunction->setLinkage(Function::ExternalLinkage);
   IRB.CreateCall(InitFunction, InitArgs);
+  if (!VersionCheckName.empty()) {
+    Function *VersionCheckFunction =
+        checkSanitizerInterfaceFunction(M.getOrInsertFunction(
+            VersionCheckName, FunctionType::get(IRB.getVoidTy(), {}, false),
+            AttributeSet()));
+    IRB.CreateCall(VersionCheckFunction, {});
+  }
   return std::make_pair(Ctor, InitFunction);
 }
 

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

extern "C" {
   // Changes between ABI versions:
   // 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.
-  #define __asan_init __asan_init_v5
-  #define __asan_init_name "__asan_init_v5"
+  // v5=>v6: changed the name of the version check symbol
+  #define __asan_version_mismatch_check __asan_version_mismatch_check_v6
 }
 
 #endif  // ASAN_INIT_VERSION_H

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

 
 #include "asan_init_version.h"
 
 using __sanitizer::uptr;
 
 extern "C" {
   // This function should be called at the very beginning of the process,
   // before any instrumented code is executed and before any call to malloc.
-  // Please note that __asan_init is a macro that is replaced with
-  // __asan_init_vXXX at compile-time.
   SANITIZER_INTERFACE_ATTRIBUTE void __asan_init();
 
+  // This function exists purely to get a linker/loader error when using
+  // incompatible versions of instrumentation and runtime library. Please note
+  // that __asan_version_mismatch_check is a macro that is replaced with
+  // __asan_version_mismatch_check_vXXX at compile-time.
+  SANITIZER_INTERFACE_ATTRIBUTE void __asan_version_mismatch_check();
+
   // This structure is used to describe the source location of a place where
   // global was defined.
   struct __asan_global_source_location {
     const char *filename;
     int line_no;
     int column_no;
   };
 

projects/compiler-rt/lib/asan/asan_rtl.cc

 }
 
 // Initialize as requested from instrumented application code.
 // We use this call as a trigger to wake up ASan from deactivated state.
 void __asan_init() {
   AsanActivate();
   AsanInitInternal();
 }
+
+void __asan_version_mismatch_check() {
+  // Do nothing.
+}

projects/compiler-rt/lib/asan/asan_win_dll_thunk.cc

extern "C" {
   // Manually wrap __asan_init as we need to initialize
   // __asan_option_detect_stack_use_after_return afterwards.
   void __asan_init() {
     typedef void (*fntype)();
     static fntype fn = 0;
     // __asan_init is expected to be called by only one thread.
     if (fn) return;
 
-    fn = (fntype)getRealProcAddressOrDie(__asan_init_name);
+    fn = (fntype)getRealProcAddressOrDie("__asan_init");
     fn();
     __asan_option_detect_stack_use_after_return =
         (__asan_should_detect_stack_use_after_return() != 0);
 
     InterceptHooks();
   }
 }
 

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

 // Check the presence of interface symbols in the ASan runtime dylib.
 // If you're changing this file, please also change
 // ../Linux/interface_symbols.c
 
 // RUN: %clang_asan -dead_strip -O2 %s -o %t.exe
 // RUN: rm -f %t.symbols %t.interface
 
 // RUN: nm -g `%clang_asan %s -fsanitize=address -### 2>&1 | grep "libclang_rt.asan_osx_dynamic.dylib" | sed -e 's/.*"\(.*libclang_rt.asan_osx_dynamic.dylib\)".*/\1/'` \
 // RUN:   | grep " T " | sed "s/.* T //" \
 // RUN:   | grep "__asan_" | sed "s/___asan_/__asan_/" \
-// RUN:   | sed -E "s/__asan_init_v[0-9]+/__asan_init/" \
+// RUN:   | sed -E "s/__asan_version_mismatch_check_v[0-9]+/__asan_version_mismatch_check/" \
 // RUN:   | grep -v "__asan_default_options" \
 // RUN:   | grep -v "__asan_on_error" > %t.symbols
 
 // RUN: cat %p/../../../../lib/asan/asan_interface_internal.h \
 // RUN:    | sed "s/\/\/.*//" | sed "s/typedef.*//" \
 // RUN:    | grep -v "OPTIONAL" \
 // RUN:    | grep "__asan_.*(" | sed "s/.* __asan_/__asan_/;s/(.*//" \
 // RUN:    > %t.interface

projects/compiler-rt/test/asan/TestCases/Linux/interface_symbols_linux.c

 // Check the presence of interface symbols in compiled file.
 
 // RUN: %clang_asan -O2 %s -o %t.exe
 // RUN: nm -D %t.exe | grep " T " | sed "s/.* T //" \
 // RUN:    | grep "__asan_" | sed "s/___asan_/__asan_/" \
-// RUN:    | sed -E "s/__asan_init_v[0-9]+/__asan_init/" \
+// RUN:    | sed -E "s/__asan_version_mismatch_check_v[0-9]+/__asan_version_mismatch_check/" \
 // RUN:    | grep -v "__asan_default_options" \
 // RUN:    | grep -v "__asan_stack_" \
 // RUN:    | grep -v "__asan_on_error" > %t.symbols
 // RUN: cat %p/../../../../lib/asan/asan_interface_internal.h \
 // RUN:    | sed "s/\/\/.*//" | sed "s/typedef.*//" \
 // RUN:    | grep -v "OPTIONAL" \
 // RUN:    | grep "__asan_.*(" | sed "s/.* __asan_/__asan_/;s/(.*//" \
 // RUN:    > %t.interface