This is an archive of the discontinued LLVM Phabricator instance.

[ASan] Use dylib interposition to hook memory allocation in the dynamic runtime.
ClosedPublic

Authored by glider on Dec 13 2012, 9:18 AM.

Details

Reviewers
kcc
Summary

This CL drastically simplifies the way we're hooking the memory allocation routines in ASan on Mac by using dylib interposition to replace the main malloc_zone_* functions. This allows us to avoid replacing the default CFAllocator and drop the CF dependency at all.
Committing this patch as is will result in the static runtime being broken, so we probably need to work out the transition plan before switching to the dynamic runtime.

Diff Detail

Event Timeline

Cool. Do we have many clients on Mac that would have to hack their specific build systems for the dynamic runtime? I think that if installed Clang would be able to produce working binaries with "clang++ -fsanitize=address a.cc" the change is fine.

minor nits below.

asan_malloc_mac.cc
146 ↗(On Diff #521)

Don't need else after return.

205 ↗(On Diff #521)

Hm? So the code for free_common is just
if (!ptr) return;
GET_STACK_TRACE_FREE;
asan_free(ptr, &stack);
?

368 ↗(On Diff #521)

delete this whole block?

kcc added a comment.Dec 14 2012, 1:12 AM

This is getting rid of mach_override, right? Yay!!
I'd like some Apple folks to review this too, but as long as the tests pass I think
the review may happen post-commit.

asan_malloc_mac.cc
346 ↗(On Diff #521)

note sure I understand this comment and the following line

glider updated this revision to Unknown Object (????).Dec 18 2012, 3:20 AM

Addressed Phabricator comments by Kostya and Alexey and off-list comments by Anna Zaks and Dave Payne

glider added inline comments.Dec 18 2012, 3:25 AM
asan_malloc_mac.cc
146 ↗(On Diff #521)

Fixed.

205 ↗(On Diff #521)

Yes. Fixed.

346 ↗(On Diff #521)

It's just wrong. Removed it.

368 ↗(On Diff #521)

done

glider updated this revision to Unknown Object (????).Dec 18 2012, 4:01 AM

Dummy implementations for malloc_make_purgeable and malloc_make_nonpurgeable (required by Chrome)

samsonov added inline comments.Dec 18 2012, 5:34 AM
asan_intercepted_functions.h
232 ↗(On Diff #539)

Hm, can you use uptr instead of size_t?

asan_malloc_mac.cc
87 ↗(On Diff #539)

Why do you need to allocate/free memory for zone name via ASan allocator (with fetching stack trace for malloc etc.)
Can you use InternalScopedBuffer instead?

109 ↗(On Diff #539)

Remove this (or hide under verbosity?)

149 ↗(On Diff #539)

Can memptr be zero? Or it's fine to segfault in this case?

204 ↗(On Diff #539)

Why? (just curious)

lit_tests/heap-overflow.cc
32 ↗(On Diff #539)

_?wrap_malloc here and below?

tests/CMakeLists.txt
84 ↗(On Diff #539)

Will static runtime work at all after this change? If no, plan to remove rules for building it in /lib/asan/CMakeLists.txt

glider updated this revision to Unknown Object (????).Dec 18 2012, 6:22 AM

More comments from Alexey

asan_intercepted_functions.h
232 ↗(On Diff #539)

I think it's generally wrong to rewrite the function prototypes using our typenames.
Anyway, Kostya wants this file to be gone soon after my CL, so let us just leave it as is for now.

asan_malloc_mac.cc
87 ↗(On Diff #539)

Good catch, thanks! Done.

109 ↗(On Diff #539)

Done

149 ↗(On Diff #539)

Added a check.

204 ↗(On Diff #539)

We needed it because some allocations used to be intercepted incorrectly on Mac, and sometimes invalid free were reported because of the zone mismatch. In such cases we wanted to keep going.
Now everything will be allocated from a single zone, thus no mismatches

lit_tests/heap-overflow.cc
32 ↗(On Diff #539)

Seems to work as is, but good idea anyway.

tests/CMakeLists.txt
84 ↗(On Diff #539)

It won't. There'll be a cleanup CL afterwards.

LGTM

asan_malloc_mac.cc
87 ↗(On Diff #540)

Will this work for buflen == 0?

89 ↗(On Diff #540)

We have internal_snprintf

glider updated this revision to Unknown Object (????).Dec 19 2012, 4:29 AM

Addressed two comments from Alexey

asan_malloc_mac.cc
87 ↗(On Diff #540)

Refactored the code so that buflen is always > 0.

89 ↗(On Diff #540)

Done, thanks.

glider updated this revision to Unknown Object (????).Dec 20 2012, 5:43 AM

Merge with the current trunk.
Fix lit_tests/interface_symbols.c

PTAL at the changed interface_symbols.c test.

samsonov added inline comments.Dec 20 2012, 5:57 AM
lit_tests/interface_symbols.c
7 ↗(On Diff #557)

This RUN-lines won't work, simply create platofrm-specific tests in lit_tests/Linux and lit_tests/Darwin

lit_tests/lit.cfg
91 ↗(On Diff #557)

Remove this

glider updated this revision to Unknown Object (????).Dec 20 2012, 6:43 AM

Split interface_symbols.c into two platform-specific tests.

glider updated this revision to Unknown Object (????).Jan 11 2013, 8:03 AM

Update the patch so that it applies against the current compiler-rt.

kcc accepted this revision.Jan 14 2013, 1:21 AM

LGTM

glider updated this revision to Unknown Object (????).Jan 21 2013, 10:00 AM

Merge with the current LLVM trunk. I'm about to finally commit this tomorrow.

LGTM

asan_malloc_mac.cc
46 ↗(On Diff #736)

I think lint would advice you to prefer sizeof(asan_zone)

86 ↗(On Diff #736)

I think that internal_snprintf() adds a trailing \0

lit_tests/malloc_delete_mismatch.cc
1 ↗(On Diff #736)

Did you move this file somewhere?

glider added inline comments.Jan 22 2013, 12:44 AM
asan_malloc_mac.cc
46 ↗(On Diff #736)

Done. Although I'm a bit unsure about this since I'm not allocating asan_zone itself.

86 ↗(On Diff #736)

It sure does. Something clicked in my head and made me apply the bug mitigation pattern used for strncpy :)
Thanks for catching this!

lit_tests/malloc_delete_mismatch.cc
1 ↗(On Diff #736)

Yes. Let me upload another diff.

glider updated this revision to Unknown Object (????).Jan 22 2013, 12:45 AM

Fixed Alexey's comments, added the missing files to the patch using arc diff.

glider updated this revision to Unknown Object (????).Jan 22 2013, 12:55 AM

Fix lint warnings.

Eugene.Zelenko closed this revision.Oct 5 2016, 4:37 PM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in rL173134.

lib/asan/lit_tests/Darwin/interface_symbols_darwin.c