[PATCH] Fix invalid free in trace_close_memstream

[Dmitry V. Levin]

On Sat, Aug 03, 2019 at 11:19:03PM +0100, Pierre Marsais wrote:
> In maybe_switch_tcbs we exchange the pointers to the memstream's
> buffers between 2 tcb, however the libc doesn't know and keeps updating
> the tcb->memfptr as if the exchange didn't happen. This leads to
> unsynchronized tcb->memfptr and tcb->outf and invalid frees. Adding a
> new indirection fixes the problem.
> 
> * defs.h (struct tcb): Change memfptr and memfloc to a pointer to struct
> staged_output_data.
> * strace.c (maybe_switch_tcbs): Use it.
> * stage_output.c (strace_open_memstream, strace_close_memstream): Ditto.

Nice catch, thanks!

> Signed-off-by: Pierre Marsais <pierre.marsais at lse.epita.fr>
> ---
> Hi strace developers,
> 
> Running the tests with valgrind seems to show some errors:
> 
> ```
> $ TESTS=status-none-threads.test make -e check-valgrind-memcheck
> $ head -n40  tests/status-none-threads.log
> ==25966== Invalid free() / delete / delete[] / realloc()
> ==25966==    at 0x48399AB: free (vg_replace_malloc.c:530)
> ==25966==    by 0x1CD397: strace_close_memstream (stage_output.c:60)
> ==25966==    by 0x1D0E50: droptcb (strace.c:814)
> ==25966==    by 0x1D35F0: maybe_switch_tcbs (strace.c:2116)
> ==25966==    by 0x1D323D: maybe_switch_current_tcp (strace.c:2139)
> ==25966==    by 0x1CFBA5: dispatch_event (strace.c:2694)
> ==25966==    by 0x1CECCC: main (strace.c:2889)
> ==25966==  Address 0x500acd0 is 0 bytes inside a block of size 8,192 free'd
> ==25966==    at 0x483AD7B: realloc (vg_replace_malloc.c:826)
> ==25966==    by 0x495D457: _IO_mem_finish (in /usr/lib/libc-2.29.so)
> ==25966==    by 0x4954870: fclose@@GLIBC_2.2.5 (in /usr/lib/libc-2.29.so)
> ==25966==    by 0x1CD2EC: strace_close_memstream (stage_output.c:50)
> ==25966==    by 0x1D0E50: droptcb (strace.c:814)
> ==25966==    by 0x1D35F0: maybe_switch_tcbs (strace.c:2116)
> ==25966==    by 0x1D323D: maybe_switch_current_tcp (strace.c:2139)
> ==25966==    by 0x1CFBA5: dispatch_event (strace.c:2694)
> ==25966==    by 0x1CECCC: main (strace.c:2889)
> ==25966==  Block was alloc'd at
> ==25966==    at 0x483AB65: calloc (vg_replace_malloc.c:752)
> ==25966==    by 0x495D4E7: open_memstream (in /usr/lib/libc-2.29.so)
> ==25966==    by 0x1CD239: strace_open_memstream (stage_output.c:24)
> ==25966==    by 0x1D4B71: syscall_entering_trace (syscall.c:654)
> ==25966==    by 0x1D2F4A: trace_syscall (strace.c:2598)
> ==25966==    by 0x1CFAAE: dispatch_event (strace.c:2637)
> ==25966==    by 0x1CECCC: main (strace.c:2889)
> ==25966==
> ==25966== 95 bytes in 1 blocks are definitely lost in loss record 8 of 12
> ==25966==    at 0x483AD7B: realloc (vg_replace_malloc.c:826)
> ==25966==    by 0x495D457: _IO_mem_finish (in /usr/lib/libc-2.29.so)
> ==25966==    by 0x4954870: fclose@@GLIBC_2.2.5 (in /usr/lib/libc-2.29.so)
> ==25966==    by 0x1CD2EC: strace_close_memstream (stage_output.c:50)
> ==25966==    by 0x1D0E50: droptcb (strace.c:814)
> ==25966==    by 0x1D35F0: maybe_switch_tcbs (strace.c:2116)
> ==25966==    by 0x1D323D: maybe_switch_current_tcp (strace.c:2139)
> ==25966==    by 0x1CFBA5: dispatch_event (strace.c:2694)
> ==25966==    by 0x1CECCC: main (strace.c:2889)
> ==25966==
> 25969 +++ superseded by execve in pid 25970 +++
> 25969 +++ exited with 0 +++
> ```
> 
> Here is a patch that should solve this problem.
> 
> Questions:
>  - Would it make sense to move real_outf in struct staged_output_data ?

Yes, I think it makes sense.  This would decrease the size of struct tcb
by one pointer, and the code would look more consistent with real_outf
checks replaced with staged_output_data checks.


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20190804/103eb981/attachment.bin>