From 7df9a0049e31fdbe3fb14cf5d52a445c7ec2769d Mon Sep 17 00:00:00 2001 From: Pierre Marsais Date: Sun, 4 Aug 2019 16:39:29 +0100 Subject: [PATCH] Fix invalid free in trace_close_memstream 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. * stage_output.c (struct staged_output_data): New struct. (strace_open_memstream, strace_close_memstream): Use it. * defs.h (struct tcb): Replace real_outf, memfptr, and memfloc with a pointer to struct staged_output_data. * strace.c (maybe_switch_tcbs): Use it. * syscall.c (print_syscall_resume): Ditto. Signed-off-by: Pierre Marsais --- defs.h | 4 +--- stage_output.c | 33 ++++++++++++++++++++++----------- strace.c | 21 +++++++-------------- syscall.c | 2 +- 4 files changed, 31 insertions(+), 29 deletions(-) diff --git a/defs.h b/defs.h index 51622c05..a9ef361a 100644 --- a/defs.h +++ b/defs.h @@ -204,9 +204,7 @@ struct tcb { int sys_func_rval; /* Syscall entry parser's return value */ 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..d9d0352b 100644 --- a/stage_output.c +++ b/stage_output.c @@ -14,14 +14,21 @@ #include "defs.h" +struct staged_output_data { + char *memfptr; + size_t memfloc; + FILE *real_outf; /* Backup for real outf while staging */ +}; + FILE * 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 = xmalloc(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"); /* @@ -31,7 +38,7 @@ strace_open_memstream(struct tcb *tcp) fflush(fp); /* Store the FILE pointer for later restauration. */ - tcp->real_outf = tcp->outf; + tcp->staged_output_data->real_outf = tcp->outf; tcp->outf = fp; #endif @@ -42,7 +49,7 @@ void strace_close_memstream(struct tcb *tcp, bool publish) { #if HAVE_OPEN_MEMSTREAM - if (!tcp->real_outf) { + if (!tcp->staged_output_data) { debug_msg("memstream already closed"); return; } @@ -50,15 +57,19 @@ strace_close_memstream(struct tcb *tcp, bool publish) if (fclose(tcp->outf)) perror_msg("fclose(tcp->outf)"); - tcp->outf = tcp->real_outf; - tcp->real_outf = NULL; - if (tcp->memfptr) { + tcp->outf = tcp->staged_output_data->real_outf; + 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); + tcp->staged_output_data->memfptr = NULL; } + free(tcp->staged_output_data); + tcp->staged_output_data = NULL; #endif } diff --git a/strace.c b/strace.c index afa841a7..5e8b0417 100644 --- a/strace.c +++ b/strace.c @@ -604,7 +604,7 @@ printleader(struct tcb *tcp) if (printing_tcp) { set_current_tcp(printing_tcp); - if (tcp->real_outf == NULL && printing_tcp->curcol != 0 && + if (!tcp->staged_output_data && printing_tcp->curcol != 0 && (followfork < 2 || printing_tcp == tcp)) { /* * case 1: we have a shared log (i.e. not -ff), and last line @@ -2094,19 +2094,12 @@ maybe_switch_tcbs(struct tcb *tcp, const int pid) FILE *fp = execve_thread->outf; execve_thread->outf = tcp->outf; tcp->outf = fp; - if (execve_thread->real_outf || tcp->real_outf) { - char *memfptr; - size_t memfloc; - - 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; + if (execve_thread->staged_output_data || tcp->staged_output_data) { + struct staged_output_data *staged_output_data; + + 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 */ diff --git a/syscall.c b/syscall.c index 8203ef06..20d04d0d 100644 --- a/syscall.c +++ b/syscall.c @@ -715,7 +715,7 @@ print_syscall_resume(struct tcb *tcp) * "strace -ff -oLOG test/threaded_execve" corner case. * It's the only case when -ff mode needs reprinting. */ - if ((followfork < 2 && printing_tcp != tcp && tcp->real_outf == NULL) + if ((followfork < 2 && printing_tcp != tcp && !tcp->staged_output_data) || (tcp->flags & TCB_REPRINT)) { tcp->flags &= ~TCB_REPRINT; printleader(tcp); -- 2.40.0