[PATCH 0/3] trace-cmd: Fix the memory mangement of trace-cmd agent.

[PATCH 0/3] trace-cmd: Fix the memory mangement of trace-cmd agent.

[Steven Rostedt]

When testing out the time sync logic, the agent crashed due to
various memory management issues.

Steven Rostedt (VMware) (3):
      trace-cmd agent: Clean up tsync_agent_thread() and tsync_host_thread()
      trace-cmd: Have tracecmd_tsync_free() free even with no context
      trace-cmd: Do not free tsync from thread

----
 lib/trace-cmd/trace-timesync.c | 49 +++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

[PATCH 1/3] trace-cmd agent: Clean up tsync_agent_thread() and tsync_host_thread()

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

There's no reason to assign tsync to NULL nor add a typecast to data, as
void pointers naturally can be converted to any type.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 lib/trace-cmd/trace-timesync.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/lib/trace-cmd/trace-timesync.c b/lib/trace-cmd/trace-timesync.c
index ad73f08f7070..4ed283eb1fb8 100644
--- a/lib/trace-cmd/trace-timesync.c
+++ b/lib/trace-cmd/trace-timesync.c
@@ -697,9 +697,8 @@ static int tsync_with_guest(struct tracecmd_time_sync *tsync)
 
 static void *tsync_host_thread(void *data)
 {
-	struct tracecmd_time_sync *tsync = NULL;
+	struct tracecmd_time_sync *tsync = data;
 
-	tsync = (struct tracecmd_time_sync *)data;
 	tsync_with_guest(tsync);
 	tracecmd_msg_handle_close(tsync->msg_handle);
 	tsync->msg_handle = NULL;
@@ -865,11 +864,9 @@ int tracecmd_tsync_with_guest_stop(struct tracecmd_time_sync *tsync)
 
 static void *tsync_agent_thread(void *data)
 {
-	struct tracecmd_time_sync *tsync = NULL;
+	struct tracecmd_time_sync *tsync = data;
 	int sd;
 
-	tsync = (struct tracecmd_time_sync *)data;
-
 	while (true) {
 		sd = accept(tsync->msg_handle->fd, NULL, NULL);
 		if (sd < 0) {
-- 
2.30.1

[PATCH 2/3] trace-cmd: Have tracecmd_tsync_free() free even with no context

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

tracecmd_tsync_free() incorrectly exits the function if the tsync has no
context. But it may still need to free up the resources in that case. Do
not leak memory if the context failed to be created.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 lib/trace-cmd/trace-timesync.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/lib/trace-cmd/trace-timesync.c b/lib/trace-cmd/trace-timesync.c
index 4ed283eb1fb8..24984fb17dab 100644
--- a/lib/trace-cmd/trace-timesync.c
+++ b/lib/trace-cmd/trace-timesync.c
@@ -513,25 +513,28 @@ void tracecmd_tsync_free(struct tracecmd_time_sync *tsync)
 	struct clock_sync_context *tsync_context;
 	struct tsync_proto *proto;
 
-	if (!tsync || !tsync->context)
+	if (!tsync)
 		return;
+
 	tsync_context = (struct clock_sync_context *)tsync->context;
 
 	proto = tsync_proto_find(tsync->proto_name);
 	if (proto && proto->clock_sync_free)
 		proto->clock_sync_free(tsync);
 
-	clock_synch_delete_instance(tsync_context->instance);
-	tsync_context->instance = NULL;
-
-	free(tsync_context->sync_ts);
-	free(tsync_context->sync_offsets);
-	free(tsync_context->sync_scalings);
-	tsync_context->sync_ts = NULL;
-	tsync_context->sync_offsets = NULL;
-	tsync_context->sync_scalings = NULL;
-	tsync_context->sync_count = 0;
-	tsync_context->sync_size = 0;
+	if (tsync_context) {
+		clock_synch_delete_instance(tsync_context->instance);
+		tsync_context->instance = NULL;
+
+		free(tsync_context->sync_ts);
+		free(tsync_context->sync_offsets);
+		free(tsync_context->sync_scalings);
+		tsync_context->sync_ts = NULL;
+		tsync_context->sync_offsets = NULL;
+		tsync_context->sync_scalings = NULL;
+		tsync_context->sync_count = 0;
+		tsync_context->sync_size = 0;
+	}
 	pthread_mutex_destroy(&tsync->lock);
 	pthread_cond_destroy(&tsync->cond);
 	pthread_barrier_destroy(&tsync->first_sync);
-- 
2.30.1

[PATCH 3/3] trace-cmd: Do not free tsync from thread

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

The freeing of an object should happen in the same context as where it is
created. The tsync object is created in the main thread and passed to the
guest and host handling threads. It should be closed and freed by the thread
that created it, especially since that thread accesses the tsync object
after creating the thread. Having the created thread free it would cause all
sorts of dangerous race conditions.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 lib/trace-cmd/trace-timesync.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/lib/trace-cmd/trace-timesync.c b/lib/trace-cmd/trace-timesync.c
index 24984fb17dab..ba877f701c85 100644
--- a/lib/trace-cmd/trace-timesync.c
+++ b/lib/trace-cmd/trace-timesync.c
@@ -535,6 +535,10 @@ void tracecmd_tsync_free(struct tracecmd_time_sync *tsync)
 		tsync_context->sync_count = 0;
 		tsync_context->sync_size = 0;
 	}
+
+	if (tsync->msg_handle)
+		tracecmd_msg_handle_close(tsync->msg_handle);
+
 	pthread_mutex_destroy(&tsync->lock);
 	pthread_cond_destroy(&tsync->cond);
 	pthread_barrier_destroy(&tsync->first_sync);
@@ -703,9 +707,6 @@ static void *tsync_host_thread(void *data)
 	struct tracecmd_time_sync *tsync = data;
 
 	tsync_with_guest(tsync);
-	tracecmd_msg_handle_close(tsync->msg_handle);
-	tsync->msg_handle = NULL;
-
 	pthread_exit(0);
 }
 
@@ -868,6 +869,7 @@ int tracecmd_tsync_with_guest_stop(struct tracecmd_time_sync *tsync)
 static void *tsync_agent_thread(void *data)
 {
 	struct tracecmd_time_sync *tsync = data;
+	long ret = 0;
 	int sd;
 
 	while (true) {
@@ -875,6 +877,7 @@ static void *tsync_agent_thread(void *data)
 		if (sd < 0) {
 			if (errno == EINTR)
 				continue;
+			ret = -1;
 			goto out;
 		}
 		break;
@@ -885,11 +888,7 @@ static void *tsync_agent_thread(void *data)
 	tsync_with_host(tsync);
 
 out:
-	tracecmd_msg_handle_close(tsync->msg_handle);
-	tracecmd_tsync_free(tsync);
-	close(sd);
-
-	pthread_exit(0);
+	pthread_exit((void *)ret);
 }
 
 /**
-- 
2.30.1