Add __sanitizer_get_allocated_begin API and implementations

[thurston]

Summary

This function will return the start of the allocation, if given a pointer that lies within an allocation. Otherwise, it returns NULL.

It will be useful for detecting dynamic TLS allocations in glibc >=2.25, which
uses malloc (see https://github.com/google/sanitizers/issues/1409#issuecomment-1214244142).

[thurston]

Add test for freed (new int)

[kstoimenov]

compiler-rt/lib/hwasan/hwasan_allocator.cpp

646 ↗

(On Diff #508810)

Nit: no space after function name.

653 ↗

(On Diff #508810)

I think that beg will be an untagged pointer. Please take that into account when implementing the client code because there is a good chance the pointer in the client code will be tagged.

compiler-rt/test/msan/get_allocation_bounds.cpp

1 ↗

(On Diff #508810)

Can we try moving this to compiler-rt/test/sanitizer_common? I am working on enabling HWASAN for those so when it's done HWASAN will be covered as well. Currently "supported tools" are: set(SUPPORTED_TOOLS_INIT asan lsan msan tsan ubsan) in compiler-rt/test/sanitizer_common/CMakeLists.txt.

[thurston]

compiler-rt/test/msan/get_allocation_bounds.cpp

1 ↗

(On Diff #508810)

To put this in sanitizer_common tests, I would need to add __sanitizer_get_allocation_bounds to stand-alone ubsan, even though AFAICS it doesn't have its own memory allocator. Should I proceed with that?

[kstoimenov]

compiler-rt/test/msan/get_allocation_bounds.cpp

1 ↗

(On Diff #508810)

Main motivation is to have this test run with all applicable sanitizers. You could disabled UBSAN using the UNSUPPORTED keyword. Another option is to provide an implementation which would satisfy the test with the standard allocator. Please double-check with vitalybuka@ before going down that path.

[thurston]

Minor bug fix

[thurston]

Revert to diff 2 (+ whitespace changes)

[thurston]

Formatting and minor bug fixes

[thurston]

Add function to sanitizer_common_interface.inc

[thurston]

Change HWASan implementation of __sanitizer_get_allocation_bounds to return tagged pointer

Mark test as unsupported for UBSan

[thurston]

compiler-rt/lib/hwasan/hwasan_allocator.cpp

653 ↗

(On Diff #508810)

Thanks for pointing this out! I've changed the implementation to return a tagged pointer.

[thurston]

Test both the primary and secondary allocators, by trying many different allocation sizes

[vitalybuka]

Could you please add a patch into the stack where it's used?

compiler-rt/include/sanitizer/allocator_interface.h

36 ↗

(On Diff #509789)

This is public interface, if we don't have to, we should avoid exposing it here.
Do we have a reason to do so?

sanitizer_common/sanitizer_interface_internal.h probably more appropriate?

compiler-rt/test/sanitizer_common/TestCases/get_allocation_bounds.cpp

7 ↗

(On Diff #509789)

how long this takes with? Can you make sure that it does not get into the top of:
LIT_OPTS="--time-tests" ninja check-sanitizer

[thurston]

compiler-rt/include/sanitizer/allocator_interface.h

36 ↗

(On Diff #509789)

I'll try moving it there.

compiler-rt/test/sanitizer_common/TestCases/get_allocation_bounds.cpp

7 ↗

(On Diff #509789)

It's not among the slowest tests:

4.70s: SanitizerCommon-tsan-x86_64-Linux :: get_allocation_bounds.cpp
4.53s: SanitizerCommon-asan-i386-Linux :: get_allocation_bounds.cpp
4.17s: SanitizerCommon-asan-x86_64-Linux :: get_allocation_bounds.cpp
3.99s: SanitizerCommon-msan-x86_64-Linux :: get_allocation_bounds.cpp
3.45s: SanitizerCommon-lsan-i386-Linux :: get_allocation_bounds.cpp
3.29s: SanitizerCommon-lsan-x86_64-Linux :: get_allocation_bounds.cpp

Slowest Tests:
--------------------------------------------------------------------------
16.38s: SanitizerCommon-msan-x86_64-Linux :: Linux/soft_rss_limit_mb_test.cpp
14.82s: SanitizerCommon-asan-i386-Linux :: Linux/soft_rss_limit_mb_test.cpp
14.77s: SanitizerCommon-asan-x86_64-Linux :: Linux/soft_rss_limit_mb_test.cpp
14.42s: SanitizerCommon-ubsan-i386-Linux :: Posix/qsort.cpp
14.14s: SanitizerCommon-tsan-x86_64-Linux :: Linux/soft_rss_limit_mb_test.cpp
14.12s: SanitizerCommon-ubsan-x86_64-Linux :: Posix/qsort.cpp
14.06s: SanitizerCommon-lsan-i386-Linux :: Linux/soft_rss_limit_mb_test.cpp
13.83s: SanitizerCommon-lsan-x86_64-Linux :: Linux/soft_rss_limit_mb_test.cpp
11.52s: SanitizerCommon-msan-x86_64-Linux :: compress_stack_depot.cpp
11.18s: SanitizerCommon-asan-i386-Linux :: Posix/qsort.cpp
10.47s: SanitizerCommon-Unit :: ./Sanitizer-x86_64-Test/SanitizerCommon/CombinedAllocator32Compact
10.29s: SanitizerCommon-msan-x86_64-Linux :: Posix/qsort.cpp
10.24s: SanitizerCommon-asan-x86_64-Linux :: Posix/qsort.cpp
9.82s: SanitizerCommon-lsan-i386-Linux :: Posix/qsort.cpp
9.81s: SanitizerCommon-asan-i386-Linux :: get_allocation_bounds.cpp
9.57s: SanitizerCommon-lsan-x86_64-Linux :: Posix/qsort.cpp
9.52s: SanitizerCommon-asan-i386-Linux :: Linux/signal_segv_handler.cpp
9.48s: SanitizerCommon-tsan-x86_64-Linux :: Linux/resize_tls_dynamic.cpp
9.39s: SanitizerCommon-asan-i386-Linux :: compress_stack_depot.cpp
9.37s: SanitizerCommon-lsan-i386-Linux :: Linux/signal_segv_handler.cpp

[thurston]

Move __sanitizer_get_allocation_bounds to internal header, per Vitaly's suggestion.

Update test accordingly.

[thurston]

compiler-rt/lib/sanitizer_common/sanitizer_allocator_interface.h

40–42 ↗

(On Diff #509789)

I'll work on removing this too.

[vitalybuka]

compiler-rt/include/sanitizer/allocator_interface.h

36 ↗

(On Diff #509789)

Sorry, after looking into this more, seems original header was fine.

36 ↗

(On Diff #509789)

there is redundancy sanitizer_get_allocation_bounds sanitizer_get_allocated_size

sanitizer_get_allocated_size does half the work
would it be better to add:

void * __sanitizer_get_allocated_begin(const volatile void *p)

[vitalybuka]

compiler-rt/lib/hwasan/hwasan_allocator.cpp

644 ↗

(On Diff #510148)

I'd recommend to extract AllocationBegin() next to AllocationSize() and keep interface methods implementation short like __sanitizer_get_allocated_size.

Please do the same for all sanitizers

646 ↗

(On Diff #510148)

looks tool long, please

git clang-format -f --extensions=inc,cc,h,c,cpp,rst HEAD^

compiler-rt/lib/tsan/rtl/tsan_mman.cpp

450 ↗

(On Diff #510148)

is this for malloc(0)? looks strange, but let's keep it consistent with __sanitizer_get_allocated_size

[vitalybuka]

compiler-rt/lib/tsan/rtl/tsan_mman.cpp

450 ↗

(On Diff #510148)

is this for malloc(0)? looks strange, but let's keep it consistent with __sanitizer_get_allocated_size

strange that we return 1 to the user, I would expected the same value as passed into malloc.

[thurston]

Change sanitizer_get_allocation_bounds to sanitizer_get_allocated_begin,
per Vitaly's comment

[thurston]

In D147005#4237707, @vitalybuka wrote:

Could you please add a patch into the stack where it's used?

Coming soon (tm)

compiler-rt/include/sanitizer/allocator_interface.h

36 ↗

(On Diff #509789)

Sorry, after looking into this more, seems original header was fine.

I've moved it back to the original header

36 ↗

(On Diff #509789)

there is redundancy sanitizer_get_allocation_bounds sanitizer_get_allocated_size

sanitizer_get_allocated_size does half the work
would it be better to add:

void * __sanitizer_get_allocated_begin(const volatile void *p)

Done

compiler-rt/lib/hwasan/hwasan_allocator.cpp

644 ↗

(On Diff #510148)

I'd recommend to extract AllocationBegin() next to AllocationSize() and keep interface methods implementation short like __sanitizer_get_allocated_size.

Please do the same for all sanitizers

Done

646 ↗

(On Diff #510148)

looks tool long, please

git clang-format -f --extensions=inc,cc,h,c,cpp,rst HEAD^

Done

[thurston]

In D147005#4241093, @thurston wrote:

In D147005#4237707, @vitalybuka wrote:

Could you please add a patch into the stack where it's used?

Coming soon (tm)

This patch is useful for "Fix tls_get_addr handling for glibc >=2.25" (https://reviews.llvm.org/D147459)

[vitalybuka]

compiler-rt/lib/dfsan/dfsan_allocator.cpp

181 ↗

(On Diff #510552)

We use implicit pointer tests, please fix for entire patch

[vitalybuka]

compiler-rt/lib/lsan/lsan_allocator.cpp

366

why this one is SANITIZER_WEAK_ATTRIBUTE? It should not be

[thurston]

Use implicit pointer checks, per Vitaly's feedback

Also removed unnecessary weak annotation (though the
header still has it).

[thurston]

compiler-rt/lib/dfsan/dfsan_allocator.cpp

181 ↗

(On Diff #510552)

Done

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_allocator_interface.h

24 ↗

(On Diff #510613)

It was LGTM assuming you'll remove SANITIZER_WEAK_ATTRIBUTE

compiler-rt/lib/lsan/lsan_allocator.cpp

 uptr __sanitizer_get_unmapped_bytes() { return 0; }
 
 SANITIZER_INTERFACE_ATTRIBUTE
 uptr __sanitizer_get_estimated_allocated_size(uptr size) { return size; }
 
 SANITIZER_INTERFACE_ATTRIBUTE
 int __sanitizer_get_ownership(const void *p) { return Metadata(p) != nullptr; }
 
 SANITIZER_INTERFACE_ATTRIBUTE
 uptr __sanitizer_get_allocated_size(const void *p) {
   return GetMallocUsableSize(p);
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
 int __sanitizer_get_allocation_bounds(const void *p, void **start, unsigned long long *size) {
   if (!p) return 0;
   const void *beg = allocator.GetBlockBegin(p);
   if (beg == nullptr) return 0;
   ChunkMetadata *m = Metadata(beg);
   if (m == nullptr) return 0;
+  if (!m->allocated) return 0;
   if (m->requested_size == 0) return 0;
   *start = (void*)beg;
   *size = m->requested_size;
   return 1;
 }
 
 } // extern "C"