This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Switch dlsym hack to internal_allocator
ClosedPublic

Authored by vitalybuka on Oct 26 2021, 6:03 PM.

Details

Summary

Since glibc 2.34, dlsym does

  1. malloc 1
  2. malloc 2
  3. free pointer from malloc 1
  4. free pointer from malloc 2

These sequence was not handled by trivial dlsym hack.

This fixes https://bugs.llvm.org/show_bug.cgi?id=52278

Diff Detail

Event Timeline

hjl.tools requested review of this revision.Oct 26 2021, 6:03 PM
hjl.tools created this revision.
hjl.tools updated this revision to Diff 382606.Oct 27 2021, 4:42 AM
vitalybuka added inline comments.Nov 1 2021, 4:39 PM
compiler-rt/lib/asan/asan_malloc_linux.cpp
43

I realy don't like were we are going. What if we need 3 soon?

Any idea why we can't use internal_allocator()?

if we can't use internal allocator here I propose the following to simplify this.

  1. AllocateFromLocalPool store size at (return value - 1) location.
  2. so DeallocateFromLocalPool can find size from there.
  3. To create byte(or bit) map to mark used words.
  4. AllocateFromLocalPool will use last_dlsym_alloc_size_in_words_index to walkback and find start of unused bytest (or maybe just remove last_dlsym_alloc_size_in_words_index and scan full range)
vitalybuka requested changes to this revision.Nov 3 2021, 12:36 PM
vitalybuka added inline comments.
compiler-rt/lib/asan/asan_malloc_linux.cpp
43

Please let me know if you are going to implement suggested changes.
If if not, you are fixing a real issue, and I will do that myself.

This revision now requires changes to proceed.Nov 3 2021, 12:36 PM
hjl.tools added inline comments.Nov 3 2021, 12:45 PM
compiler-rt/lib/asan/asan_malloc_linux.cpp
43

Please let me know if you are going to implement suggested changes.
If if not, you are fixing a real issue, and I will do that myself.

I am not familiar with the code. Could you please fix it? Thanks.

vitalybuka commandeered this revision.Nov 3 2021, 5:41 PM
vitalybuka edited reviewers, added: hjl.tools; removed: vitalybuka.
This revision now requires review to proceed.Nov 3 2021, 5:41 PM
vitalybuka planned changes to this revision.Nov 3 2021, 5:41 PM
vitalybuka retitled this revision from [sanitizer] Avoid memory leak from dlsym in glibc 2.34 to [sanitizer] Switch to dlsym hack to internal_allocator.Nov 10 2021, 6:10 PM
vitalybuka edited the summary of this revision. (Show Details)
vitalybuka retitled this revision from [sanitizer] Switch to dlsym hack to internal_allocator to [sanitizer] Switch dlsym hack to internal_allocator.
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2021, 6:18 PM
Herald added subscribers: Restricted Project, abrachet, phosek, mgorny. · View Herald Transcript
vitalybuka added a reviewer: morehouse.

remove global

morehouse accepted this revision.Nov 11 2021, 3:27 PM
morehouse added inline comments.
compiler-rt/lib/memprof/memprof_malloc_linux.cpp
32–33

Shouldn't this be GET_STACK_TRACE_MALLOC?

This revision is now accepted and ready to land.Nov 11 2021, 3:27 PM
eugenis added inline comments.Nov 11 2021, 3:38 PM
compiler-rt/lib/asan/asan_malloc_linux.cpp
42–45

Why do we need to specialize ReallocImpl? And do we even need stack traces here?

vitalybuka marked 2 inline comments as done.

Add root region hooks

vitalybuka added inline comments.Nov 12 2021, 10:20 AM
compiler-rt/lib/asan/asan_malloc_linux.cpp
39

This one is annoying. To keep behavioral closer to original I use root regions.
What I see is that "dlsym hack" allocations contains pointers to later regular dlerror allocations.

it would be nice to investigate if dlerror is special. If so we can remove regions in followup patches
and add dlerror interceptors to lsan and asan.

eugenis added inline comments.Nov 12 2021, 3:34 PM
compiler-rt/lib/sanitizer_common/sanitizer_allocator_dlsym.h
68

I'd rather call OnFree(p) before deallocating, even if it does not matter for lsan.
Or does it?

vitalybuka added inline comments.Nov 12 2021, 3:37 PM
compiler-rt/lib/sanitizer_common/sanitizer_allocator_dlsym.h
68

we do on like 52

vitalybuka added inline comments.Nov 12 2021, 3:40 PM
compiler-rt/lib/sanitizer_common/sanitizer_allocator_dlsym.h
68

if the question about Details::OnAllocate Details::OnFree order then
the point to keep at least one of buffers, ptr or new_ptr, in lsan roots.

redo Realloc

eugenis accepted this revision.Nov 12 2021, 4:03 PM

LGTM

This revision was landed with ongoing or failed builds.Nov 12 2021, 4:11 PM
This revision was automatically updated to reflect the committed changes.