[PATCH 0/7] trace-cmd: Fix misc issues uncoverd by static analysis

[PATCH 0/7] trace-cmd: Fix misc issues uncoverd by static analysis

[Jerome Marchand]

More issues were found by running static analysers on the code
of trace-cmd with openscanhub[1].

[1] https://fedoraproject.org/wiki/OpenScanHub

Jerome Marchand (7):
  trace-cmd lib: Close FDs in create_buffer_recorder_fd2 it allocation
    fails
  trace-cmd lib: Prevent a memory leak in tracecmd_tsync_with_guest()
  trace-cmd lib: Prevent a leaked FD in
    __tracecmd_create_buffer_recorder()
  trace-cmd lib: Prevent memory leak in tracecmd_msg_wait_for_cmd()
  trace-cmd sqlhist: Initialize name in trace_sqlhist()
  trace-cmd: Fix memory leak in stop_mapping_vcpus()
  trace-cmd record: Fix stdin redirection to /dev/null

 lib/trace-cmd/trace-msg.c      |  4 +++-
 lib/trace-cmd/trace-recorder.c | 19 ++++++++++---------
 lib/trace-cmd/trace-timesync.c |  1 +
 tracecmd/trace-record.c        |  2 +-
 tracecmd/trace-sqlhist.c       |  2 +-
 tracecmd/trace-tsync.c         |  1 +
 6 files changed, 17 insertions(+), 12 deletions(-)

-- 
2.47.0

[PATCH 1/7] trace-cmd lib: Close FDs in create_buffer_recorder_fd2 it allocation fails

[Jerome Marchand]

The behavior of create_buffer_recorder_fd2() wrt closing the file
descriptors is inconsistent. They aren't close if the function fails
early when allocating recorder, but they are closed in
tracecmd_free_recorder() if it fails later.

This cause use-after-free access when the caller tries to close the
FDs afterwards.

Always close the FDs in create_buffer_recorder_fd2() when it fails and
stop the caller to close them themselves.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 lib/trace-cmd/trace-recorder.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/lib/trace-cmd/trace-recorder.c b/lib/trace-cmd/trace-recorder.c
index 44f245d5..0413e529 100644
--- a/lib/trace-cmd/trace-recorder.c
+++ b/lib/trace-cmd/trace-recorder.c
@@ -114,8 +114,12 @@ create_buffer_recorder_fd2(int fd, int fd2, int cpu, unsigned flags,
 	bool nonblock = false;
 
 	recorder = malloc(sizeof(*recorder));
-	if (!recorder)
+	if (!recorder) {
+		close(fd);
+		if (fd2 != -1)
+			close(fd2);
 		return NULL;
+	}
 
 	recorder->flags = flags;
 
@@ -204,12 +208,8 @@ __tracecmd_create_buffer_recorder(const char *file, int cpu, unsigned flags,
 	}
 
 	recorder = create_buffer_recorder_fd2(fd, fd2, cpu, flags, instance, maxkb, tfd);
-	if (!recorder) {
-		close(fd);
+	if (!recorder)
 		unlink(file);
-		if (fd2 != -1)
-			close(fd2);
-	}
 
 	if (fd2 != -1) {
 		/* Unlink file2, we need to add everything to file at the end */
@@ -257,10 +257,9 @@ tracecmd_create_buffer_recorder_maxkb(const char *file, int cpu, unsigned flags,
 	free(file2);
 
 	return recorder;
- err2:
-	close(fd2);
  err:
 	close(fd);
+ err2:
 	unlink(file);
 	goto out;
 }
-- 
2.47.0

Re: [PATCH 1/7] trace-cmd lib: Close FDs in create_buffer_recorder_fd2 it allocation fails

[Steven Rostedt]

On Thu,  5 Dec 2024 15:44:33 +0100
"Jerome Marchand" <jmarchan@redhat.com> wrote:

> The behavior of create_buffer_recorder_fd2() wrt closing the file
> descriptors is inconsistent. They aren't close if the function fails
> early when allocating recorder, but they are closed in
> tracecmd_free_recorder() if it fails later.
> 
> This cause use-after-free access when the caller tries to close the
> FDs afterwards.
> 
> Always close the FDs in create_buffer_recorder_fd2() when it fails and
> stop the caller to close them themselves.
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> ---
>  lib/trace-cmd/trace-recorder.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-recorder.c b/lib/trace-cmd/trace-recorder.c
> index 44f245d5..0413e529 100644
> --- a/lib/trace-cmd/trace-recorder.c
> +++ b/lib/trace-cmd/trace-recorder.c
> @@ -114,8 +114,12 @@ create_buffer_recorder_fd2(int fd, int fd2, int cpu, unsigned flags,
>  	bool nonblock = false;
>  
>  	recorder = malloc(sizeof(*recorder));
> -	if (!recorder)
> +	if (!recorder) {
> +		close(fd);
> +		if (fd2 != -1)
> +			close(fd2);
>  		return NULL;
> +	}

No. If a function does not open a file descriptor, it should not close it.
It's bad programming style if a called function closes a file descriptor on
error that was passed to it. That can easily introduce new bugs.

The fix is to not have it close a file descriptor at all!

-- Steve

diff --git a/lib/trace-cmd/trace-recorder.c b/lib/trace-cmd/trace-recorder.c
index 0413e5293766..df971e4adf97 100644
--- a/lib/trace-cmd/trace-recorder.c
+++ b/lib/trace-cmd/trace-recorder.c
@@ -69,7 +69,7 @@ void tracecmd_free_recorder(struct tracecmd_recorder *recorder)
 	if (!recorder)
 		return;
 
-	if (recorder->max) {
+	if (recorder->max && recorder->fd >= 0) {
 		/* Need to put everything into fd1 */
 		if (recorder->fd == recorder->fd1) {
 			int ret;
@@ -140,14 +140,12 @@ create_buffer_recorder_fd2(int fd, int fd2, int cpu, unsigned flags,
 	recorder->count = 0;
 	recorder->pages = 0;
 
-	/* fd always points to what to write to */
-	recorder->fd = fd;
-	recorder->fd1 = fd;
-	recorder->fd2 = fd2;
-
 	if (recorder->flags & TRACECMD_RECORD_POLL)
 		nonblock = true;
 
+	/* In case of error */
+	recorder->fd = -1;
+
 	if (tfd >= 0)
 		recorder->tcpu = tracefs_cpu_alloc_fd(tfd, recorder->page_size, nonblock);
 	else
@@ -156,6 +154,11 @@ create_buffer_recorder_fd2(int fd, int fd2, int cpu, unsigned flags,
 	if (!recorder->tcpu)
 		goto out_free;
 
+	/* fd always points to what to write to */
+	recorder->fd = fd;
+	recorder->fd1 = fd;
+	recorder->fd2 = fd2;
+
 	recorder->subbuf_size = tracefs_cpu_read_size(recorder->tcpu);
 	return recorder;
 

Re: [PATCH 1/7] trace-cmd lib: Close FDs in create_buffer_recorder_fd2 it allocation fails

[Steven Rostedt]

On Wed, 18 Dec 2024 17:37:31 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> No. If a function does not open a file descriptor, it should not close it.
> It's bad programming style if a called function closes a file descriptor on
> error that was passed to it. That can easily introduce new bugs.
> 
> The fix is to not have it close a file descriptor at all!

And this means all the callers should close their fd if the recorder fails.
It should have never done that in the first place. :-/

Oh well, that was written over 10 years ago ;-)

-- Steve

[PATCH 2/7] trace-cmd lib: Prevent a memory leak in tracecmd_tsync_with_guest()

[Jerome Marchand]

Free tsync->proto_name in tracecmd_tsync_with_guest() error path.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 lib/trace-cmd/trace-timesync.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/trace-cmd/trace-timesync.c b/lib/trace-cmd/trace-timesync.c
index 6ee4e643..94f31bcd 100644
--- a/lib/trace-cmd/trace-timesync.c
+++ b/lib/trace-cmd/trace-timesync.c
@@ -805,6 +805,7 @@ tracecmd_tsync_with_guest(unsigned long long trace_id, int loop_interval,
 		tracecmd_msg_handle_close(tsync->msg_handle);
 	else if (fd >= 0)
 		close(fd);
+	free(tsync->proto_name);
 	free(tsync);
 
 	return NULL;
-- 
2.47.0

[PATCH 3/7] trace-cmd lib: Prevent a leaked FD in __tracecmd_create_buffer_recorder()

[Jerome Marchand]

Close fd if the allocation of file2 fails in
__tracecmd_create_buffer_recorder().

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 lib/trace-cmd/trace-recorder.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/trace-cmd/trace-recorder.c b/lib/trace-cmd/trace-recorder.c
index 0413e529..f02c3a55 100644
--- a/lib/trace-cmd/trace-recorder.c
+++ b/lib/trace-cmd/trace-recorder.c
@@ -193,8 +193,10 @@ __tracecmd_create_buffer_recorder(const char *file, int cpu, unsigned flags,
 		int len = strlen(file);
 
 		file2 = malloc(len + 3);
-		if (!file2)
+		if (!file2) {
+			close(fd);
 			return NULL;
+		}
 
 		sprintf(file2, "%s.1", file);
 
-- 
2.47.0

[PATCH 4/7] trace-cmd lib: Prevent memory leak in tracecmd_msg_wait_for_cmd()

[Jerome Marchand]

When tracecmd_msg_wait_for_cmd() returns successfully, msg->buf isn't
freed. Call msg_free() to free it.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 lib/trace-cmd/trace-msg.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/trace-cmd/trace-msg.c b/lib/trace-cmd/trace-msg.c
index f5c604f1..c7583587 100644
--- a/lib/trace-cmd/trace-msg.c
+++ b/lib/trace-cmd/trace-msg.c
@@ -1034,8 +1034,10 @@ static int tracecmd_msg_wait_for_cmd(struct tracecmd_msg_handle *msg_handle, enu
 		if (ret < 0)
 			goto error;
 
-		if (ntohl(msg.hdr.cmd) == cmd)
+		if (ntohl(msg.hdr.cmd) == cmd) {
+			msg_free(&msg);
 			return 0;
+		}
 
 		error_operation(&msg);
 		ret = handle_unexpected_msg(msg_handle, &msg);
-- 
2.47.0

[PATCH 5/7] trace-cmd sqlhist: Initialize name in trace_sqlhist()

[Jerome Marchand]

The variable name in trace_sqlhist() can be used uninitialized in
do_sql() if it's not set with -n option. Initialize it to NULL like
the other strings.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 tracecmd/trace-sqlhist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tracecmd/trace-sqlhist.c b/tracecmd/trace-sqlhist.c
index 9e304e43..9f2b94e6 100644
--- a/tracecmd/trace-sqlhist.c
+++ b/tracecmd/trace-sqlhist.c
@@ -136,7 +136,7 @@ void trace_sqlhist (int argc, char **argv)
 	const char *instance = NULL;
 	bool execute = false;
 	char **save_fields = NULL;
-	const char *name;
+	const char *name = NULL;
 	const char *var = NULL;
 	char **save_argv;
 	int action = 0;
-- 
2.47.0

[PATCH 6/7] trace-cmd: Fix memory leak in stop_mapping_vcpus()

[Jerome Marchand]

In stop_mapping_vcpus(), tmap.vcpu is allocated in map_kvm_vcpus() but
not freed in the error path.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 tracecmd/trace-tsync.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tracecmd/trace-tsync.c b/tracecmd/trace-tsync.c
index 57baff39..834b4da4 100644
--- a/tracecmd/trace-tsync.c
+++ b/tracecmd/trace-tsync.c
@@ -258,6 +258,7 @@ static void stop_mapping_vcpus(int cpu_count, struct trace_guest *guest)
  out_free:
 	tep_free(tep);
  out:
+	free(tmap.vcpu);
 	free(tmap.map);
 	tracefs_instance_destroy(guest->instance);
 	tracefs_instance_free(guest->instance);
-- 
2.47.0

[PATCH 7/7] trace-cmd record: Fix stdin redirection to /dev/null

[Jerome Marchand]

In daemonize_start(), stdin file descriptor is closed after the call
to dup2. This doesn't make sense and could lead to FD zero being
reused. I assume the original intend was to close the devnull FD which
in the current code remains opened while the devnull variable goes out
of scope.

Close devnull instead of 0.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 tracecmd/trace-record.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 6e9b4535..fe9ceaa8 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -1677,7 +1677,7 @@ static void daemonize_start(void)
 		if (devnull > 0) {
 			if (dup2(devnull, 0) == -1)
 				die("daemonize: dup2");
-			close(0);
+			close(devnull);
 		}
 
 		return;
-- 
2.47.0

Re: [PATCH 0/7] trace-cmd: Fix misc issues uncoverd by static analysis

[Steven Rostedt]

On Thu,  5 Dec 2024 15:44:32 +0100
"Jerome Marchand" <jmarchan@redhat.com> wrote:

> More issues were found by running static analysers on the code
> of trace-cmd with openscanhub[1].
> 
> [1] https://fedoraproject.org/wiki/OpenScanHub
> 
> Jerome Marchand (7):
>   trace-cmd lib: Close FDs in create_buffer_recorder_fd2 it allocation
>     fails
>   trace-cmd lib: Prevent a memory leak in tracecmd_tsync_with_guest()
>   trace-cmd lib: Prevent a leaked FD in
>     __tracecmd_create_buffer_recorder()
>   trace-cmd lib: Prevent memory leak in tracecmd_msg_wait_for_cmd()
>   trace-cmd sqlhist: Initialize name in trace_sqlhist()
>   trace-cmd: Fix memory leak in stop_mapping_vcpus()
>   trace-cmd record: Fix stdin redirection to /dev/null
> 

I'm going to apply all your patches except the two that deal with file
descriptors. The functions should not be closing file descriptors that they
did not open. The real fix there is to remove where it does close the file
descriptors and move the closing in the error paths of the callers.

I must have gotten lazy and just let the functions do the closing when they
failed, but that is just prone to bugs.

-- Steve


>  lib/trace-cmd/trace-msg.c      |  4 +++-
>  lib/trace-cmd/trace-recorder.c | 19 ++++++++++---------
>  lib/trace-cmd/trace-timesync.c |  1 +
>  tracecmd/trace-record.c        |  2 +-
>  tracecmd/trace-sqlhist.c       |  2 +-
>  tracecmd/trace-tsync.c         |  1 +
>  6 files changed, 17 insertions(+), 12 deletions(-)
> 

Re: [PATCH 0/7] trace-cmd: Fix misc issues uncoverd by static analysis

[Jerome Marchand]

On 18/12/2024 23:53, Steven Rostedt wrote:
> On Thu,  5 Dec 2024 15:44:32 +0100
> "Jerome Marchand" <jmarchan@redhat.com> wrote:
> 
>> More issues were found by running static analysers on the code
>> of trace-cmd with openscanhub[1].
>>
>> [1] https://fedoraproject.org/wiki/OpenScanHub
>>
>> Jerome Marchand (7):
>>    trace-cmd lib: Close FDs in create_buffer_recorder_fd2 it allocation
>>      fails
>>    trace-cmd lib: Prevent a memory leak in tracecmd_tsync_with_guest()
>>    trace-cmd lib: Prevent a leaked FD in
>>      __tracecmd_create_buffer_recorder()
>>    trace-cmd lib: Prevent memory leak in tracecmd_msg_wait_for_cmd()
>>    trace-cmd sqlhist: Initialize name in trace_sqlhist()
>>    trace-cmd: Fix memory leak in stop_mapping_vcpus()
>>    trace-cmd record: Fix stdin redirection to /dev/null
>>
> 
> I'm going to apply all your patches except the two that deal with file
> descriptors. The functions should not be closing file descriptors that they
> did not open. The real fix there is to remove where it does close the file
> descriptors and move the closing in the error paths of the callers.
> 
> I must have gotten lazy and just let the functions do the closing when they
> failed, but that is just prone to bugs.

That makes sense. I'll send updated fixes after the holidays break.

Thanks,
Jerome

> 
> -- Steve
> 
> 
>>   lib/trace-cmd/trace-msg.c      |  4 +++-
>>   lib/trace-cmd/trace-recorder.c | 19 ++++++++++---------
>>   lib/trace-cmd/trace-timesync.c |  1 +
>>   tracecmd/trace-record.c        |  2 +-
>>   tracecmd/trace-sqlhist.c       |  2 +-
>>   tracecmd/trace-tsync.c         |  1 +
>>   6 files changed, 17 insertions(+), 12 deletions(-)
>>
>