[PATCH v2] Fix invalid free in trace_close_memstream

[Pierre Marsais]

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): Change memfptr and memfloc to 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 <pierre.marsais at lse.epita.fr>
---
Hi,

Here is a second version fixing some small oversights.

Changes in v2:
 - use xmalloc instead of malloc
 - move real_outf inside struct staged_output_data
 - make struct staged_output_data opaque
 - If for some reason tcp->staged_output_data->memfptr == NULL in
   strace_close_memstream, we wouldn't free(tcp->staged_output_data)
   (can it happen ?). Fix this behaviour.

 defs.h         |  4 +---
 stage_output.c | 32 +++++++++++++++++++++-----------
 strace.c       | 21 +++++++--------------
 syscall.c      |  2 +-
 4 files changed, 30 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..729a73bb 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,18 @@ 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);
 	}
+	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.22.0