[Support] Bugprone usage of __has_feature(address_sanitizer) in Allocator

[Dinistro]

tl;dr:
llvm/Support/Allocator.h transitively uses __has_feature(address_sanitizer) to decide if it uses __msan_* and __asan_* markers and if it should allocate a red zone. This is very problematic when this header is pulled into an LLVM compilation unit but is also made available through some LLVM template to down-stream users.

Extended bug explanation

We observe an ASAN crash in the following setup:
We build LLVM + MLIR without sanitizers enabled, but have a downstream project that we build with ASAN. Due to relying on MLIR's StorageUniquer, which is a templated implementation in a header, we pull in a copy of BumpPtrAllocatorImpl::Allocate. This function changes behavior, depending on ASAN being enabled or not:

llvm-project/llvm/include/llvm/Support/Allocator.h

Lines 156 to 159 in 0e9a102

	#if LLVM_ADDRESS_SANITIZER_BUILD
	// Add trailing bytes as a "red zone" under ASan.
	SizeToAllocate += RedZoneSize;
	#endif

Additionally, this code contains multiple __msan_* and __asan_* usages.

Unfortunately, other parts of MLIR's StorageUniquer are implemented in a .cpp file, which result in it being compiled without ASAN. This is problematic, due the a non-ASAN build replacing all __msan_* and __asan_* functions with empty macros.

llvm-project/llvm/include/llvm/Support/Compiler.h

Lines 441 to 461 in 2d98d76

	#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)
	# define LLVM_ADDRESS_SANITIZER_BUILD 1
	#if __has_include(<sanitizer/asan_interface.h>)
	# include <sanitizer/asan_interface.h>
	#else
	// These declarations exist to support ASan with MSVC. If MSVC eventually ships
	// asan_interface.h in their headers, then we can remove this.
	#ifdef __cplusplus
	extern "C" {
	#endif
	void __asan_poison_memory_region(void const volatile *addr, size_t size);
	void __asan_unpoison_memory_region(void const volatile *addr, size_t size);
	#ifdef __cplusplus
	} // extern "C"
	#endif
	#endif
	#else
	# define LLVM_ADDRESS_SANITIZER_BUILD 0
	# define __asan_poison_memory_region(p, size)
	# define __asan_unpoison_memory_region(p, size)
	#endif

Therefore, some of the poison marking is compiled in, while unpoison markings are omitted. This can therefore lead to ASAN violations that are very tricky to hunt down.

CC @ibookstein

[llvmbot]

@llvm/issue-subscribers-mlir-core

Author: Christian Ulmann (Dinistro)

tl;dr: `llvm/Support/Allocator.h` transitively uses `__has_feature(address_sanitizer)` to decide if it uses `__msan_*` and `__asan_*` markers and if it should allocate a red zone. This is very problematic when this header is pulled into an LLVM compilation unit but is also made available through some LLVM template to downstream users.

Extended bug explanation

We observed an ASAN crash in the following setup:
We build LLVM + MLIR without sanitizers enabled, but have a downstream project that is build with ASAN. Due to relying on MLIR's StorageUniquer, which is a templated implementation in a header, we pull in a copy of BumpPtrAllocatorImpl::Allocate. This function changes behavior, depending on ASAN being enabled or not:

llvm-project/llvm/include/llvm/Support/Allocator.h

Lines 156 to 159 in 0e9a102

	#if LLVM_ADDRESS_SANITIZER_BUILD
	// Add trailing bytes as a "red zone" under ASan.
	SizeToAllocate += RedZoneSize;
	#endif

Additionally, this code contains multiple __msan_* and __asan_* usages.

Unfortunately, other parts of MLIR's StorageUniquer are implemented in a .cpp file, which result in it being compiled without ASAN. This is problematic, due the a non-ASAN build replacing all __msan_* and __asan_* function with empty macros.

llvm-project/llvm/include/llvm/Support/Compiler.h

Lines 441 to 461 in 2d98d76

	#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)
	# define LLVM_ADDRESS_SANITIZER_BUILD 1
	#if __has_include(<sanitizer/asan_interface.h>)
	# include <sanitizer/asan_interface.h>
	#else
	// These declarations exist to support ASan with MSVC. If MSVC eventually ships
	// asan_interface.h in their headers, then we can remove this.
	#ifdef __cplusplus
	extern "C" {
	#endif
	void __asan_poison_memory_region(void const volatile *addr, size_t size);
	void __asan_unpoison_memory_region(void const volatile *addr, size_t size);
	#ifdef __cplusplus
	} // extern "C"
	#endif
	#endif
	#else
	# define LLVM_ADDRESS_SANITIZER_BUILD 0
	# define __asan_poison_memory_region(p, size)
	# define __asan_unpoison_memory_region(p, size)
	#endif

Therefore, some of the poison marking is compiled in, while unpoison markings are omitted. This can therefore lead to ASAN violations that very tricky to hunt down.

CC @ibookstein

[llvmbot]

@llvm/issue-subscribers-bug

Author: Christian Ulmann (Dinistro)

tl;dr: `llvm/Support/Allocator.h` transitively uses `__has_feature(address_sanitizer)` to decide if it uses `__msan_*` and `__asan_*` markers and if it should allocate a red zone. This is very problematic when this header is pulled into an LLVM compilation unit but is also made available through some LLVM template to downstream users.

Extended bug explanation

We observed an ASAN crash in the following setup:
We build LLVM + MLIR without sanitizers enabled, but have a downstream project that is build with ASAN. Due to relying on MLIR's StorageUniquer, which is a templated implementation in a header, we pull in a copy of BumpPtrAllocatorImpl::Allocate. This function changes behavior, depending on ASAN being enabled or not:

llvm-project/llvm/include/llvm/Support/Allocator.h

Lines 156 to 159 in 0e9a102

	#if LLVM_ADDRESS_SANITIZER_BUILD
	// Add trailing bytes as a "red zone" under ASan.
	SizeToAllocate += RedZoneSize;
	#endif

Additionally, this code contains multiple __msan_* and __asan_* usages.

Unfortunately, other parts of MLIR's StorageUniquer are implemented in a .cpp file, which result in it being compiled without ASAN. This is problematic, due the a non-ASAN build replacing all __msan_* and __asan_* function with empty macros.

llvm-project/llvm/include/llvm/Support/Compiler.h

Lines 441 to 461 in 2d98d76

	#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)
	# define LLVM_ADDRESS_SANITIZER_BUILD 1
	#if __has_include(<sanitizer/asan_interface.h>)
	# include <sanitizer/asan_interface.h>
	#else
	// These declarations exist to support ASan with MSVC. If MSVC eventually ships
	// asan_interface.h in their headers, then we can remove this.
	#ifdef __cplusplus
	extern "C" {
	#endif
	void __asan_poison_memory_region(void const volatile *addr, size_t size);
	void __asan_unpoison_memory_region(void const volatile *addr, size_t size);
	#ifdef __cplusplus
	} // extern "C"
	#endif
	#endif
	#else
	# define LLVM_ADDRESS_SANITIZER_BUILD 0
	# define __asan_poison_memory_region(p, size)
	# define __asan_unpoison_memory_region(p, size)
	#endif

Therefore, some of the poison marking is compiled in, while unpoison markings are omitted. This can therefore lead to ASAN violations that very tricky to hunt down.

CC @ibookstein

[Dinistro]

A potential mitigation for this issue would be to generate a header file that defines these macros depending on the build configuration of LLVM. While this would work for the our bug, I'm unsure if there are downsides for direct down-stream users of Allocator.h in that case.

[nextsilicon-itay-bookstein]

This is essentially an ODR violation, since the user and the library disagree on the sanitizer settings in a way which affects the definitions of the header's contents. I think it would be ideal that sanitized LLVM wouldn't impose sanitization on client code, and more importantly, vice versa.

To that end, if LLVM_ADDRESS_SANITIZER_BUILD becomes a build setting exposed to the user (like other -D defines, RTTI, exceptions), then there's no ODR violation anymore. There's also a question of whether the code in the headers needs to explicitly opt out of sanitizers in that case (via attribute no_sanitize) so that when it's compiled in the context of the user's code there are no discrepancies.

[Dinistro]

LLVM already has an LLVM_USE_SANITIZER option, maybe that could be used to derive the value of LLVM_ADDRESS_SANITIZER_BUILD. Given that client code should know what kind of LLVM distribution they work with, this could be set accordingly by invoking the necessary CMake machinery.

[MaskRay]

I believe we just don't support asan-instrumented LLVM with uninstrumented user.

The proposed patch tries to support the configuration. If we decide to so, the setting probably should be recorded in llvm/include/llvm/Config/llvm-config.h.cmake.

[Dinistro]

Thanks for the feedback.

I believe we just don't support asan-instrumented LLVM with uninstrumented user.

I was not fully aware of this, tbh. The main issue we are phasing is the other way around, i.e., ODR violations in the case of using a uninstrumented LLVM with an instrumented user.

The proposed patch tries to support the configuration. If we decide to so, the setting probably should be recorded in llvm/include/llvm/Config/llvm-config.h.cmake.

Either we go down this road and enable a full scale support of this type of solution, or we find an alternative solution that just allows the other case. So far, I'm uncertain how we could implement something similar to my draft which does not allow the user to somehow set a flag/option which would lead to ODR violations.

We could, ofc., just assume that people will not set the LLVM_ADDRESS_SANITIZER_BUILD option, but it's sheer existence might already tempt this bug prone usage. Let me know what you believe is a reasonable approach for this.