Introduce convenience macro ASAN_NO_INSTR_PTR.

[delcypher]

Summary

This macro makes it convenient to annotate pointer types in source code
to avoid ASan instrumentation.

Previously __attribute__((address_space(1))) had to be applied to
pointer types to avoid instrumentation. However, this leaks
implementation details and is cumbersome to write.

The intended use case here is to annotate accesses to regions of memory
on Darwin that are outside of normal user memory. These regions do not
have a corresponding shadow memory and so when instrumentation tries to
look to access the shadow bytes it causes non-deterministic behaviour.

A test case is included to test various aspects of the macro.

rdar://problem/70234898

[yln]

Previously attribute((address_space(1))) had to be applied to pointer types to avoid instrumentation.

I wasn't aware of this. Is this a principled way to tell ASan to skip instrumentation of a local var, or is it used because "it works" (but it depends on implementation details)?
Note that I don't know the actual meaning/semantics of address_space.

Would generalizing __attribute__((no_sanitize("address"))) to also apply to local vars (of pointer type) be a cleaner way?
https://clang.llvm.org/docs/AddressSanitizer.html#disabling-instrumentation-with-attribute-no-sanitize-address

[delcypher]

In D89344#2328787, @yln wrote:

Previously attribute((address_space(1))) had to be applied to pointer types to avoid instrumentation.

I wasn't aware of this. Is this a principled way to tell ASan to skip instrumentation of a local var, or is it used because "it works" (but it depends on implementation details)?
Note that I don't know the actual meaning/semantics of address_space.

My understanding is that attribute is used "because it works", not because it's principled in anyway.

IIUC correctly the address_space attribute maps to LLVM IR's notion of address space. Here's a snippet from the LLVM language reference

A global variable may be declared to reside in a target-specific numbered address space. For targets that support them, address spaces may affect how optimizations are performed and/or what target instructions are used to access the variable. The default address space is zero. The address space qualifier must precede any other attributes.

This suggests that there might be downsides to using this attribute (e.g. it might inhibit optimizations)

Would generalizing __attribute__((no_sanitize("address"))) to also apply to local vars (of pointer type) be a cleaner way?
https://clang.llvm.org/docs/AddressSanitizer.html#disabling-instrumentation-with-attribute-no-sanitize-address

I don't think that will work because I think no_sanitize is a function level attribute.

If there is a more principled attribute then I'm happy to change this patch to use it. If such an attribute doesn't exist then I do not think that this patch should be blocked on having a more principled attribute . The whole point of this patch is to introduce a macro that hides the current implementation details. Having the macro introduced in this patch means that we can change the underlying implementation details and users of the macro will not need to care.

[delcypher]

In D89344#2328898, @delcypher wrote:

In D89344#2328787, @yln wrote:

Previously attribute((address_space(1))) had to be applied to pointer types to avoid instrumentation.

I wasn't aware of this. Is this a principled way to tell ASan to skip instrumentation of a local var, or is it used because "it works" (but it depends on implementation details)?
Note that I don't know the actual meaning/semantics of address_space.

My understanding is that attribute is used "because it works", not because it's principled in anyway.

IIUC correctly the address_space attribute maps to LLVM IR's notion of address space. Here's a snippet from the LLVM language reference

A global variable may be declared to reside in a target-specific numbered address space. For targets that support them, address spaces may affect how optimizations are performed and/or what target instructions are used to access the variable. The default address space is zero. The address space qualifier must precede any other attributes.

This suggests that there might be downsides to using this attribute (e.g. it might inhibit optimizations)

Would generalizing __attribute__((no_sanitize("address"))) to also apply to local vars (of pointer type) be a cleaner way?
https://clang.llvm.org/docs/AddressSanitizer.html#disabling-instrumentation-with-attribute-no-sanitize-address

I don't think that will work because I think no_sanitize is a function level attribute.

If there is a more principled attribute then I'm happy to change this patch to use it. If such an attribute doesn't exist then I do not think that this patch should be blocked on having a more principled attribute . The whole point of this patch is to introduce a macro that hides the current implementation details. Having the macro introduced in this patch means that we can change the underlying implementation details and users of the macro will not need to care.

@yln I tried using the attribute on a local variable. It's not supported.

Volumes/data/dev/llvm/llvm.org/master/llvm/compiler-rt/test/asan/TestCases/no_instrument_pointer.cpp:35:3: error: 'no_sanitize' attribute only applies to functions, Objective-C methods, and global variables
  ASAN_NO_INSTR_PTR(int *)
  ^
/Volumes/data/dev/llvm/llvm.org/master/builds/RelWithDebInfo/lib/clang/12.0.0/include/sanitizer/asan_interface.h:343:54: note: expanded from macro 'ASAN_NO_INSTR_PTR'
#      define ASAN_NO_INSTR_PTR(TYPE) __attribute__((no_sanitize("address"))) TYPE
                                                     ^
/Volumes/data/dev/llvm/llvm.org/master/llvm/compiler-rt/test/asan/TestCases/no_instrument_pointer.cpp:36:10: warning: 'no_sanitize' attribute ignored when parsing type [-Wignored-attributes]
  ptr = (ASAN_NO_INSTR_PTR(int *))source();
         ^~~~~~~~~~~~~~~~~~~~~~~~
/Volumes/data/dev/llvm/llvm.org/master/builds/RelWithDebInfo/lib/clang/12.0.0/include/sanitizer/asan_interface.h:343:54: note: expanded from macro 'ASAN_NO_INSTR_PTR'
#      define ASAN_NO_INSTR_PTR(TYPE) __attribute__((no_sanitize("address"))) TYPE
                                                     ^~~~~~~~~~~~~~~~~~~~~~
1 warning and 1 error generated.

[vitalybuka]

I recommend abandon interface change, add a test for llvm/lib/Transforms/Instrumentation if there is no any yet, and document this trick in https://clang.llvm.org/docs/AddressSanitizer.html

compiler-rt/include/sanitizer/asan_interface.h

328

I don't think it should be here
we don't put here defines for attribute((no_sanitize("address")))

Also this header is interface for runtime, and define controls compiler side.

[vitalybuka]

Ubsan ignores volatile vars https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
maybe better to make sure that asan does the same here as well?

[delcypher]

In D89344#2329256, @vitalybuka wrote:

I recommend abandon interface change, add a test for llvm/lib/Transforms/Instrumentation if there is no any yet, and document this trick in https://clang.llvm.org/docs/AddressSanitizer.html

I don't think comparing __attribute__((address_space(1))) with attribute((no_sanitize("address"))) is a fair comparison. They are very different.

In the case of no_sanitize("address"), this attribute was built for the sanitizers and it is actually clear that this attribute disables ASan in someway. This seems like an acceptable (it's somewhat clear what it does) interface to use and hence why we have documentation telling users that they can use this.

We cannot say the same about address_space(1). It was not built for the sanitizers and it is not clear at all that this attribute has anything to with the sanitizers at all. The fact that changing the address space on a pointer type opts loads/stores out of ASan is an implementation detail that I don't think we should be exposing to users.
Therefore I do not see asking users to use address_space(1) directly as an acceptable solution.

compiler-rt/include/sanitizer/asan_interface.h

328

Also this header is interface for runtime, and define controls compiler side.

This header already provides defines for use at compile time so I don't agree with this objection.

Alternatively we could introduce a new header file (e.g. santizer/asan_util.h) for convenience macros that are used at compile time.

[delcypher]

In D89344#2329258, @vitalybuka wrote:

Ubsan ignores volatile vars https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
maybe better to make sure that asan does the same here as well?

Why would we want that behaviour for ASan? I checked and it looks like volatile pointers do get instrumented by ASan and I see no reason to change that.

[vitalybuka]

In D89344#2330706, @delcypher wrote:

In D89344#2329258, @vitalybuka wrote:

Ubsan ignores volatile vars https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
maybe better to make sure that asan does the same here as well?

Why would we want that behaviour for ASan? I checked and it looks like volatile pointers do get instrumented by ASan and I see no reason to change that.

To have consistent way to disable instrumentation on variable.

compiler-rt/include/sanitizer/asan_interface.h

328

Either this or asan_util.h are going to be used by external code which we can't see. In future it's going to be hard to remove or change anything that here so we like to keep as simple as possible and avoid adding unnecessary convenience stuff here.

Defines you mentioned exist from 2011 and probably we didn't remove them for the reason above. But I don't see recent convenience stuff here.

I don't know if it's official but I assume we expect such tools inside of clients code. @kcc ?

343

Why does it need (TYPE) as parameter?

compiler-rt/include/sanitizer/asan_interface.h

 //===-- sanitizer/asan_interface.h ------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of AddressSanitizer (ASan).

 /// Update allocation stack trace for the given allocation to the current stack
 /// trace. Returns 1 if successfull, 0 if not.
 int __asan_update_allocation_context(void* addr);
 
 #ifdef __cplusplus
 }  // extern "C"
 #endif
 
+// Macro for declaring pointer types that should not be instrumented
+// by ASan. Loads and stores on pointers of this type will not be
+// instrumented.
+//
+// \note Macro is a no-op when ASan is disabled.
+// \note This macro is only supported by Clang.
+//
+// \param TYPE A pointer type.
+//
+// Example:
+//   ASAN_NO_INSTR_PTR(int*) a = 0;
+//   int b = a[0]; // Not instrumented
+//   a[0] = 5; // Not instrumented
+//
+#ifdef __clang__
+#  if defined(__has_feature)
+#    if __has_feature(address_sanitizer)
+#      define ASAN_NO_INSTR_PTR(TYPE) __attribute__((address_space(1))) TYPE
+#    else
+#      define ASAN_NO_INSTR_PTR(TYPE) TYPE
+#    endif
+#  else
+#    error Unreachable
+#  endif
+#else
+   // Using macro on unsupported compilers should be an error
+#  ifdef __GNUC__
+#    define ASAN_NO_INSTR_PTR(TYPE) _Pragma ("GCC error \"ASAN_NO_INSTR_PTR is only supported by Clang\"")
+#  else
+     // TODO(dliew): Support other compilers. For now avoid breaking compilation.
+#    define ASAN_NO_INSTR_PTR(TYPE) TYPE
+#  endif
+#endif
+
 #endif  // SANITIZER_ASAN_INTERFACE_H

compiler-rt/test/asan/TestCases/no_instrument_pointer.cpp

+// REQUIRES: Clang
+// Check non-instrumented binary using ASAN_NO_INSTR_PTR compiles and runs.
+// RUN: %clangxx -O0 %s -DUSE_ASAN_MACRO -o %t.no_asan
+// RUN: %run %t.no_asan
+
+// Check instrumented binary without ASAN_NO_INSTR_PTR detects the access to poisoned memory.
+// RUN: %clangxx_asan -O0 %s -o %t.asan_find_poison
+// RUN: not %run %t.asan_find_poison 2>&1 | FileCheck -check-prefix=CHECK-POISON %s
+
+// Check instrumented binary with ASAN_NO_INSTR_PTR does not detect access to poisoned memory.
+// RUN: %clangxx_asan -O0 -DUSE_ASAN_MACRO %s -o %t.asan_no_find_poison
+// RUN: %run %t.asan_no_find_poison
+#include "sanitizer/asan_interface.h"
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+
+#ifndef ASAN_NO_INSTR_PTR
+#error ASAN_NO_INSTR_PTR macro is missing
+#endif
+
+int *source() {
+  unsigned size = sizeof(int) * 32;
+  int *a = (int *)malloc(size);
+  memset(a, 0, size);
+#if __has_feature(address_sanitizer)
+  __asan_poison_memory_region(a, size);
+#endif
+  return a;
+}
+
+int main() {
+#ifdef USE_ASAN_MACRO
+  ASAN_NO_INSTR_PTR(int *)
+  ptr = (ASAN_NO_INSTR_PTR(int *))source();
+#else
+  int *ptr = source();
+#endif
+
+  // CHECK-POISON: AddressSanitizer: use-after-poison
+  printf("READ: %d\n", ptr[0]); // Read
+  ptr[0] = 5;                   // Write
+  free((int *)ptr);
+  return 0;
+}

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

bool AddressSanitizer::isInterestingAlloca(const AllocaInst &AI) {
   ProcessedAllocas[&AI] = IsInteresting;
   return IsInteresting;
 }
 
 bool AddressSanitizer::ignoreAccess(Value *Ptr) {
   // Do not instrument acesses from different address spaces; we cannot deal
   // with them.
   Type *PtrTy = cast<PointerType>(Ptr->getType()->getScalarType());
+  // If this condition is ever changed the `ASAN_NO_INSTR_PTR` macro
+  // in `compiler-rt/include/sanitizer/asan_interface.h` must be updated.
   if (PtrTy->getPointerAddressSpace() != 0)
     return true;
 
   // Ignore swifterror addresses.
   // swifterror memory addresses are mem2reg promoted by instruction
   // selection. As such they cannot have regular uses like an instrumentation
   // function and it makes no sense to track them as memory.
   if (Ptr->isSwiftError())