This is an archive of the discontinued LLVM Phabricator instance.

Add __sanitizer_get_allocated_begin API and implementations
ClosedPublic

Authored by thurston on Mar 27 2023, 3:02 PM.

Details

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).

Diff Detail

Event Timeline

thurston created this revision.Mar 27 2023, 3:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2023, 3:02 PM
Herald added a subscriber: Enna1. · View Herald Transcript
thurston requested review of this revision.Mar 27 2023, 3:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2023, 3:02 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
thurston updated this revision to Diff 508810.Mar 27 2023, 3:09 PM

Add test for freed (new int)

kstoimenov added inline comments.
compiler-rt/lib/hwasan/hwasan_allocator.cpp
667

Nit: no space after function name.

674

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 added inline comments.Mar 27 2023, 4:42 PM
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 added inline comments.Mar 27 2023, 5:00 PM
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 updated this revision to Diff 508854.Mar 27 2023, 5:26 PM

Minor bug fix

thurston updated this revision to Diff 508859.Mar 27 2023, 5:35 PM

Revert to diff 2 (+ whitespace changes)

thurston updated this revision to Diff 508861.Mar 27 2023, 5:42 PM

Formatting and minor bug fixes

thurston updated this revision to Diff 509059.Mar 28 2023, 9:52 AM

Add function to sanitizer_common_interface.inc

thurston updated this revision to Diff 509135.Mar 28 2023, 2:16 PM

Change HWASan implementation of __sanitizer_get_allocation_bounds to return tagged pointer

Mark test as unsupported for UBSan

thurston added inline comments.Mar 28 2023, 2:18 PM
compiler-rt/lib/hwasan/hwasan_allocator.cpp
674

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

thurston marked an inline comment as done.Mar 29 2023, 9:43 AM
thurston updated this revision to Diff 509789.Mar 30 2023, 12:52 PM

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

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

compiler-rt/include/sanitizer/allocator_interface.h
40

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 added inline comments.Mar 31 2023, 3:23 PM
compiler-rt/include/sanitizer/allocator_interface.h
40

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 updated this revision to Diff 510148.Mar 31 2023, 4:40 PM

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

Update test accordingly.

thurston marked an inline comment as not done.Mar 31 2023, 4:52 PM
thurston added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_allocator_interface.h
42–44

I'll work on removing this too.

vitalybuka added inline comments.Mar 31 2023, 5:13 PM
compiler-rt/include/sanitizer/allocator_interface.h
40

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

40

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 added inline comments.Mar 31 2023, 5:30 PM
compiler-rt/lib/hwasan/hwasan_allocator.cpp
665

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

667

looks tool long, please

git clang-format -f --extensions=inc,cc,h,c,cpp,rst HEAD^
compiler-rt/lib/tsan/rtl/tsan_mman.cpp
468

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

vitalybuka added inline comments.Mar 31 2023, 5:31 PM
compiler-rt/lib/tsan/rtl/tsan_mman.cpp
468

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 updated this revision to Diff 510552.Apr 3 2023, 10:42 AM

Change sanitizer_get_allocation_bounds to sanitizer_get_allocated_begin,
per Vitaly's comment

thurston retitled this revision from Add __sanitizer_get_allocation_bounds API and implementations to Add __sanitizer_get_allocated_begin API and implementations.Apr 3 2023, 10:43 AM
thurston edited the summary of this revision. (Show Details)

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

Coming soon (tm)

compiler-rt/include/sanitizer/allocator_interface.h
40

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

I've moved it back to the original header

40

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
665

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

667

looks tool long, please

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

Done

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 accepted this revision.Apr 3 2023, 1:10 PM
vitalybuka added inline comments.
compiler-rt/lib/dfsan/dfsan_allocator.cpp
181

We use implicit pointer tests, please fix for entire patch

This revision is now accepted and ready to land.Apr 3 2023, 1:10 PM
vitalybuka added inline comments.Apr 3 2023, 1:12 PM
compiler-rt/lib/lsan/lsan_allocator.cpp
382

why this one is SANITIZER_WEAK_ATTRIBUTE? It should not be

thurston updated this revision to Diff 510595.Apr 3 2023, 1:45 PM

Use implicit pointer checks, per Vitaly's feedback

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

thurston added inline comments.Apr 3 2023, 1:47 PM
compiler-rt/lib/dfsan/dfsan_allocator.cpp
181

Done

vitalybuka accepted this revision.Apr 3 2023, 2:08 PM
This revision was landed with ongoing or failed builds.Apr 3 2023, 2:45 PM
This revision was automatically updated to reflect the committed changes.
vitalybuka added inline comments.Apr 3 2023, 6:02 PM
compiler-rt/lib/sanitizer_common/sanitizer_allocator_interface.h
24

It was LGTM assuming you'll remove SANITIZER_WEAK_ATTRIBUTE