[PATCH] Fix invalid free in trace_close_memstream
Pierre Marsais
pierre.marsais at lse.epita.fr
Sat Aug 3 22:19:03 UTC 2019
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.
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 ?
defs.h | 8 ++++++--
stage_output.c | 18 +++++++++++-------
strace.c | 13 +++++--------
3 files changed, 22 insertions(+), 17 deletions(-)
diff --git a/defs.h b/defs.h
index 51622c05..e8507066 100644
--- a/defs.h
+++ b/defs.h
@@ -187,6 +187,11 @@ struct inject_opts {
struct inject_data data;
};
+struct staged_output_data {
+ char *memfptr;
+ size_t memfloc;
+};
+
# define MAX_ERRNO_VALUE 4095
/* Trace Control Block */
@@ -205,8 +210,7 @@ struct tcb {
int curcol; /* Output column for this process */
FILE *outf; /* Output file for this process */
FILE *real_outf; /* Backup for real outf while staging */
- char *memfptr;
- size_t memfloc;
+ struct staged_output_data *staged_output_data;
const char *auxstr; /* Auxiliary info from syscall (see RVAL_STR) */
void *_priv_data; /* Private data for syscall decoding functions */
diff --git a/stage_output.c b/stage_output.c
index 971714b7..6d473a57 100644
--- a/stage_output.c
+++ b/stage_output.c
@@ -20,8 +20,9 @@ strace_open_memstream(struct tcb *tcp)
FILE *fp = NULL;
#if HAVE_OPEN_MEMSTREAM
- tcp->memfptr = NULL;
- fp = open_memstream(&tcp->memfptr, &tcp->memfloc);
+ tcp->staged_output_data = malloc(sizeof(*tcp->staged_output_data));
+ fp = open_memstream(&tcp->staged_output_data->memfptr,
+ &tcp->staged_output_data->memfloc);
if (!fp)
perror_msg_and_die("open_memstream");
/*
@@ -52,13 +53,16 @@ strace_close_memstream(struct tcb *tcp, bool publish)
tcp->outf = tcp->real_outf;
tcp->real_outf = NULL;
- if (tcp->memfptr) {
+ if (tcp->staged_output_data->memfptr) {
if (publish)
- fputs_unlocked(tcp->memfptr, tcp->outf);
+ fputs_unlocked(tcp->staged_output_data->memfptr,
+ tcp->outf);
else
- debug_msg("syscall output dropped: %s", tcp->memfptr);
- free(tcp->memfptr);
- tcp->memfptr = NULL;
+ debug_msg("syscall output dropped: %s",
+ tcp->staged_output_data->memfptr);
+ free(tcp->staged_output_data->memfptr);
+ free(tcp->staged_output_data);
+ tcp->staged_output_data = NULL;
}
#endif
}
diff --git a/strace.c b/strace.c
index afa841a7..7d2be20a 100644
--- a/strace.c
+++ b/strace.c
@@ -2095,18 +2095,15 @@ maybe_switch_tcbs(struct tcb *tcp, const int pid)
execve_thread->outf = tcp->outf;
tcp->outf = fp;
if (execve_thread->real_outf || tcp->real_outf) {
- char *memfptr;
- size_t memfloc;
+ struct staged_output_data *staged_output_data;
fp = execve_thread->real_outf;
execve_thread->real_outf = tcp->real_outf;
tcp->real_outf = fp;
- memfptr = execve_thread->memfptr;
- execve_thread->memfptr = tcp->memfptr;
- tcp->memfptr = memfptr;
- memfloc = execve_thread->memfloc;
- execve_thread->memfloc = tcp->memfloc;
- tcp->memfloc = memfloc;
+
+ staged_output_data = execve_thread->staged_output_data;
+ execve_thread->staged_output_data = tcp->staged_output_data;
+ tcp->staged_output_data = staged_output_data;
}
/* And their column positions */
--
2.22.0
More information about the Strace-devel
mailing list