Bug 23343 - tcache_init() confuses mtrace()
Summary: tcache_init() confuses mtrace()
Status: NEW
Alias: None
Product: glibc
Classification: Unclassified
Component: malloc (show other bugs)
Version: 2.29
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-06-27 02:39 UTC by Carlos O'Donell
Modified: 2021-10-14 00:38 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments
Avoid calling hooks on unhooked allocations. (390 bytes, patch)
2021-09-23 20:41 UTC, bungeman
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos O'Donell 2018-06-27 02:39:13 UTC
In tcache_init() we call directly into _int_malloc() to get a chunk. This direct fetching of a chunk from internal malloc bypasses the hook tracing used by mtrace().

Later in tcache_thread_shutdown() we issue __libc_free() which does go through the __free_hook.

This causes mtrace() to return errors like:
- 0x00007f50400008d0 Free 13 was never alloc'd ??:?

Which indicate that there was an allocation that was free'd but never allocated.

Tracing this down shows it is the tcache:

(gdb) p victim
$3 = (void *) 0x7ffff00008d0
(gdb) bt
#0  tcache_init () at malloc.c:2988
#1  0x00007ffff767f75b in tcache_init () at malloc.c:2983
#2  __GI___libc_malloc (bytes=1024) at malloc.c:3042
#3  0x00007ffff7681c58 in tr_mallochook (size=1024, caller=0x7ffff766a7e9 <__GI__IO_file_doallocate+121>) at mtrace.c:162
#4  0x00007ffff766a7e9 in __GI__IO_file_doallocate (fp=0x7ffff79af760 <_IO_2_1_stdout_>) at filedoalloc.c:101
#5  0x00007ffff7678a89 in __GI__IO_doallocbuf (fp=fp@entry=0x7ffff79af760 <_IO_2_1_stdout_>) at genops.c:347
#6  0x00007ffff7677d68 in _IO_new_file_overflow (f=0x7ffff79af760 <_IO_2_1_stdout_>, ch=-1) at fileops.c:752
#7  0x00007ffff7676e2f in _IO_new_file_xsputn (f=0x7ffff79af760 <_IO_2_1_stdout_>, data=<optimized out>, n=15) at fileops.c:1251
#8  0x00007ffff766cd8f in __GI__IO_puts (str=0x4009f0 "Running thread!") at ioputs.c:40
#9  0x000000000040088d in worker (arg=0x0) at ./tst-pthreads.c:10
#10 0x00007ffff79bb777 in start_thread (arg=0x7ffff75fe700) at pthread_create.c:474
#11 0x00007ffff76f17cf in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

mtrace ./tst-pthreads ./outTC.mtrace
- 0x00007ffff00008d0 Free 11 was never alloc'd 0x7ffff775eb64
No memory leaks.

As you can see the tcache in the non-main thread is allocated, and then free'd later, and this confuses mtrace().

This is a non-issue for valgrind which replaces the entire malloc implementation entirely.
Comment 1 bungeman 2021-09-22 23:55:19 UTC
I recently also ran into this with `mcheck` and the associated hooks. Both the `tcache` and its `entries` are always backed directly as chunks (bare `mchunkptr` or `malloc_chunk` allocations) as they always come directly out of `_int_malloc` or `int_free`. Looking at the rest of the file (and since these are internal allocations) it appears that these should be freed with `_int_free` after finding the correct arena.

Currently `tcache_thread_shutdown` calls `__libc_free` on both the `tcache` and its `entries`. When running with `mcheck` the hooks install or expect a `hdr` before the mem ptr (the `malloc_chunk` will come before this `hdr`). Since these allocations did not go through `__libc_malloc` or have already gone through `__libc_free` they do not have a `hdr` present (and the pointer value has already been adjusted to before where the `hdr` was installed in the allocation). As a result, the mcheck `freehook` which is called when `__libc_free` is called fails in `checkhdr` since there isn't actually a `hdr` there.

I managed to capture this happening in rr with an tcache entry. A somewhat simplified reverse debugging session demonstrating the issue looks like

347           msg = _ ("memory clobbered before allocated block\n");
#0  0x00007f6f2b98bc95 in mabort (status=<optimized out>) at mcheck.c:347
#1  0x00007f6f2b98bd2b in checkhdr (hdr=hdr@entry=0x55e969231090) at mcheck.c:111
#2  0x00007f6f2b98c129 in checkhdr (hdr=0x55e969231090) at mcheck.c:86
#3  freehook (ptr=0x55e9692310c0, caller=0x7f6f2b98a87b <__malloc_arena_thread_freeres+75>) at mcheck.c:184
#4  0x00007f6f2b98a87b in tcache_thread_shutdown () at malloc.c:2964
#5  __malloc_arena_thread_freeres () at arena.c:951
#6  0x00007f6f2b98db6c in __libc_thread_freeres () at thread-freeres.c:38
#7  0x00007f6f2c0b9ebf in start_thread (arg=<optimized out>) at pthread_create.c:491
#8  0x00007f6f2b9fddef in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

(rr) reverse-finish  // a few times

2964              __libc_free (e);
#0  tcache_thread_shutdown () at malloc.c:2964
#1  __malloc_arena_thread_freeres () at arena.c:951
#2  0x00007f6f2b98db6c in __libc_thread_freeres () at thread-freeres.c:38
#3  0x00007f6f2c0b9ebf in start_thread (arg=<optimized out>) at pthread_create.c:491

(rr) when-ticks
Current tick: 25415

(rr) print e
$1 = (tcache_entry *) 0x55e9692310c0

(rr) print *e
$2 = {next = 0x0, key = 0x7f6f240008d0}

// The data before e looks like a legal malloc_chunk
(rr) print *(mchunkptr)((char*)e - 2*sizeof(size_t))
$11 = {mchunk_prev_size = 96, mchunk_size = 97, ...}

// The data before e does not look like a legal mcheck hdr, so checkhdr aborted
(rr) print ((struct hdr *) e) - 1
$17 = (struct hdr *) 0x55e969231090
(rr) print *(((struct hdr *) e) - 1)
$19 = {size = 4294967298, magic = 10778686036297936231, prev = 0x9595959595959595, next = 0x9595959595959595, block = 0x60, magic2 = 97}

(rr) reverse-next   // a few times to get before while loop

2954      tcache_shutting_down = true;
#0  tcache_thread_shutdown () at malloc.c:2954
#1  __malloc_arena_thread_freeres () at arena.c:951
#2  0x00007f6f2b98db6c in __libc_thread_freeres () at thread-freeres.c:38
#3  0x00007f6f2c0b9ebf in start_thread (arg=<optimized out>) at pthread_create.c:491
#4  0x00007f6f2b9fddef in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

(rr) print *tcache_tmp
$20 = {counts = {0, 0, 0, 0, 1, 0 <repeats 59 times>}, entries = {0x0, 0x0, 0x0, 0x0, 0x55e9692310c0, 0x0 <repeats 59 times>}}

(rr) print *tcache_tmp->entries[4]
$21 = {next = 0x0, key = 0x7f6f240008d0}

(rr) watch -l tcache_tmp->entries[4]
(rr) reverse-cont

2927      tcache->entries[tc_idx] = e;
#0  tcache_put (tc_idx=4, chunk=0x55e9692310b0) at malloc.c:2927
#1  _int_free (av=0x7f6f2babeb80 <main_arena>, p=0x55e9692310b0, have_lock=0) at malloc.c:4208
#2  0x00007f6f2b98c1b8 in freehook (ptr=0x55e9692310c0, caller=0x55e964dbca21) at mcheck.c:196
#3  0x000055e964dbca21 in std::thread::_State_impl<...>::~_State_impl() (this=0x55e9692310f0, __in_chrg=<optimized out>) at /usr/include/c++/10/thread:205
#4  0x00007f6f2bcf531a in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
#5  0x00007f6f2c0b9ea7 in start_thread (arg=<optimized out>) at pthread_create.c:477
#6  0x00007f6f2b9fddef in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

(rr) when-ticks
Current tick: 13449

(rr) print e
$25 = (tcache_entry *) 0x55e9692310c0
(rr) print *e
$26 = {next = 0x0, key = 0x7f6f240008d0}

Where it is easy to verify that `freehook` removed the `hdr` (overwriting it to mark it) by adjusting the pointer back to before the `hdr`, then calls `_int_free` which adds this now non-mcheck chunk to the tcache.  As a result this now "hook-clean" freed allocation must not be freed through the hooks (as they have already cleaned up their claim on this allocation and marked it free). tcache_thread_shutdown is already careful to remove the entry from the entries list before attempting to free it, so just calling _int_free (with the correct arena) should be correct.
Comment 2 bungeman 2021-09-23 20:41:40 UTC
Created attachment 13678 [details]
Avoid calling hooks on unhooked allocations.

I believe this is more or less the correct logical approach to avoid calling the potentially hooked `__libc_free` on the unhooked allocations in the tchace.
Comment 3 bungeman 2021-09-30 02:17:54 UTC
Even though it looks like the hooks are on their way out ( https://sourceware.org/bugzilla/show_bug.cgi?id=23328 ), wanted to ensure someone was copied on this particular issue since this code still exists.
Comment 4 Siddhesh Poyarekar 2021-10-14 00:38:47 UTC
(In reply to bungeman from comment #2)
> Created attachment 13678 [details]
> Avoid calling hooks on unhooked allocations.
> 
> I believe this is more or less the correct logical approach to avoid calling
> the potentially hooked `__libc_free` on the unhooked allocations in the
> tchace.

Could you please send the patch on the libc-alpha mailing list?  Please review this patch contribution checklist when you do so:

https://sourceware.org/glibc/wiki/Contribution%20checklist

Thanks!