[clang] Allow `no_sanitize("all")` on global variables

[abrachet]

Summary

This is a trivial change because "all" is already correctly handled

[aaron.ballman]

Generally heading in the right direction, but missing some things:

1. Test coverage where you try to apply the attribute to something other than a global variable (like a function)
2. Changes to AttrDocs.td (it currently says what's supported is everything -fno-sanitize supports, but https://clang.llvm.org/docs/UsersManual.html#controlling-code-generation doesn't list all as an option)
3. Release notes

[aaron.ballman]

LGTM assuming precommit CI comes back clean, thanks!

[hctim]

Gosh, I really don't like the no_sanitize("all") - IMHO it's cutting off your nose to spite your face. Generally there's a specific reason to disable sanitization of a function / global variable - and that reason is almost always specific to a sanitizer. Disabling all sanitizers is a maintenance nightmare when you want to roll out a new sanitizer.

We have a similar problem in Android, where the build system supports a concept called sanitize: { none: true } which disables all sanitizers for a source file. It's a very poor decision in hindsight.

I would strongly discourage anyone from using this feature. We also already have __attribute__((disable_sanitizer_instrumentation)) which is the same thing, and from my understanding, is much more used.

Can you describe a use case where you'd use this, that isn't better served by individually disabling sanitizers?

[abrachet]

In D133031#3761904, @hctim wrote:

Gosh, I really don't like the no_sanitize("all") - IMHO it's cutting off your nose to spite your face. Generally there's a specific reason to disable sanitization of a function / global variable - and that reason is almost always specific to a sanitizer. Disabling all sanitizers is a maintenance nightmare when you want to roll out a new sanitizer.

We have a similar problem in Android, where the build system supports a concept called sanitize: { none: true } which disables all sanitizers for a source file. It's a very poor decision in hindsight.

I would strongly discourage anyone from using this feature. We also already have __attribute__((disable_sanitizer_instrumentation)) which is the same thing, and from my understanding, is much more used.

Can you describe a use case where you'd use this, that isn't better served by individually disabling sanitizers?

no_sanitize("all") is certainly a heavy hand, we use it sparingly, it's useful in code like ldso/libc before sanitizer runtimes are available. That doesn't actually apply to global variables, but it's common to just have a macro to disable sanitizers and having a separate one for globals and functions is inconvenient. For globals in particularly where we ran into this where a global is put into a section and dumped but the redzone was undesirable, we currently just use no_sanitize("address", "hwaddress"). Also as a side note/rant, I wish no_sanitize("address") implied no_sanitize("hwaddress").

Regarding __attribute__((disable_sanitizer_instrumentation)), I actually just found out about this when working on this patch, we plan on using it, but it isn't supported by gcc. Though as it turns out no_sanitize("...") is ignored and warned about for globals on gcc.

It is surprising that no_sanitize("all") is available for functions but not for globals, but if you feel strongly that no_sanitize("all") shouldn't be supported in more places then I don't have a huge preference that this be landed.

[hctim]

In D133031#3762026, @abrachet wrote:

no_sanitize("all") is certainly a heavy hand, we use it sparingly, it's useful in code like ldso/libc before sanitizer runtimes are available.

Yeah, we have to do the same in Android's libc (example), but it's still almost always more useful to be specific with the sanitizer. For example, if the code was updated to have ubsan-int-overflow checks with fsanitize-trap, and we used no_sanitize("all"), then we'd silently just never sanitize this code.

That doesn't actually apply to global variables, but it's common to just have a macro to disable sanitizers and having a separate one for globals and functions is inconvenient.

Agreed, having a single attribute is better, which is another reason why I don't like disable_sanitizer_instrumentation. If I had to pick a poison, it'd be no_sanitize("all"), but the former already exists :(.

Also as a side note/rant, I wish no_sanitize("address") implied no_sanitize("hwaddress").

Yeah, we often have the same semantics of "no asan + no hwasan [+ no memtag]". Better to keep them separate though because otherwise you need to handle the "no, ASan is fine, but seriously no hwasan" and no_sanitize("only_hwasan") makes me want to cry. This happens, for example, with global variables that are referenced by inline assembly. Fine to be ASan-ified, not okay to be HWASan-ified (as the address tag needs a special movk to materialize the tag bits) if the materialized pointer is passed to an instrumented function, for example.

Regarding __attribute__((disable_sanitizer_instrumentation)), I actually just found out about this when working on this patch, we plan on using it, but it isn't supported by gcc. Though as it turns out no_sanitize("...") is ignored and warned about for globals on gcc.

It is surprising that no_sanitize("all") is available for functions but not for globals, but if you feel strongly that no_sanitize("all") shouldn't be supported in more places then I don't have a huge preference that this be landed.

In a perfect world, I'd

1. delete disable_sanitizer_instrumentation and
2. delete no_sanitize("all") for code.

I don't think #2 is feasible, but maybe it's better we follow the gcc approach and make no_sanitize("all") a warning on global variables ("Hey, this doesn't work, and also you should really reconsider doing this for code as well").

I tried looking for references on #1 - only finding a linux kernel kcsan reference, which I'm wondering whether we can fix... I looked at a lot of the indexed code I have access to and only found that, tried grepping github but ended up getting rate-limited and not seeing results correctly (https://github.com/search?q=disable_sanitizer_instrumentation+-path%3Allvm+-path%3Atest%2FCodeGen+-path%3Aclang+-path%3Allvm-project+-filename%3Acompiler_attributes.h+-filename%3Aattr-disable-sanitizer-instrumentation.c&type=Code).

Sorry for the bad news, but yeah, I think I'd prefer not to add no_sanitize("all") for globals. Definitely willing to change my mind though if someone comes along with a compelling reason why they need the mega-hammer, but I really think disablement is a decision that should be done on a per-sanitizer basis.

[aaron.ballman]

In D133031#3762057, @hctim wrote:

In D133031#3762026, @abrachet wrote:

That doesn't actually apply to global variables, but it's common to just have a macro to disable sanitizers and having a separate one for globals and functions is inconvenient.

Agreed, having a single attribute is better, which is another reason why I don't like disable_sanitizer_instrumentation. If I had to pick a poison, it'd be no_sanitize("all"), but the former already exists :(.

That's not the end of the world; we can deprecate attributes if we think that's valuable to do so.

In a perfect world, I'd

1. delete disable_sanitizer_instrumentation and
2. delete no_sanitize("all") for code.

I don't think #2 is feasible, but maybe it's better we follow the gcc approach and make no_sanitize("all") a warning on global variables ("Hey, this doesn't work, and also you should really reconsider doing this for code as well").

I tried looking for references on #1 - only finding a linux kernel kcsan reference, which I'm wondering whether we can fix... I looked at a lot of the indexed code I have access to and only found that, tried grepping github but ended up getting rate-limited and not seeing results correctly (https://github.com/search?q=disable_sanitizer_instrumentation+-path%3Allvm+-path%3Atest%2FCodeGen+-path%3Aclang+-path%3Allvm-project+-filename%3Acompiler_attributes.h+-filename%3Aattr-disable-sanitizer-instrumentation.c&type=Code).

Other interesting information:
https://sourcegraph.com/search?q=context:global+disable_sanitizer_instrumentation+-file:.*test.*&patternType=standard
https://sourcegraph.com/search?q=context:global+no_sanitize%28%22all%22%29+-file:.*test.*&patternType=standard

Sorry for the bad news, but yeah, I think I'd prefer not to add no_sanitize("all") for globals. Definitely willing to change my mind though if someone comes along with a compelling reason why they need the mega-hammer, but I really think disablement is a decision that should be done on a per-sanitizer basis.

I don't have a horse in this race, but if I had a time machine, I would try to get us to a world where disable_sanitizer_instrumentation was always spelled no_sanitizer("all") because that fits with the other no_sanitizer arguments nicely without needing to introduce an entirely separate attribute to control sanitization behavior. disable_sanitizer_instrumentation came from an era where we introduced no_sanitize_address, no_sanitize_memory, and so on.

Marking as requesting changes so we don't accidentally land this while still under discussion.

[abrachet]

I'm fine abandoning this after discussion. I've created D133117 so that we at least get a better diagnostic message :)

clang/docs/ReleaseNotes.rst

- Added support for ``__attribute__((guard(nocf)))`` and C++-style
   when using the MSVC environment. This is to support enabling Windows Control
   Flow Guard checks with the ability to disable them for specific functions when
   using the MinGW environment. This attribute is only available for Windows
   targets.
 
 - Introduced a new function attribute ``__attribute__((nouwtable))`` to suppress
   LLVM IR ``uwtable`` function attribute.
 
+- ``__attribute__((no_sanitize("all")))`` now works on global variables. This
+  previously was only supported on functions.
+
 Windows Support
 ---------------
 
 AIX Support
 -----------
 * When using `-shared`, the clang driver now invokes llvm-nm to create an
   export list if the user doesn't specify one via linker flag or pass an
   alternative export control option.

clang/docs/UsersManual.rst

**-f[no-]sanitize=check1,check2,...**
    -  ``-fsanitize=dataflow``: :doc:`DataFlowSanitizer`, a general data
       flow analysis.
    -  ``-fsanitize=cfi``: :doc:`control flow integrity <ControlFlowIntegrity>`
       checks. Requires ``-flto``.
    -  ``-fsanitize=kcfi``: kernel indirect call forward-edge control flow
       integrity.
    -  ``-fsanitize=safe-stack``: :doc:`safe stack <SafeStack>`
       protection against stack-based memory corruption errors.
+   -  ``-fno-sanitize=all``: Disable all previously specified sanitizers.
 
    There are more fine-grained checks available: see
    the :ref:`list <ubsan-checks>` of specific kinds of
    undefined behavior that can be detected and the :ref:`list <cfi-schemes>`
    of control flow integrity schemes.
 
    The ``-fsanitize=`` argument must also be provided when linking, in
    order to link to the appropriate runtime library.

clang/lib/Sema/SemaDeclAttr.cpp

 static bool isGlobalVar(const Decl *D) {
   if (const auto *S = dyn_cast<VarDecl>(D))
     return S->hasGlobalStorage();
   return false;
 }
 
 static bool isSanitizerAttributeAllowedOnGlobals(StringRef Sanitizer) {
   return Sanitizer == "address" || Sanitizer == "hwaddress" ||
-         Sanitizer == "memtag";
+         Sanitizer == "memtag" || Sanitizer == "all";
 }
 
 static void handleNoSanitizeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   if (!AL.checkAtLeastNumArgs(S, 1))
     return;
 
   std::vector<StringRef> Sanitizers;
 

clang/test/CodeGen/asan-globals.cpp

 // RUN: echo "int extra_global;" > %t.extra-source.cpp
 // RUN: echo "global:*ignorelisted_global*" > %t.ignorelist
 // RUN: %clang_cc1 -include %t.extra-source.cpp -fsanitize=address -fsanitize-ignorelist=%t.ignorelist -emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,GLOBS,ASAN
 // RUN: %clang_cc1 -include %t.extra-source.cpp -fsanitize=kernel-address -fsanitize-ignorelist=%t.ignorelist -emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,GLOBS,KASAN
 // The ignorelist file uses regexps, so Windows path backslashes.
 // RUN: echo "src:%s" | sed -e 's/\\/\\\\/g' > %t.ignorelist-src
 // RUN: %clang_cc1 -include %t.extra-source.cpp -fsanitize=address -fsanitize-ignorelist=%t.ignorelist-src -emit-llvm -o - %s | FileCheck %s --check-prefix=IGNORELIST-SRC
 // RUN: %clang_cc1 -include %t.extra-source.cpp -fsanitize=kernel-address -fsanitize-ignorelist=%t.ignorelist-src -emit-llvm -o - %s | FileCheck %s --check-prefix=IGNORELIST-SRC
 
 int global;
 int dyn_init_global = global;
 int __attribute__((no_sanitize("address"))) attributed_global;
 int __attribute__((disable_sanitizer_instrumentation)) disable_instrumentation_global;
+int __attribute__((no_sanitize("all"))) no_sanitize_all_global;
 int ignorelisted_global;
 extern int __attribute__((no_sanitize("address"))) external_global;
 
 int __attribute__((section("__DATA, __common"))) sectioned_global; // KASAN - ignore globals in a section
 extern "C" {
 int __special_global; // KASAN - ignore globals with __-prefix
 }
 
 void func() {
   static int static_var = 0;
   const char *literal = "Hello, world!";
   external_global = 1;
 }
 
 // GLOBS:     @{{.*}}extra_global{{.*}} ={{.*}} global
 // GLOBS-NOT: no_sanitize_address
 // GLOBS:     @{{.*}}global{{.*}} ={{.*}} global
 // GLOBS-NOT: no_sanitize_address
 // GLOBS:     @{{.*}}dyn_init_global{{.*}} ={{.*}} global {{.*}}, sanitize_address_dyninit
 // GLOBS-NOT: no_sanitize_address
 
 // GLOBS:     @{{.*}}attributed_global{{.*}} ={{.*}} global {{.*}} no_sanitize_address
 // GLOBS:     @{{.*}}disable_instrumentation_global{{.*}} ={{.*}} global {{.*}} no_sanitize_address
+// GLOBS:     @{{.*}}no_sanitize_all_global{{.*}} ={{.*}} global {{.*}} no_sanitize_address
 // GLOBS:     @{{.*}}ignorelisted_global{{.*}} ={{.*}} global {{.*}} no_sanitize_address
 
 // ASAN:     @{{.*}}sectioned_global{{.*}} ={{.*}} global { i32, [28 x i8] }{{.*}}, align 32
 // ASAN-NOT: no_sanitize_address
 // ASAN:     @{{.*}}__special_global{{.*}} ={{.*}} global { i32, [28 x i8] }{{.*}}, align 32
 // ASAN-NOT: no_sanitize_address
 
 /// Note: No attribute is added by the IR pass, but the type didn't change, so

 // UWTABLE: ![[#]] = !{i32 7, !"uwtable", i32 2}
 
 // IGNORELIST-SRC:     @{{.*}}extra_global{{.*}} ={{.*}} global
 // IGNORELIST-SRC-NOT: no_sanitize_address
 // IGNORELIST-SRC:     @{{.*}}global{{.*}} ={{.*}} global {{.*}} no_sanitize_address
 // IGNORELIST-SRC:     @{{.*}}dyn_init_global{{.*}} ={{.*}} global {{.*}} no_sanitize_address
 // IGNORELIST-SRC:     @{{.*}}attributed_global{{.*}} ={{.*}} global {{.*}} no_sanitize_address
 // IGNORELIST-SRC:     @{{.*}}disable_instrumentation_global{{.*}} ={{.*}} global {{.*}} no_sanitize_address
+// IGNORELIST-SRC:     @{{.*}}no_sanitize_all_global{{.*}} ={{.*}} global {{.*}} no_sanitize_address
 // IGNORELIST-SRC:     @{{.*}}ignorelisted_global{{.*}} ={{.*}} global {{.*}} no_sanitize_address
 // IGNORELIST-SRC:     @{{.*}}sectioned_global{{.*}} ={{.*}} global {{.*}} no_sanitize_address
 // IGNORELIST-SRC:     @{{.*}}__special_global{{.*}} ={{.*}} global {{.*}} no_sanitize_address
 // IGNORELIST-SRC:     @{{.*}}static_var{{.*}} ={{.*}} global {{.*}} no_sanitize_address
 // IGNORELIST-SRC:     @{{.*}} ={{.*}} c"Hello, world!\00"{{.*}} no_sanitize_address
 // IGNORELIST-SRC:     @{{.*}}external_global{{.*}} ={{.*}} no_sanitize_address

clang/test/CodeGen/hwasan-globals.cpp

 // REQUIRES: aarch64-registered-target
 
 // RUN: %clang_cc1 -include %S/Inputs/sanitizer-extra-source.cpp \
 // RUN:   -fsanitize-ignorelist=%S/Inputs/sanitizer-ignorelist-global.txt \
 // RUN:   -fsanitize=hwaddress -emit-llvm -triple aarch64-linux-android31 -o -\
 // RUN:    %s | FileCheck %s
 
 // RUN: %clang_cc1 -include %S/Inputs/sanitizer-extra-source.cpp \
 // RUN:   -fsanitize-ignorelist=%S/Inputs/sanitizer-ignorelist-src.txt \
 // RUN:   -fsanitize=hwaddress -emit-llvm -triple aarch64-linux-android31 -o -\
 // RUN:   %s | FileCheck %s --check-prefix=IGNORELIST
 
 int global;
 int __attribute__((no_sanitize("hwaddress"))) attributed_global;
 int __attribute__((disable_sanitizer_instrumentation)) disable_instrumentation_global;
+int __attribute__((no_sanitize("all"))) no_sanitize_all_global;
 int ignorelisted_global;
 extern int __attribute__((no_sanitize("hwaddress"))) external_global;
 
 void func() {
   static int static_var = 0;
   const char *literal = "Hello, world!";
   external_global = 1;
 }
 
 // CHECK: @{{.*}}attributed_global{{.*}} ={{.*}} global {{.*}}, no_sanitize_hwaddress
 // CHECK: @{{.*}}disable_instrumentation_global{{.*}} ={{.*}} global {{.*}}, no_sanitize_hwaddress
+// CHECK: @{{.*}}no_sanitize_all_global{{.*}} ={{.*}} global {{.*}}, no_sanitize_hwaddress
 // CHECK: @{{.*}}ignorelisted_global{{.*}} ={{.*}} global {{.*}}, no_sanitize_hwaddress
 // CHECK: @{{.*}}external_global{{.*}} ={{.*}}, no_sanitize_hwaddress
 // CHECK: @{{.*}}extra_global{{.*}}.hwasan{{.*}} =
 // CHECK: @{{.*}}global{{.*}}.hwasan{{.*}} =
 // CHECK: @{{.*}}static_var{{.*}}.hwasan{{.*}} =
 // CHECK: @{{.*}}.hwasan{{.*}} = {{.*}} c"Hello, world!\00"
 
 // IGNORELIST: @{{.*}}global{{.*}} ={{.*}} global {{.*}}, no_sanitize_hwaddress
 // IGNORELIST: @{{.*}}attributed_global{{.*}} ={{.*}} global {{.*}}, no_sanitize_hwaddress
 // IGNORELIST: @{{.*}}disable_instrumentation_global{{.*}} ={{.*}} global {{.*}}, no_sanitize_hwaddress
+// IGNORELIST: @{{.*}}no_sanitize_all_global{{.*}} ={{.*}} global {{.*}}, no_sanitize_hwaddress
 // IGNORELIST: @{{.*}}ignorelisted_globa{{.*}} ={{.*}} global {{.*}}, no_sanitize_hwaddress
 // IGNORELIST: @{{.*}}static_var{{.*}} ={{.*}} global {{.*}}, no_sanitize_hwaddress
 // IGNORELIST: @{{.*}} = {{.*}} c"Hello, world!\00"{{.*}}, no_sanitize_hwaddress
 // IGNORELIST: @{{.*}}external_global{{.*}} ={{.*}}, no_sanitize_hwaddress
 // IGNORELIST: @{{.*}}extra_global{{.*}}.hwasan{{.*}} =

clang/test/CodeGen/memtag-globals.cpp

 // RUN: %clang_cc1 -include %S/Inputs/sanitizer-extra-source.cpp \
 // RUN:   -fsanitize-ignorelist=%S/Inputs/sanitizer-ignorelist-global.txt \
 // RUN:   -fsanitize=memtag-globals -emit-llvm -o - %s | FileCheck %s
 
 // RUN: %clang_cc1 -include %S/Inputs/sanitizer-extra-source.cpp \
 // RUN:   -fsanitize-ignorelist=%S/Inputs/sanitizer-ignorelist-src.txt \
 // RUN:   -fsanitize=memtag-globals -emit-llvm -o - %s | \
 // RUN:   FileCheck %s --check-prefix=IGNORELIST
 
 int global;
 int __attribute__((no_sanitize("memtag"))) attributed_global;
 int __attribute__((disable_sanitizer_instrumentation)) disable_instrumentation_global;
+int __attribute__((no_sanitize("all"))) no_sanitize_all_global;
 int ignorelisted_global;
 extern int external_global;
 
 void func() {
   static int static_var = 0;
   const char *literal = "Hello, world!";
   external_global = 1;
 }
 
 // CHECK: @{{.*}}extra_global{{.*}} ={{.*}} sanitize_memtag
 // CHECK: @{{.*}}global{{.*}} ={{.*}} sanitize_memtag
 
 // CHECK:     @{{.*}}attributed_global{{.*}} =
 // CHECK-NOT: sanitize_memtag
 // CHECK:     @{{.*}}disable_instrumentation_global{{.*}} =
 // CHECK-NOT: sanitize_memtag
+// CHECK:     @{{.*}}no_sanitize_all_global{{.*}} =
+// CHECK-NOT: sanitize_memtag
 // CHECK:     @{{.*}}ignorelisted_global{{.*}} =
 // CHECK-NOT: sanitize_memtag
 
 // CHECK: @{{.*}}static_var{{.*}} ={{.*}} sanitize_memtag
 // CHECK: @{{.*}} = {{.*}} c"Hello, world!\00"{{.*}} sanitize_memtag
 // CHECK: @{{.*}}external_global{{.*}} ={{.*}} sanitize_memtag
 
 // IGNORELIST: @{{.*}}extra_global{{.*}} ={{.*}} sanitize_memtag
 
 // IGNORELIST:     @{{.*}}global{{.*}} =
 // IGNORELIST-NOT: sanitize_memtag
 // IGNORELIST:     @{{.*}}attributed_global{{.*}} =
 // IGNORELIST-NOT: sanitize_memtag
 // IGNORELIST:     @{{.*}}disable_instrumentation_global{{.*}} =
 // IGNORELIST-NOT: sanitize_memtag
+// IGNORELIST:     @{{.*}}no_sanitize_all_global{{.*}} =
+// IGNORELIST-NOT: sanitize_memtag
 // IGNORELIST:     @{{.*}}ignorelisted_globa{{.*}} =
 // IGNORELIST-NOT: sanitize_memtag
 // IGNORELIST:     @{{.*}}static_var{{.*}} =
 // IGNORELIST-NOT: sanitize_memtag
 // IGNORELIST:     @{{.*}} = {{.*}} c"Hello, world!\00"{{.*}}
 // IGNORELIST-NOT: sanitize_memtag
 // IGNORELIST:     @{{.*}}external_global{{.*}} =
 // IGNORELIST-NOT: sanitize_memtag

clang/test/CodeGen/no-sanitize.cpp

+// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,ASAN
+// RUN: %clang_cc1 -fsanitize=thread -emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,TSAN
+
+__attribute__((no_sanitize("all"))) int a;
+__attribute__((no_sanitize("all"))) void b() {}
+
+// ASAN: @a = global i32 0, no_sanitize_address, align 4
+// TSan doesn't instrument global variables, test that no_sanitize("all") does
+// nothing in this case.
+// TSAN: @a = global i32 0, align 4
+// CHECK: define dso_local void @_Z1bv() #[[#ATTR:]] {
+// ASAN-NOT: attributes #[[#ATTR]]] = {{{.*}}sanitize_address{{.*}}}
+// TSAN-NOT: attributes #[[#ATTR]]] = {{{.*}}sanitize_thread{{.*}}}