[compiler-rt][hwasan] Remove __sanitizer allocation functions from hwasan interface

[leonardchan]

Summary

These functions should not be externally used. We also do not need them internally for Fuchsia.

[mcgrathr]

I don't think this is warranted. There should not be any references to such things when building for Fuchsia.
This looks like it's intended to satisfy one reference in dead code in hwasan_interceptors.cpp.
I think the right solution is just to refactor the hwasan code so that the dead code is not compiled for Fuchsia at all.

[leonardchan]

In D99381#2651896, @mcgrathr wrote:

I don't think this is warranted. There should not be any references to such things when building for Fuchsia.
This looks like it's intended to satisfy one reference in dead code in hwasan_interceptors.cpp.
I think the right solution is just to refactor the hwasan code so that the dead code is not compiled for Fuchsia at all.

To add more context, this patch was meant to resolve this missing type error:

[27/32] Building CXX object compiler-rt/lib/hwasan/CMakeFiles/RTHwasan_dynamic.aarch64.dir/hwasan.cpp.obj
FAILED: compiler-rt/lib/hwasan/CMakeFiles/RTHwasan_dynamic.aarch64.dir/hwasan.cpp.obj 
/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-4-master-fuchsia-toolchain/./bin/clang++ --target=aarch64-unknown-fuchsia --sysroot=/usr/local/google/home/leonardchan/sdk/garnet/arch/arm64/sysroot -DHWASAN_REPLACE_OPERATORS_NEW_AND_DELETE=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-4/compiler-rt/lib/hwasan/.. --target=aarch64-unknown-fuchsia -I/usr/local/google/home/leonardchan/sdk/garnet/pkg/fdio/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -ffunction-sections -fdata-sections -ffile-prefix-map=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-4-master-fuchsia-toolchain/runtimes/runtimes-aarch64-unknown-fuchsia-bins=../llvm-build-4-master-fuchsia-toolchain/runtimes/runtimes-aarch64-unknown-fuchsia-bins -ffile-prefix-map=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-4/= -no-canonical-prefixes -Wall -std=c++14 -Wno-unused-parameter -O2 -g -DNDEBUG  -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -O3 -gline-tables-only -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -nostdinc++ -fno-rtti -fPIC -ffreestanding -ftls-model=initial-exec -MD -MT compiler-rt/lib/hwasan/CMakeFiles/RTHwasan_dynamic.aarch64.dir/hwasan.cpp.obj -MF compiler-rt/lib/hwasan/CMakeFiles/RTHwasan_dynamic.aarch64.dir/hwasan.cpp.obj.d -o compiler-rt/lib/hwasan/CMakeFiles/RTHwasan_dynamic.aarch64.dir/hwasan.cpp.obj -c /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-4/compiler-rt/lib/hwasan/hwasan.cpp
In file included from /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-4/compiler-rt/lib/hwasan/hwasan.cpp:14:
In file included from /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-4/compiler-rt/lib/hwasan/hwasan.h:20:
/usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-4/compiler-rt/lib/hwasan/hwasan_interface_internal.h:204:11: error: no type named '__sanitizer_struct_mallinfo' in namespace '__hwasan'
__hwasan::__sanitizer_struct_mallinfo __sanitizer_mallinfo();
~~~~~~~~~~^

__sanitizer_struct_mallinfo is used as part of the hwasan interface:

SANITIZER_INTERFACE_ATTRIBUTE
__hwasan::__sanitizer_struct_mallinfo __sanitizer_mallinfo();

So I'm thinking unless we want to change the API, we may not want to change or remove this function. The error comes up because __sanitizer_struct_mallinfo is currently only defined if SANITIZER_LINUX || SANITIZER_MAC is true, so it felt appropriate to add a fuchsia-specific definition in sanitizer_platform_limits_fuchsia.h which can just be empty if we don't use it anyway. Would refactoring still be applicable in this case?

[phosek]

I see, it's exposed here https://github.com/llvm/llvm-project/blob/6d2d3bd0a61f5fc7fd9f61f48bc30e9ca77cc619/compiler-rt/include/sanitizer/hwasan_interface.h#L88.

[leonardchan]

*ping* @mcgrathr Given my explanation before, does it still make sense to refactor in this case?

[vitalybuka]

compiler-rt/lib/hwasan/hwasan_interceptors.cpp

28 ↗

(On Diff #333426)

we usually include them unconditionaly and .h itself has guards against unsupported platforms

[leonardchan]

we usually include them unconditionaly and .h itself has guards against unsupported platforms

@vitalybuka Did you mean something like this? Definitely simplifies the patch.

[phosek]

What I think @vitalybuka meant is keeping sanitizer_platform_limits_fuchsia.h as you had it, but including it unconditionally. Since the entire file is wrapped in #if SANITIZER_FUCHSIA ... #endif, the inclusion would be a no-op on platforms other than Fuchsia so no need to wrap the #include in #if SANITIZER_FUCHSIA as well.

[vitalybuka]

In D99381#2721619, @phosek wrote:

What I think @vitalybuka meant is keeping sanitizer_platform_limits_fuchsia.h as you had it, but including it unconditionally. Since the entire file is wrapped in #if SANITIZER_FUCHSIA ... #endif, the inclusion would be a no-op on platforms other than Fuchsia so no need to wrap the #include in #if SANITIZER_FUCHSIA as well.

Correct., Just:

#include "sanitizer_common/sanitizer_platform_limits_fuchsia.h"
#include "sanitizer_common/sanitizer_platform_limits_posix.h"

[leonardchan]

In D99381#2723563, @vitalybuka wrote:

In D99381#2721619, @phosek wrote:

What I think @vitalybuka meant is keeping sanitizer_platform_limits_fuchsia.h as you had it, but including it unconditionally. Since the entire file is wrapped in #if SANITIZER_FUCHSIA ... #endif, the inclusion would be a no-op on platforms other than Fuchsia so no need to wrap the #include in #if SANITIZER_FUCHSIA as well.

Correct., Just:

#include "sanitizer_common/sanitizer_platform_limits_fuchsia.h"
#include "sanitizer_common/sanitizer_platform_limits_posix.h"

I see. Thanks for the clarifications.

[leonardchan]

*ping*

[mcgrathr]

Frankly I don't think it makes sense to have __sanitizer_mallinfo and those other allocator functions declared in hwasan_interface_internal.h at all.
Those are neither APIs that the compiler emits nor ones that anyone else should use. They are just implementation details of the allocator interceptors.
I think a better cleanup is to move those declarations out of that file. I don't see why they need to be in any header file rather than just in hwasan_allocation_functions.cpp.

[leonardchan]

In D99381#2858599, @mcgrathr wrote:

Frankly I don't think it makes sense to have __sanitizer_mallinfo and those other allocator functions declared in hwasan_interface_internal.h at all.
Those are neither APIs that the compiler emits nor ones that anyone else should use. They are just implementation details of the allocator interceptors.
I think a better cleanup is to move those declarations out of that file. I don't see why they need to be in any header file rather than just in hwasan_allocation_functions.cpp.

I see. So something like this?

[vitalybuka]

@eugenis Why do we have them in here and in sanitizer/hwasan_interface.h ?

[vitalybuka]

In D99381#2908102, @vitalybuka wrote:

@eugenis Why do we have them in here and in sanitizer/hwasan_interface.h ?

And if we don't need them in sanitizer/hwasan_interface.h and interface_internal,
why they need SANITIZER_INTERFACE_ATTRIBUTE

I can't find any callers other than compiler-rt/test/hwasan/TestCases/sanitizer_malloc.cpp
Could we remove these functions (including implementation)?

[eugenis]

They are used here:
https://cs.android.com/android/platform/superproject/+/master:bionic/libc/bionic/malloc_common.h;l=54;drc=f3968e89cb72400951f93a2a8237ac1428d2627c

[vitalybuka]

In D99381#2911057, @eugenis wrote:

They are used here:
https://cs.android.com/android/platform/superproject/+/master:bionic/libc/bionic/malloc_common.h;l=54;drc=f3968e89cb72400951f93a2a8237ac1428d2627c

Thanks, it's preprocessor generated, so I can't find it :)

If so I guess LGTM.
But now they are in the same file, so instead of declarations, maybe just place SANITIZER_INTERFACE_ATTRIBUTE next to to the definition?

[vitalybuka]

compiler-rt/lib/hwasan/hwasan_allocation_functions.cpp

20

isn't HWASAN_WITH_INTERCEPTORS enough?

[vitalybuka]

compiler-rt/lib/hwasan/hwasan_allocation_functions.cpp

20

Ignore this. !SANITIZER_FUCHSIA is OK here.

[leonardchan]

In D99381#2911459, @vitalybuka wrote:

In D99381#2911057, @eugenis wrote:

They are used here:
https://cs.android.com/android/platform/superproject/+/master:bionic/libc/bionic/malloc_common.h;l=54;drc=f3968e89cb72400951f93a2a8237ac1428d2627c

Thanks, it's preprocessor generated, so I can't find it :)

If so I guess LGTM.
But now they are in the same file, so instead of declarations, maybe just place SANITIZER_INTERFACE_ATTRIBUTE next to to the definition?

Done. I'll take this as a review LGTM and submit tomorrow unless others have comments.

compiler-rt/lib/hwasan/hwasan_allocation_functions.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "hwasan.h"
 #include "interception/interception.h"
 #include "sanitizer_common/sanitizer_allocator_interface.h"
 #include "sanitizer_common/sanitizer_tls_get_addr.h"
 
+#if !SANITIZER_FUCHSIA
+
 using namespace __hwasan;
 
 static uptr allocated_for_dlsym;
 static const uptr kDlsymAllocPoolSize = 1024;
 static uptr alloc_memory_for_dlsym[kDlsymAllocPoolSize];
 
 static bool IsInDlsymAllocPool(const void *ptr) {
   uptr off = (uptr)ptr - (uptr)alloc_memory_for_dlsym;
   return off < sizeof(alloc_memory_for_dlsym);
 }
 
 static void *AllocateFromLocalPool(uptr size_in_bytes) {
   uptr size_in_words = RoundUpTo(size_in_bytes, kWordSize) / kWordSize;
   void *mem = (void *)&alloc_memory_for_dlsym[allocated_for_dlsym];
   allocated_for_dlsym += size_in_words;
   CHECK_LT(allocated_for_dlsym, kDlsymAllocPoolSize);
   return mem;
 }
 
+extern "C" {
+
+SANITIZER_INTERFACE_ATTRIBUTE
 int __sanitizer_posix_memalign(void **memptr, uptr alignment, uptr size) {
   GET_MALLOC_STACK_TRACE;
   CHECK_NE(memptr, 0);
   int res = hwasan_posix_memalign(memptr, alignment, size, &stack);
   return res;
 }
 
+SANITIZER_INTERFACE_ATTRIBUTE
 void *__sanitizer_memalign(uptr alignment, uptr size) {
   GET_MALLOC_STACK_TRACE;
   return hwasan_memalign(alignment, size, &stack);
 }
 
+SANITIZER_INTERFACE_ATTRIBUTE
 void *__sanitizer_aligned_alloc(uptr alignment, uptr size) {
   GET_MALLOC_STACK_TRACE;
   return hwasan_aligned_alloc(alignment, size, &stack);
 }
 
+SANITIZER_INTERFACE_ATTRIBUTE
 void *__sanitizer___libc_memalign(uptr alignment, uptr size) {
   GET_MALLOC_STACK_TRACE;
   void *ptr = hwasan_memalign(alignment, size, &stack);
   if (ptr)
     DTLS_on_libc_memalign(ptr, size);
   return ptr;
 }
 
+SANITIZER_INTERFACE_ATTRIBUTE
 void *__sanitizer_valloc(uptr size) {
   GET_MALLOC_STACK_TRACE;
   return hwasan_valloc(size, &stack);
 }
 
+SANITIZER_INTERFACE_ATTRIBUTE
 void *__sanitizer_pvalloc(uptr size) {
   GET_MALLOC_STACK_TRACE;
   return hwasan_pvalloc(size, &stack);
 }
 
+SANITIZER_INTERFACE_ATTRIBUTE
 void __sanitizer_free(void *ptr) {
   GET_MALLOC_STACK_TRACE;
   if (!ptr || UNLIKELY(IsInDlsymAllocPool(ptr)))
     return;
   hwasan_free(ptr, &stack);
 }
 
+SANITIZER_INTERFACE_ATTRIBUTE
 void __sanitizer_cfree(void *ptr) {
   GET_MALLOC_STACK_TRACE;
   if (!ptr || UNLIKELY(IsInDlsymAllocPool(ptr)))
     return;
   hwasan_free(ptr, &stack);
 }
 
+SANITIZER_INTERFACE_ATTRIBUTE
 uptr __sanitizer_malloc_usable_size(const void *ptr) {
   return __sanitizer_get_allocated_size(ptr);
 }
 
+SANITIZER_INTERFACE_ATTRIBUTE
 struct __sanitizer_struct_mallinfo __sanitizer_mallinfo() {
   __sanitizer_struct_mallinfo sret;
   internal_memset(&sret, 0, sizeof(sret));
   return sret;
 }
 
+SANITIZER_INTERFACE_ATTRIBUTE
 int __sanitizer_mallopt(int cmd, int value) { return 0; }
 
+SANITIZER_INTERFACE_ATTRIBUTE
 void __sanitizer_malloc_stats(void) {
   // FIXME: implement, but don't call REAL(malloc_stats)!
 }
 
+SANITIZER_INTERFACE_ATTRIBUTE
 void *__sanitizer_calloc(uptr nmemb, uptr size) {
   GET_MALLOC_STACK_TRACE;
   if (UNLIKELY(!hwasan_inited))
     // Hack: dlsym calls calloc before REAL(calloc) is retrieved from dlsym.
     return AllocateFromLocalPool(nmemb * size);
   return hwasan_calloc(nmemb, size, &stack);
 }
 
+SANITIZER_INTERFACE_ATTRIBUTE
 void *__sanitizer_realloc(void *ptr, uptr size) {
   GET_MALLOC_STACK_TRACE;
   if (UNLIKELY(IsInDlsymAllocPool(ptr))) {
     uptr offset = (uptr)ptr - (uptr)alloc_memory_for_dlsym;
     uptr copy_size = Min(size, kDlsymAllocPoolSize - offset);
     void *new_ptr;
     if (UNLIKELY(!hwasan_inited)) {
       new_ptr = AllocateFromLocalPool(copy_size);
     } else {
       copy_size = size;
       new_ptr = hwasan_malloc(copy_size, &stack);
     }
     internal_memcpy(new_ptr, ptr, copy_size);
     return new_ptr;
   }
   return hwasan_realloc(ptr, size, &stack);
 }
 
+SANITIZER_INTERFACE_ATTRIBUTE
 void *__sanitizer_reallocarray(void *ptr, uptr nmemb, uptr size) {
   GET_MALLOC_STACK_TRACE;
   return hwasan_reallocarray(ptr, nmemb, size, &stack);
 }
 
+SANITIZER_INTERFACE_ATTRIBUTE
 void *__sanitizer_malloc(uptr size) {
   GET_MALLOC_STACK_TRACE;
   if (UNLIKELY(!hwasan_init_is_running))
     ENSURE_HWASAN_INITED();
   if (UNLIKELY(!hwasan_inited))
     // Hack: dlsym calls malloc before REAL(malloc) is retrieved from dlsym.
     return AllocateFromLocalPool(size);
   return hwasan_malloc(size, &stack);
 }
 
+}  // extern "C"
+
 #if HWASAN_WITH_INTERCEPTORS
 #  define INTERCEPTOR_ALIAS(RET, FN, ARGS...)                                 \
     extern "C" SANITIZER_INTERFACE_ATTRIBUTE RET WRAP(FN)(ARGS)               \
         ALIAS("__sanitizer_" #FN);                                            \
     extern "C" SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE RET FN( \
         ARGS) ALIAS("__sanitizer_" #FN)
 
 INTERCEPTOR_ALIAS(int, posix_memalign, void **memptr, SIZE_T alignment,

 INTERCEPTOR_ALIAS(void *, memalign, SIZE_T alignment, SIZE_T size);
 INTERCEPTOR_ALIAS(void *, pvalloc, SIZE_T size);
 INTERCEPTOR_ALIAS(void, cfree, void *ptr);
 INTERCEPTOR_ALIAS(__sanitizer_struct_mallinfo, mallinfo);
 INTERCEPTOR_ALIAS(int, mallopt, int cmd, int value);
 INTERCEPTOR_ALIAS(void, malloc_stats, void);
 #  endif
 #endif  // #if HWASAN_WITH_INTERCEPTORS
+
+#endif  // SANITIZER_FUCHSIA

compiler-rt/lib/hwasan/hwasan_interface_internal.h

 
 SANITIZER_INTERFACE_ATTRIBUTE
 void __hwasan_thread_exit();
 
 SANITIZER_INTERFACE_ATTRIBUTE
 void __hwasan_print_memory_usage();
 
 SANITIZER_INTERFACE_ATTRIBUTE
-int __sanitizer_posix_memalign(void **memptr, uptr alignment, uptr size);
-
-SANITIZER_INTERFACE_ATTRIBUTE
-void * __sanitizer_memalign(uptr alignment, uptr size);
-
-SANITIZER_INTERFACE_ATTRIBUTE
-void * __sanitizer_aligned_alloc(uptr alignment, uptr size);
-
-SANITIZER_INTERFACE_ATTRIBUTE
-void * __sanitizer___libc_memalign(uptr alignment, uptr size);
-
-SANITIZER_INTERFACE_ATTRIBUTE
-void * __sanitizer_valloc(uptr size);
-
-SANITIZER_INTERFACE_ATTRIBUTE
-void * __sanitizer_pvalloc(uptr size);
-
-SANITIZER_INTERFACE_ATTRIBUTE
-void __sanitizer_free(void *ptr);
-
-SANITIZER_INTERFACE_ATTRIBUTE
-void __sanitizer_cfree(void *ptr);
-
-SANITIZER_INTERFACE_ATTRIBUTE
-uptr __sanitizer_malloc_usable_size(const void *ptr);
-
-SANITIZER_INTERFACE_ATTRIBUTE
-__hwasan::__sanitizer_struct_mallinfo __sanitizer_mallinfo();
-
-SANITIZER_INTERFACE_ATTRIBUTE
-int __sanitizer_mallopt(int cmd, int value);
-
-SANITIZER_INTERFACE_ATTRIBUTE
-void __sanitizer_malloc_stats(void);
-
-SANITIZER_INTERFACE_ATTRIBUTE
-void * __sanitizer_calloc(uptr nmemb, uptr size);
-
-SANITIZER_INTERFACE_ATTRIBUTE
-void * __sanitizer_realloc(void *ptr, uptr size);
-
-SANITIZER_INTERFACE_ATTRIBUTE
-void * __sanitizer_reallocarray(void *ptr, uptr nmemb, uptr size);
-
-SANITIZER_INTERFACE_ATTRIBUTE
-void * __sanitizer_malloc(uptr size);
-
-SANITIZER_INTERFACE_ATTRIBUTE
 void *__hwasan_memcpy(void *dst, const void *src, uptr size);
 SANITIZER_INTERFACE_ATTRIBUTE
 void *__hwasan_memset(void *s, int c, uptr n);
 SANITIZER_INTERFACE_ATTRIBUTE
 void *__hwasan_memmove(void *dest, const void *src, uptr n);
 
 SANITIZER_INTERFACE_ATTRIBUTE
 void __hwasan_set_error_report_callback(void (*callback)(const char *));
 }  // extern "C"
 
 #endif  // HWASAN_INTERFACE_INTERNAL_H