[AddressSanitizer] Add flag to disable linking with CXX runtime

[evgeny777]

Summary

This prevents link errors when operators new/delete are overridden by application.

[kcc]

I am reluctant to add any new flags if there is any possibility to avoid doing so.
Is there any other solution for your problem?
E.g. you can disable the override of new/delete when building with asan.

[evgeny777]

Unfortunately, this is not an option. I have very custom heap implementation (in fact multiple heap types sharing contiguous memory block), so ASAN heap can't be a drop-in replacement. I'm using manual poisoning and it works pretty well, but have to disable C++ runtime each time I build with ASAN.

Another problem is ASANing libcxx tests because of overridden new operator (count_new.hpp). I think it makes sense to have counterpart for -fsanitize-link-c++-runtime, no?

[kcc]

In D54905#1312386, @evgeny777 wrote:

Unfortunately, this is not an option. I have very custom heap implementation (in fact multiple heap types sharing contiguous memory block), so ASAN heap can't be a drop-in replacement. I'm using manual poisoning and it works pretty well,

It may appear to work well, but still be less powerful than what ASAN does for the regular malloc.
Do you have readzones? quarantine? [de]allocation stack traces?

but have to disable C++ runtime each time I build with ASAN.

How do you do this now?

Another problem is ASANing libcxx tests because of overridden new operator (count_new.hpp). I think it makes sense to have counterpart for -fsanitize-link-c++-runtime, no?

Is that related to https://github.com/google/sanitizers/issues/295 ?

[eugenis]

Would turning asan operator new/delete into weak symbols help?

[kcc]

weak new/delete may solve this particular problem, but may cause confusion to lots of other users,
e.g. in cases when a user has overridden new/delete for non-essential reasons (e.g. debuging) and can disable the overrides in asan mode.
With weak symbols such a user will never know they have a problem.

[eugenis]

Our malloc definition is weak already, this will only change new/delete to match.

This also makes the behaviour of static runtime consistent with the one of dynamic runtime where we can not prevent user from overloading operator new/delete.

[evgeny777]

How do you do this now?

Sometimes with this patch, sometimes simply issuing linker command line manually

Is that related to https://github.com/google/sanitizers/issues/295 ?

I don't think so. I'm experiencing link error, not C++ standard discrepancy.

Would turning asan operator new/delete into weak symbols help?

The new operator is already a weak symbol in libcxx, so it looks like an ODR violation, doesn't it?

[eugenis]

Would turning asan operator new/delete into weak symbols help?

The new operator is already a weak symbol in libcxx, so it looks like an ODR violation, doesn't it?

I don't think so. It's just symbol interposition. Every module in the process gets the same definition.

[evgeny777]

don't think so. It's just symbol interposition. Every module in the process gets the same definition.

If I have two weak symbols in lib1.a and lib2.a and don't define strong one, which one will be chosen at link time?

[eugenis]

Linux linkers won't include a member of an archive only because it resolves a weak symbol, so in your example the answer is none.

ASan uses -Wl,-whole-archive to pull all its members, this will resolve operator new to ASan definition unless there is another definition in the main executable. In general, the first one in the order of command line arguments will be used.

[evgeny777]

ASan uses -Wl,-whole-archive to pull all its members

Yep, I forgot about this. Looks like weak new/delete will do the job then.

include/clang/Driver/Options.td

 def fno_sanitize_undefined_trap_on_error : Flag<["-"], "fno-sanitize-undefined-trap-on-error">,
                                            Group<f_clang_Group>;
 def fsanitize_minimal_runtime : Flag<["-"], "fsanitize-minimal-runtime">,
                                         Group<f_clang_Group>;
 def fno_sanitize_minimal_runtime : Flag<["-"], "fno-sanitize-minimal-runtime">,
                                         Group<f_clang_Group>;
 def fsanitize_link_cxx_runtime : Flag<["-"], "fsanitize-link-c++-runtime">,
                                  Group<f_clang_Group>;
+def fno_sanitize_link_cxx_runtime : Flag<["-"], "fno-sanitize-link-c++-runtime">,
+                                 Group<f_clang_Group>;
 def fsanitize_cfi_cross_dso : Flag<["-"], "fsanitize-cfi-cross-dso">,
                               Group<f_clang_Group>,
                               HelpText<"Enable control flow integrity (CFI) checks for cross-DSO calls.">;
 def fno_sanitize_cfi_cross_dso : Flag<["-"], "fno-sanitize-cfi-cross-dso">,
                                  Flags<[CoreOption, DriverOption]>,
                                  Group<f_clang_Group>,
                                  HelpText<"Disable control flow integrity (CFI) checks for cross-DSO calls.">;
 def fsanitize_cfi_icall_generalize_pointers : Flag<["-"], "fsanitize-cfi-icall-generalize-pointers">,

lib/Driver/SanitizerArgs.cpp

SanitizerArgs::SanitizerArgs(const ToolChain &TC,
 
   if (AllAddedKinds & SafeStack) {
     // SafeStack runtime is built into the system on Fuchsia.
     SafeStackRuntime = !TC.getTriple().isOSFuchsia();
   }
 
   // Parse -link-cxx-sanitizer flag.
   LinkCXXRuntimes =
-      Args.hasArg(options::OPT_fsanitize_link_cxx_runtime) || D.CCCIsCXX();
+      Args.hasFlag(options::OPT_fsanitize_link_cxx_runtime,
+                   options::OPT_fno_sanitize_link_cxx_runtime, D.CCCIsCXX());
 
   // Finally, initialize the set of available and recoverable sanitizers.
   Sanitizers.Mask |= Kinds;
   RecoverableSanitizers.Mask |= RecoverableKinds;
   TrapSanitizers.Mask |= TrappingKinds;
   assert(!(RecoverableKinds & TrappingKinds) &&
          "Overlap between recoverable and trapping sanitizers");
 }

test/Driver/sanitizer-ld.c

 // RUN:     --sysroot=%S/Inputs/basic_linux_tree -lstdc++ -static 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-ASAN-LINUX-CXX-STATIC %s
 //
 // CHECK-ASAN-LINUX-CXX-STATIC: "{{(.*[^-.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
 // CHECK-ASAN-LINUX-CXX-STATIC-NOT: stdc++
 // CHECK-ASAN-LINUX-CXX-STATIC: "--whole-archive" "{{.*}}libclang_rt.asan-i386.a" "--no-whole-archive"
 // CHECK-ASAN-LINUX-CXX-STATIC: stdc++
 
+// RUN: %clangxx -x c++ %s -### -o /dev/null -fsanitize=address -fno-sanitize-link-c++-runtime \
+// RUN:     -target i386-unknown-linux -fuse-ld=ld 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-ASAN-NO-LINK-CXX-RUNTIME %s
+//
+// CHECK-ASAN-NO-LINK-CXX-RUNTIME-NOT: asan_cxx
+
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN:     -target arm-linux-gnueabi -fuse-ld=ld -fsanitize=address \
 // RUN:     --sysroot=%S/Inputs/basic_android_tree/sysroot \
 // RUN:   | FileCheck --check-prefix=CHECK-ASAN-ARM %s
 //
 // CHECK-ASAN-ARM: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
 // CHECK-ASAN-ARM-NOT: "-lc"
 // CHECK-ASAN-ARM: libclang_rt.asan-arm.a"