[PATCH 00/38] trace-cmd: fix misc issues found by static analysis

[PATCH 00/38] trace-cmd: fix misc issues found by static analysis

[Jerome Marchand]

A number of issues were found by running static analysers on the code
of trace-cmd with openscanhub[1]. Mostly ressource leak, but some are
more serious like memory corruption.

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

Jerome Marchand (38):
  trace-cmd listen: close ofd before exiting process_client()
  trace-cmd msg: prevent a memory leak in get_trace_req_args()
  trace-cmd lib: prevent a memory leak in read_header_files()
  trace-cmd: call dlclose() in the error path of load_plugin()
  trace-cmd lib: prevent possible memory coruption in add_plugin_file()
  trace-cmd lib: prevent a memory leak in handle_options()
  trace-cmd lib: prevent a memory leak in regex_event_buf()
  trace-cmd lib: prevent a memory leak in create_event_list_item()
  trace-cmd lib: prevent a memory leak in read_ftrace_printk()
  trace-cmd: don't print a NULL string in append_pid_filter()
  trace-cmd record: prevent possible memory coruption in
    get_pid_addr_maps()
  trace-cmd hist: close tracecmd handle when trace_hist() exits early
  trace-cmd record: prevent a memory leak in show_error()
  trace-cmd record: prevent memory leak in update_pid_filters()
  trace-cmd lib: check the return value of do_lssek() in
    trace_get_options()
  trace-cmd lib: don't double close a file descriptor in
    read_header_files()
  trace-cmd lib: prevent memory leak in ptp_clock_server()
  trace-cmd lib: remove useless code in tracecmd_plog()
  trace-cmd record: prevent memory leak in add_all_instances()
  trace-cmd lib: check for a negative return value of read in
    tracecmd_compress_copy_from()
  trace-cmd record: prevent memory leak in clear_func_filter()
  trace-cmd dump: prevent buffer overrun in dump_clock()
  trace-cmd lib: prevent buffer overrun in read_string()
  trace-cmd: close file descriptor in trace_vsock_make()
  trace-cmd lib: prevent memory leak in glob_events()
  trace-cmd record: don't print a NULL string in get_temp_file()
  trace-cmd lib: prevent a possible file descriptor leak in
    set_proc_kptr_restrict()
  trace-cmd lib: remove unused tracecmd_parse_cmdlines() function
  trace-cmd record: prevent memory leak in setup_network()
  trace-cmd listen: prevent memory leak in communicate_with_client()
  trace-cmd listen: prevent a infinite loop in communicate_with_client()
  trace-cmd lib: prevent memory leak in tracecmd_create_event_hook()
  trace-cmd record: prevent memory corruption in parse_record_options()
  trace-cmd mem: prevent a memory leak in trace_mem()
  trace-cmd: move the initialization of found_pid at the beginning of
    stop_trace_connect()
  trace-cmd record: check the length of the protocol version received
  trace-cmd record: close socket fd before retrying to connect
  trace-cmd lib: prevent a memory leak in tracecmd_tsync_proto_getall()

 lib/trace-cmd/trace-compress.c     |  4 +--
 lib/trace-cmd/trace-hooks.c        |  1 +
 lib/trace-cmd/trace-input.c        | 17 +++++++------
 lib/trace-cmd/trace-msg.c          | 13 ++--------
 lib/trace-cmd/trace-output.c       | 39 +++++++++++++++++++-----------
 lib/trace-cmd/trace-plugin.c       |  8 +++---
 lib/trace-cmd/trace-timesync-ptp.c |  4 ++-
 lib/trace-cmd/trace-timesync.c     |  2 +-
 lib/trace-cmd/trace-util.c         | 26 ++------------------
 tracecmd/trace-dump.c              |  2 +-
 tracecmd/trace-hist.c              |  4 ++-
 tracecmd/trace-listen.c            | 13 +++++++---
 tracecmd/trace-mem.c               |  3 ++-
 tracecmd/trace-read.c              |  2 +-
 tracecmd/trace-record.c            | 19 +++++++++++----
 tracecmd/trace-vm.c                |  2 +-
 tracecmd/trace-vsock.c             |  8 ++++--
 17 files changed, 87 insertions(+), 80 deletions(-)

-- 
2.44.0

[PATCH 01/38] trace-cmd listen: close ofd before exiting process_client()

[Jerome Marchand]

Close ofd in the error path and at the end of the function.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/tracecmd/trace-listen.c b/tracecmd/trace-listen.c
index 5894a92d..09f442c7 100644
--- a/tracecmd/trace-listen.c
+++ b/tracecmd/trace-listen.c
@@ -696,8 +696,10 @@ static int process_client(struct tracecmd_msg_handle *msg_handle,
 	ofd = create_client_file(node, port);
 
 	pid_array = create_all_readers(node, port, pagesize, msg_handle);
-	if (!pid_array)
+	if (!pid_array) {
+		close(ofd);
 		return -ENOMEM;
+	}
 
 	/* on signal stop this msg */
 	stop_msg_handle = msg_handle;
@@ -725,6 +727,7 @@ static int process_client(struct tracecmd_msg_handle *msg_handle,
 					msg_handle->version < V3_PROTOCOL);
 
 	destroy_all_readers(cpus, pid_array, node, port);
+	close(ofd);
 
 	return ret;
 }
-- 
2.44.0

[PATCH 02/38] trace-cmd msg: prevent a memory leak in get_trace_req_args()

[Jerome Marchand]

In get_trace_req_args() vagrs is not freed when the function exits
without an error. This could be of course be fixed by freeing vagrs
before exit, but actually I don't see the point of the buffer at all
since it just use to copy the content of buf and then read to fill
args. Why not just read from buf to begin with?

Remove vagrs and use buf directly.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/lib/trace-cmd/trace-msg.c b/lib/trace-cmd/trace-msg.c
index 3a555c36..f5c604f1 100644
--- a/lib/trace-cmd/trace-msg.c
+++ b/lib/trace-cmd/trace-msg.c
@@ -1247,7 +1247,6 @@ static int get_trace_req_args(char *buf, int length, int *argc, char ***argv)
 	unsigned int nr_args;
 	char *p, *buf_end;
 	char **args = NULL;
-	char *vagrs = NULL;
 	int ret;
 	int i;
 
@@ -1266,15 +1265,8 @@ static int get_trace_req_args(char *buf, int length, int *argc, char ***argv)
 		goto out;
 	}
 
-	vagrs = calloc(length, sizeof(char));
-	if (!vagrs) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	memcpy(vagrs, buf, length);
-	buf_end = vagrs + length;
-	for (i = 0, p = vagrs; i < nr_args; i++, p++) {
+	buf_end = buf + length;
+	for (i = 0, p = buf; i < nr_args; i++, p++) {
 		if (p >= buf_end) {
 			ret = -EINVAL;
 			goto out;
@@ -1289,7 +1281,6 @@ static int get_trace_req_args(char *buf, int length, int *argc, char ***argv)
 
 out:
 	free(args);
-	free(vagrs);
 	return ret;
 
 }
-- 
2.44.0

[PATCH 03/38] trace-cmd lib: prevent a memory leak in read_header_files()

[Jerome Marchand]

In read_header_files() path is allocated a few times by
get_tracing_file(). It isn't freed in multiple error paths. Free it
whenever needed.

Fixes a RESOURCE_LEAK error (CWE-772)

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 lib/trace-cmd/trace-output.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index eee847e3..59498edc 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -644,8 +644,10 @@ static int read_header_files(struct tracecmd_output *handle, bool compress)
 		flags |= TRACECMD_SEC_FL_COMPRESS;
 	offset = out_write_section_header(handle, TRACECMD_OPTION_HEADER_INFO,
 					  "headers", flags, true);
-	if (offset == (off_t)-1)
+	if (offset == (off_t)-1) {
+		put_tracing_file(path);
 		return -1;
+	}
 
 	out_compression_start(handle, compress);
 	ret = stat(path, &st);
@@ -671,23 +673,23 @@ static int read_header_files(struct tracecmd_output *handle, bool compress)
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
 		tracecmd_warning("can't read '%s'", path);
-		goto out_close;
+		goto out_free;
 	}
 
 	/* unfortunately, you can not stat debugfs files for size */
 	size = get_size_fd(fd);
 
 	if (do_write_check(handle, "header_page", 12))
-		goto out_close;
+		goto out_free;
 	endian8 = convert_endian_8(handle, size);
 	if (do_write_check(handle, &endian8, 8))
-		goto out_close;
+		goto out_free;
 	check_size = copy_file_fd(handle, fd, 0);
 	close(fd);
 	if (size != check_size) {
 		tracecmd_warning("wrong size for '%s' size=%lld read=%lld", path, size, check_size);
 		errno = EINVAL;
-		goto out_close;
+		goto out_free;
 	}
 	put_tracing_file(path);
 
@@ -698,21 +700,21 @@ static int read_header_files(struct tracecmd_output *handle, bool compress)
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
 		tracecmd_warning("can't read '%s'", path);
-		goto out_close;
+		goto out_free;
 	}
 
 	size = get_size_fd(fd);
 
 	if (do_write_check(handle, "header_event", 13))
-		goto out_close;
+		goto out_free;
 	endian8 = convert_endian_8(handle, size);
 	if (do_write_check(handle, &endian8, 8))
-		goto out_close;
+		goto out_free;
 	check_size = copy_file_fd(handle, fd, 0);
 	close(fd);
 	if (size != check_size) {
 		tracecmd_warning("wrong size for '%s'", path);
-		goto out_close;
+		goto out_free;
 	}
 	put_tracing_file(path);
 	if (out_compression_end(handle, compress))
@@ -724,6 +726,8 @@ static int read_header_files(struct tracecmd_output *handle, bool compress)
 
 	return 0;
 
+ out_free:
+	put_tracing_file(path);
  out_close:
 	out_compression_reset(handle, compress);
 	if (fd >= 0)
-- 
2.44.0

[PATCH 04/38] trace-cmd: call dlclose() in the error path of load_plugin()

[Jerome Marchand]

The handle returned by dlopen() isn't closes if something else fails
afterwards. Call dlclose in the error path to fix that.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/lib/trace-cmd/trace-plugin.c b/lib/trace-cmd/trace-plugin.c
index 127771ea..0d477dff 100644
--- a/lib/trace-cmd/trace-plugin.c
+++ b/lib/trace-cmd/trace-plugin.c
@@ -126,13 +126,13 @@ load_plugin(struct trace_plugin_context *trace, const char *path,
 	if (!func) {
 		tracecmd_warning("could not find func '%s' in plugin '%s'\n%s",
 				 TRACECMD_PLUGIN_LOADER_NAME, plugin, dlerror());
-		goto out_free;
+		goto out_close;
 	}
 
 	list = malloc(sizeof(*list));
 	if (!list) {
 		tracecmd_warning("could not allocate plugin memory");
-		goto out_free;
+		goto out_close;
 	}
 
 	list->next = *plugin_list;
@@ -143,7 +143,9 @@ load_plugin(struct trace_plugin_context *trace, const char *path,
 	tracecmd_info("registering plugin: %s", plugin);
 	func(trace);
 	return;
-
+	
+out_close:
+	dlclose(handle);
  out_free:
 	free(plugin);
 }
-- 
2.44.0

[PATCH 05/38] trace-cmd lib: prevent possible memory coruption in add_plugin_file()

[Jerome Marchand]

If strdup() fails the error path access original address of
pdata->files after it has been dereferenced. Make sure that
pdata->files contains the up-to-date pointer before calling calling a
function that could fail.

This was flagged as ressource leak (CWE-772) because ptr isn't freed
in that scenario, but there is something worse going on that the
static analysis missed.

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

diff --git a/lib/trace-cmd/trace-util.c b/lib/trace-cmd/trace-util.c
index b5002f1d..47ca3db1 100644
--- a/lib/trace-cmd/trace-util.c
+++ b/lib/trace-cmd/trace-util.c
@@ -279,11 +279,11 @@ static void add_plugin_file(struct tep_handle *pevent, const char *path,
 	if (!ptr)
 		goto out_free;
 
+	pdata->files = ptr;
 	ptr[pdata->index] = strdup(name);
 	if (!ptr[pdata->index])
 		goto out_free;
 
-	pdata->files = ptr;
 	pdata->index++;
 	pdata->files[pdata->index] = NULL;
 	return;
-- 
2.44.0

[PATCH 06/38] trace-cmd lib: prevent a memory leak in handle_options()

[Jerome Marchand]

Free buf in the error path.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index ce4ecf43..2cf0d1c1 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -4030,7 +4030,7 @@ static int handle_options(struct tracecmd_input *handle)
 		}
 		ret = do_read_check(handle, buf, size);
 		if (ret)
-			goto out;
+			goto out_free;
 
 		switch (option) {
 		case TRACECMD_OPTION_DATE:
@@ -4084,7 +4084,7 @@ static int handle_options(struct tracecmd_input *handle)
 							     buf + 8, 4);
 			ret = tsync_cpu_offsets_load(handle, buf + 12, size - 12);
 			if (ret < 0)
-				goto out;
+				goto out_free;
 			tracecmd_enable_tsync(handle, true);
 			break;
 		case TRACECMD_OPTION_CPUSTAT:
@@ -4093,7 +4093,7 @@ static int handle_options(struct tracecmd_input *handle)
 					   handle->cpustats_size + size + 1);
 			if (!cpustats) {
 				ret = -ENOMEM;
-				goto out;
+				goto out_free;
 			}
 			memcpy(cpustats + handle->cpustats_size, buf, size);
 			handle->cpustats_size += size;
@@ -4104,7 +4104,7 @@ static int handle_options(struct tracecmd_input *handle)
 		case TRACECMD_OPTION_BUFFER_TEXT:
 			ret = handle_buffer_option(handle, option, buf, size);
 			if (ret < 0)
-				goto out;
+				goto out_free;
 			break;
 		case TRACECMD_OPTION_TRACECLOCK:
 			tracecmd_parse_trace_clock(handle, buf, size);
@@ -4183,6 +4183,8 @@ static int handle_options(struct tracecmd_input *handle)
 
 	ret = 0;
 
+out_free:
+	free(buf);
 out:
 	if (compress)
 		in_uncompress_reset(handle);
-- 
2.44.0

[PATCH 07/38] trace-cmd lib: prevent a memory leak in regex_event_buf()

[Jerome Marchand]

Free buf before exiting if strtok fails.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 2cf0d1c1..3284dbd4 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -666,6 +666,7 @@ static int regex_event_buf(const char *file, int size, regex_t *epreg)
 	line = strtok(buf, "\n");
 	if (!line) {
 		tracecmd_warning("No newline found in '%s'", buf);
+		free(buf);
 		return 0;
 	}
 	/* skip name if it is there */
-- 
2.44.0

[PATCH 08/38] trace-cmd lib: prevent a memory leak in create_event_list_item()

[Jerome Marchand]

Free ptr if malloc() fails.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 59498edc..4a9ecc5d 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -955,7 +955,8 @@ create_event_list_item(struct tracecmd_output *handle,
 	free(str);
 	return;
  err_mem:
-	 tracecmd_warning("Insufficient memory");
+	free(ptr);
+	tracecmd_warning("Insufficient memory");
 }
 
 static int read_ftrace_files(struct tracecmd_output *handle, bool compress)
-- 
2.44.0

[PATCH 09/38] trace-cmd lib: prevent a memory leak in read_ftrace_printk()

[Jerome Marchand]

Free path if out_write_section_header() fails.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 4a9ecc5d..5ba0a145 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -1214,8 +1214,10 @@ static int read_ftrace_printk(struct tracecmd_output *handle, bool compress)
 	if (compress)
 		flags |= TRACECMD_SEC_FL_COMPRESS;
 	offset = out_write_section_header(handle, TRACECMD_OPTION_PRINTK, "printk", flags, true);
-	if (offset == (off_t)-1)
+	if (offset == (off_t)-1) {
+		put_tracing_file(path);
 		return -1;
+	}
 
 	out_compression_start(handle, compress);
 	ret = stat(path, &st);
-- 
2.44.0

[PATCH 10/38] trace-cmd: don't print a NULL string in append_pid_filter()

[Jerome Marchand]

In append_pid_filter() a NULL pointer is passed as the argument for a
%s format. Change the error message to what is used in trace-record.c
append_pid_filter.

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

diff --git a/tracecmd/trace-read.c b/tracecmd/trace-read.c
index beaf97a8..a5a0e10a 100644
--- a/tracecmd/trace-read.c
+++ b/tracecmd/trace-read.c
@@ -482,7 +482,7 @@ static char *append_pid_filter(char *curr_filter, char *pid)
 	if (!curr_filter) {
 		filter = malloc(len);
 		if (!filter)
-			die("Failed to allocate for filter %s", curr_filter);
+			die("Failed to allocate pid filter");
 		sprintf(filter, ".*:" FILTER_FMT, pid, pid, pid);
 	} else {
 		curr_len = strlen(curr_filter);
-- 
2.44.0

[PATCH 11/38] trace-cmd record: prevent possible memory coruption in get_pid_addr_maps()

[Jerome Marchand]

If strdup() fails the error path access original address of
maps->lib_maps after it has been dereferenced. Make sure that
maps->lib_maps contains the up-to-date pointer before calling calling
a function that could fail.

This was flagged as ressource leak (CWE-772) because map isn't freed
in that scenario, but there is something worse going on that the
static analysis missed.

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 91cc90d4..f05a58d1 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -1230,12 +1230,12 @@ static int get_pid_addr_maps(struct buffer_instance *instance, int pid)
 				      (maps->nr_lib_maps + 1) * sizeof(*map));
 			if (!map)
 				goto out_fail;
+			maps->lib_maps = map;
 			map[maps->nr_lib_maps].end = end;
 			map[maps->nr_lib_maps].start = begin;
 			map[maps->nr_lib_maps].lib_name = strdup(mapname);
 			if (!map[maps->nr_lib_maps].lib_name)
 				goto out_fail;
-			maps->lib_maps = map;
 			maps->nr_lib_maps++;
 		}
 	}
-- 
2.44.0

[PATCH 12/38] trace-cmd hist: close tracecmd handle when trace_hist() exits early

[Jerome Marchand]

Call tracecmd_close() before returning if tracecmd_read_headers() fails.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/tracecmd/trace-hist.c b/tracecmd/trace-hist.c
index 62fe4f9b..47dcd6f8 100644
--- a/tracecmd/trace-hist.c
+++ b/tracecmd/trace-hist.c
@@ -1043,8 +1043,10 @@ void trace_hist(int argc, char **argv)
 		die("can't open %s\n", input_file);
 
 	ret = tracecmd_read_headers(handle, 0);
-	if (ret)
+	if (ret) {
+		tracecmd_close(handle);
 		return;
+	}
 
 	ret = tracecmd_init_data(handle);
 	if (ret < 0)
-- 
2.44.0

[PATCH 13/38] trace-cmd record: prevent a memory leak in show_error()

[Jerome Marchand]

In show_error() the pointer p is used for several functions. At some
point it is used to contain the error log file. It's not freed before
being replaced by the result of read_file(path), which is not freed
either. Free p in both case.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index f05a58d1..3e29f922 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -2364,13 +2364,16 @@ static void show_error(const char *file, const char *type)
 			goto read_file;
 		}
 		read_error_log(p);
+		free(p);
 		goto out;
 	}
 
  read_file:
 	p = read_file(path);
-	if (p)
+	if (p) {
 		printf("%s", p);
+		free(p);
+	}
 
  out:
 	printf("Failed %s of %s\n", type, file);
-- 
2.44.0

[PATCH 14/38] trace-cmd record: prevent memory leak in update_pid_filters()

[Jerome Marchand]

Free filter before exiting update_pid_filters()

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 3e29f922..c4d43469 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -2881,6 +2881,7 @@ static void update_pid_filters(struct buffer_instance *instance)
 	} while (ret >= 0 && len);
 
  out:
+	free(filter);
 	close(fd);
 }
 
-- 
2.44.0

[PATCH 15/38] trace-cmd lib: check the return value of do_lssek() in trace_get_options()

[Jerome Marchand]

Check that do_lseek doesn't fail before calling malloc() with a -1
argument.

This is flagged as an overrun error (CWE-119) by static anaysis
because of the call to read() later, but I don't imagine that malloc
would succeed.

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

diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 5ba0a145..35904620 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -2069,6 +2069,8 @@ __hidden void *trace_get_options(struct tracecmd_output *handle, size_t *len)
 	}
 
 	offset = do_lseek(&out_handle, 0, SEEK_CUR);
+	if(offset == (off_t)-1)
+		goto out;
 	buf = malloc(offset);
 	if (!buf)
 		goto out;
-- 
2.44.0

[PATCH 16/38] trace-cmd lib: don't double close a file descriptor in read_header_files()

[Jerome Marchand]

In read_header_files(), if we encounter an error after we close the
first file but before we open the second one, the exit code tries to
close it again.

Move the call to close() just before the second open to prevent that.

Fixes a USE_AFTER_FREE error (CWE-416)

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

diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 35904620..c270d03f 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -685,7 +685,6 @@ static int read_header_files(struct tracecmd_output *handle, bool compress)
 	if (do_write_check(handle, &endian8, 8))
 		goto out_free;
 	check_size = copy_file_fd(handle, fd, 0);
-	close(fd);
 	if (size != check_size) {
 		tracecmd_warning("wrong size for '%s' size=%lld read=%lld", path, size, check_size);
 		errno = EINVAL;
@@ -697,6 +696,7 @@ static int read_header_files(struct tracecmd_output *handle, bool compress)
 	if (!path)
 		goto out_close;
 
+	close(fd);
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
 		tracecmd_warning("can't read '%s'", path);
-- 
2.44.0

[PATCH 17/38] trace-cmd lib: prevent memory leak in ptp_clock_server()

[Jerome Marchand]

The pointer results is allocated in tracecmd_msg_recv_time_sync(). It
isn't freed if ptp_clock_server() exits with an error. Free it in the
error path.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/lib/trace-cmd/trace-timesync-ptp.c b/lib/trace-cmd/trace-timesync-ptp.c
index 20e6e6f1..402edeb2 100644
--- a/lib/trace-cmd/trace-timesync-ptp.c
+++ b/lib/trace-cmd/trace-timesync-ptp.c
@@ -608,8 +608,10 @@ static int ptp_clock_server(struct tracecmd_time_sync *tsync,
 					  sync_proto, &sync_msg,
 					  &size, (char **)&results);
 	if (ret || strncmp(sync_proto, PTP_NAME, TRACECMD_TSYNC_PNAME_LENGTH) ||
-	    sync_msg != PTP_SYNC_PKT_PROBES || size == 0 || results == NULL)
+	    sync_msg != PTP_SYNC_PKT_PROBES || size == 0 || results == NULL) {
+		free(results);
 		return -1;
+	}
 
 	ntoh_ptp_results(results);
 	if (ptp->flags & PTP_FLAG_USE_MARKER)
-- 
2.44.0

[PATCH 18/38] trace-cmd lib: remove useless code in tracecmd_plog()

[Jerome Marchand]

In __plog(), both branch of the "if (newline)" statement are
identical. I guess the original intent was to print a newline if the
last message didn't end in one. But it seems that all messages send to
tracecmd_plog() terminates with a newline and are short enough to not
get truncated.

Remove all newline code that doesn't do anything.

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

diff --git a/lib/trace-cmd/trace-util.c b/lib/trace-cmd/trace-util.c
index 47ca3db1..a4334a98 100644
--- a/lib/trace-cmd/trace-util.c
+++ b/lib/trace-cmd/trace-util.c
@@ -453,7 +453,6 @@ void __weak tracecmd_debug(const char *fmt, ...)
 #define LOG_BUF_SIZE 1024
 static void __plog(const char *prefix, const char *fmt, va_list ap, FILE *fp)
 {
-	static int newline = 1;
 	char buf[LOG_BUF_SIZE];
 	int r;
 
@@ -463,11 +462,7 @@ static void __plog(const char *prefix, const char *fmt, va_list ap, FILE *fp)
 		r = LOG_BUF_SIZE;
 
 	if (logfp) {
-		if (newline)
-			fprintf(logfp, "[%d]%s%.*s", getpid(), prefix, r, buf);
-		else
-			fprintf(logfp, "[%d]%s%.*s", getpid(), prefix, r, buf);
-		newline = buf[r - 1] == '\n';
+		fprintf(logfp, "[%d]%s%.*s", getpid(), prefix, r, buf);
 		fflush(logfp);
 		return;
 	}
-- 
2.44.0

[PATCH 19/38] trace-cmd record: prevent memory leak in add_all_instances()

[Jerome Marchand]

In __add_all_instances(), name is allocated by strdup(dent->d_name)
but is never freed. Anyway, the content of *name is copied in
append_file() and allocate_instance(), so it didn't need to be
strduped to begin with.

Fixes a RESOURCE_LEAK error (CWE-772)

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 c4d43469..a9e5e64d 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -422,7 +422,7 @@ static int __add_all_instances(const char *tracing_dir)
 	}
 
 	while ((dent = readdir(dir))) {
-		const char *name = strdup(dent->d_name);
+		const char *name = dent->d_name;
 		char *instance_path;
 		struct buffer_instance *instance;
 
-- 
2.44.0

[PATCH 20/38] trace-cmd lib: check for a negative return value of read in tracecmd_compress_copy_from()

[Jerome Marchand]

Check that read() return a non negative value before adding it to all
in tracecmd_compress_copy_from(). If all == 1 before that, we would
exit the loops thinking it succeeded.

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

diff --git a/lib/trace-cmd/trace-compress.c b/lib/trace-cmd/trace-compress.c
index e550dbd4..3ca0e21f 100644
--- a/lib/trace-cmd/trace-compress.c
+++ b/lib/trace-cmd/trace-compress.c
@@ -714,10 +714,10 @@ int tracecmd_compress_copy_from(struct tracecmd_compression *handle, int fd, int
 
 		do {
 			r = read(fd, buf_from + all, rchunk - all);
-			all += r;
-
 			if (r <= 0)
 				break;
+
+			all += r;
 		} while (all != rchunk);
 
 
-- 
2.44.0

[PATCH 21/38] trace-cmd record: prevent memory leak in clear_func_filter()

[Jerome Marchand]

Free buf at the end of the function.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index a9e5e64d..2223d5bb 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -2509,6 +2509,7 @@ static void clear_func_filter(const char *file)
 		filter[len+1] = '\0';
 		write_file(file, filter);
 	}
+	free(buf);
 }
 
 static void update_reset_triggers(void)
-- 
2.44.0

[PATCH 22/38] trace-cmd dump: prevent buffer overrun in dump_clock()

[Jerome Marchand]

The clock isn't big enough to hold the string with the null
terminating character. Worse, clock[size], which is out of range, is
set to 0. Allocate a big enough buffer.

Fixes an OVERRUN error (CWE-119)

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

diff --git a/tracecmd/trace-dump.c b/tracecmd/trace-dump.c
index 11c1baf1..c0a282c9 100644
--- a/tracecmd/trace-dump.c
+++ b/tracecmd/trace-dump.c
@@ -961,7 +961,7 @@ static void dump_clock(int fd)
 	}
 	if (read_file_number(fd, &size, 8))
 		die("cannot read clock size");
-	clock = calloc(1, size);
+	clock = calloc(1, size+1);
 	if (!clock)
 		die("cannot allocate clock %lld bytes", size);
 
-- 
2.44.0

[PATCH 23/38] trace-cmd lib: prevent buffer overrun in read_string()

[Jerome Marchand]

In read_string() we try to write a null character at str(size), which
is out of range:

	if (str) {
		size += i + 1;
		str = realloc(str, size);
		if (!str)
			return NULL;
		memcpy(str + (size - i), buf, i);
		str[size] = 0;
	}

The character that should be zeroed is supposed to be at the size - 1
index, which is the size of str prior the reallocation plus i. We also
know that buf[i] == 0 so we can simply memcpy that too instead of
zeroing it by hand. That simplifies the code a little.

Fixes an OVERRUN error (CWE-119)

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

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 3284dbd4..c485acea 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -447,15 +447,13 @@ static char *read_string(struct tracecmd_input *handle)
 		str = realloc(str, size);
 		if (!str)
 			return NULL;
-		memcpy(str + (size - i), buf, i);
-		str[size] = 0;
+		memcpy(str + (size - i), buf, i + 1);
 	} else {
 		size = i + 1;
 		str = malloc(size);
 		if (!str)
 			return NULL;
-		memcpy(str, buf, i);
-		str[i] = 0;
+		memcpy(str, buf, i + 1);
 	}
 
 	return str;
-- 
2.44.0

[PATCH 24/38] trace-cmd: close file descriptor in trace_vsock_make()

[Jerome Marchand]

Close the socket file descriptor in the error paths.

Fixes a RESOURCE_LEAK error (CWE-772)

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 tracecmd/trace-vsock.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tracecmd/trace-vsock.c b/tracecmd/trace-vsock.c
index baa310f7..df1a9800 100644
--- a/tracecmd/trace-vsock.c
+++ b/tracecmd/trace-vsock.c
@@ -43,12 +43,16 @@ int __hidden trace_vsock_make(unsigned int port)
 	setsockopt(sd, SOL_SOCKET, SO_REUSEADDR, &(int){1}, sizeof(int));
 
 	if (bind(sd, (struct sockaddr *)&addr, sizeof(addr)))
-		return -errno;
+		goto error;
 
 	if (listen(sd, SOMAXCONN))
-		return -errno;
+		goto error;
 
 	return sd;
+
+error:
+	close(sd);
+	return -errno;
 }
 
 int __hidden trace_vsock_make_any(void)
-- 
2.44.0

[PATCH 25/38] trace-cmd lib: prevent memory leak in glob_events()

[Jerome Marchand]

Free event_path if malloc() doesn't succeed.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index c270d03f..7be175af 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -868,8 +868,10 @@ static void glob_events(struct tracecmd_output *handle,
 
 	path = malloc(events_len + strlen(str) +
 		      strlen("/format") + 2);
-	if (!path)
+	if (!path) {
+		put_tracing_file(events_path);
 		return;
+	}
 	path[0] = '\0';
 	strcat(path, events_path);
 	strcat(path, "/");
-- 
2.44.0

[PATCH 26/38] trace-cmd record: don't print a NULL string in get_temp_file()

[Jerome Marchand]

In get_temp_file() if we fails to allocate a temp file for a top
instance, a NULL pointer is passed as the argument for a %s format in
the error message.

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 2223d5bb..b4cbd438 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -559,7 +559,7 @@ static char *get_temp_file(struct buffer_instance *instance, int cpu)
 		size = snprintf(file, 0, "%s.cpu%d", output_file, cpu);
 		file = malloc(size + 1);
 		if (!file)
-			die("Failed to allocate temp file for %s", name);
+			die("Failed to allocate temp file");
 		sprintf(file, "%s.cpu%d", output_file, cpu);
 	}
 
-- 
2.44.0

[PATCH 27/38] trace-cmd lib: prevent a possible file descriptor leak in set_proc_kptr_restrict()

[Jerome Marchand]

In set_proc_kptr_restrict() we test whether fd > 0 to close it.
Theorically, open() could have returned zero if stdin was closed. I
don't think it could happen here, but changing the test with fd >= 0
silence the static analyser which complains about a ressource leak.

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

diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 7be175af..6a9606c7 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -1124,7 +1124,7 @@ static void set_proc_kptr_restrict(int reset)
 	if (write(fd, &buf, 1) > 0)
 		ret = 0;
 err:
-	if (fd > 0)
+	if (fd >= 0)
 		close(fd);
 	if (ret)
 		tracecmd_warning("can't set kptr_restrict");
-- 
2.44.0

[PATCH 28/38] trace-cmd lib: remove unused tracecmd_parse_cmdlines() function

[Jerome Marchand]

Static analysis warned about a possible use-after-free error in
tracecmd_parse_cmdlines() sscanf() fails to match both items. However,
the function seems to not be used at all si we might as well remove it
entirely.

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

diff --git a/lib/trace-cmd/trace-util.c b/lib/trace-cmd/trace-util.c
index a4334a98..37caab45 100644
--- a/lib/trace-cmd/trace-util.c
+++ b/lib/trace-cmd/trace-util.c
@@ -135,23 +135,6 @@ bool tracecmd_get_notimeout(void)
 	return notimeout || debug;
 }
 
-void tracecmd_parse_cmdlines(struct tep_handle *pevent,
-			     char *file, int size __maybe_unused)
-{
-	char *comm;
-	char *line;
-	char *next = NULL;
-	int pid;
-
-	line = strtok_r(file, "\n", &next);
-	while (line) {
-		sscanf(line, "%d %m[^\n]s", &pid, &comm);
-		tep_register_comm(pevent, comm, pid);
-		free(comm);
-		line = strtok_r(NULL, "\n", &next);
-	}
-}
-
 void tracecmd_parse_proc_kallsyms(struct tep_handle *pevent,
 			 char *file, unsigned int size __maybe_unused)
 {
-- 
2.44.0

[PATCH 29/38] trace-cmd record: prevent memory leak in setup_network()

[Jerome Marchand]

Because of the again label, msg_handle can be already allocated if we
exit after we got a negative socket file descriptor. Free it there.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index b4cbd438..770e775b 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -3903,6 +3903,7 @@ static struct tracecmd_msg_handle *setup_network(struct buffer_instance *instanc
 
 	if (sfd < 0) {
 		free(thost);
+		free(msg_handle);
 		return NULL;
 	}
 
-- 
2.44.0

[PATCH 30/38] trace-cmd listen: prevent memory leak in communicate_with_client()

[Jerome Marchand]

Free option in the error path.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/tracecmd/trace-listen.c b/tracecmd/trace-listen.c
index 09f442c7..da46d09e 100644
--- a/tracecmd/trace-listen.c
+++ b/tracecmd/trace-listen.c
@@ -456,8 +456,10 @@ static int communicate_with_client(struct tracecmd_msg_handle *msg_handle)
 				t = size;
 				s = 0;
 				s = read(fd, option+s, t);
-				if (s <= 0)
+				if (s <= 0) {
+					free(option);
 					goto out;
+				}
 				t -= s;
 				s = size - t;
 			} while (t);
-- 
2.44.0

[PATCH 31/38] trace-cmd listen: prevent a infinite loop in communicate_with_client()

[Jerome Marchand]

The loop used to read the option is obviously wrong. If the option
isn't read with one call to read(), it will loop indefinitely. Move
the setting of the initial values out of the loop.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 tracecmd/trace-listen.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tracecmd/trace-listen.c b/tracecmd/trace-listen.c
index da46d09e..af9e3454 100644
--- a/tracecmd/trace-listen.c
+++ b/tracecmd/trace-listen.c
@@ -452,9 +452,9 @@ static int communicate_with_client(struct tracecmd_msg_handle *msg_handle)
 				goto out;
 
 			ret = -EIO;
+			t = size;
+			s = 0;
 			do {
-				t = size;
-				s = 0;
 				s = read(fd, option+s, t);
 				if (s <= 0) {
 					free(option);
-- 
2.44.0

[PATCH 32/38] trace-cmd lib: prevent memory leak in tracecmd_create_event_hook()

[Jerome Marchand]

Free hook in the error path.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/lib/trace-cmd/trace-hooks.c b/lib/trace-cmd/trace-hooks.c
index a58b5356..47f25fb3 100644
--- a/lib/trace-cmd/trace-hooks.c
+++ b/lib/trace-cmd/trace-hooks.c
@@ -151,6 +151,7 @@ struct hook_list *tracecmd_create_event_hook(const char *arg)
 
 invalid_tok:
 	tracecmd_warning("Invalid hook format '%s'", arg);
+	free(hook);
 	return NULL;
 }
 
-- 
2.44.0

[PATCH 33/38] trace-cmd record: prevent memory corruption in parse_record_options()

[Jerome Marchand]

In parse_record_options() we can end up using a deleted instance after
options have been parsed. This can be triggered by the following
command:
$ trace-cmd record -v -e block -B foo  ls

We probably need a proper to avoid to end up in this situation, but in
the mean time, check that the current instance isn't marked for
deletion before calling remove_instances(). That at least prevent an
hard to debug memory corruption bug.

Fixes a USE_AFTER_FREE error (CWE-416)

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

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 770e775b..dc3e5285 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -6909,6 +6909,8 @@ static void parse_record_options(int argc,
 		}
 	}
 
+	if (ctx->instance->delete)
+		die("Instance to be deleted is still used");
 	remove_instances(del_list);
 
 	/* If --date is specified, prepend it to all guest VM flags */
-- 
2.44.0

[PATCH 34/38] trace-cmd mem: prevent a memory leak in trace_mem()

[Jerome Marchand]

Close the tracecmd handle in the error path.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/tracecmd/trace-mem.c b/tracecmd/trace-mem.c
index 3e1ac9f3..6182b096 100644
--- a/tracecmd/trace-mem.c
+++ b/tracecmd/trace-mem.c
@@ -555,9 +555,10 @@ void trace_mem(int argc, char **argv)
 
 	ret = tracecmd_read_headers(handle, 0);
 	if (ret)
-		return;
+		goto out;
 
 	do_trace_mem(handle);
 
+out:
 	tracecmd_close(handle);
 }
-- 
2.44.0

[PATCH 35/38] trace-cmd: move the initialization of found_pid at the beginning of stop_trace_connect()

[Jerome Marchand]

In stop_trace_connect(), trace_fields.found_pid is used in the error
path before it has been initialized. Move the initialization at the
beginning of the function.

Fixes a UNINIT error (CWE-457)

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

diff --git a/tracecmd/trace-vm.c b/tracecmd/trace-vm.c
index f78f1d83..960ed5d5 100644
--- a/tracecmd/trace-vm.c
+++ b/tracecmd/trace-vm.c
@@ -280,6 +280,7 @@ static int stop_trace_connect(struct tracefs_instance *open_instance)
 
 	tep = tracefs_local_events_system(NULL, systems);
 
+	trace_fields.found_pid = -1;
 	trace_fields.sched_waking = tep_find_event_by_name(tep, "sched", "sched_waking");
 	if (!trace_fields.sched_waking)
 		goto out;
@@ -295,7 +296,6 @@ static int stop_trace_connect(struct tracefs_instance *open_instance)
 	if (!trace_fields.sched_next)
 		goto out;
 
-	trace_fields.found_pid = -1;
 	trace_fields.pids = NULL;
 	add_pid(&trace_fields.pids, getpid());
 	tracefs_iterate_raw_events(tep, open_instance, NULL, 0, callback, &trace_fields);
-- 
2.44.0

[PATCH 36/38] trace-cmd record: check the length of the protocol version received

[Jerome Marchand]

In check_protocol_version we compare the protocol version string with
the expected one ("V3") with memcmp(). The received string could be
longer than the constant string used for the comparison. That could
lead to out of range access.

Check that the received protocol version is not too long.

Fixes a OVERRUN error (CWE-119)

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 dc3e5285..c3118546 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -3810,7 +3810,7 @@ static void check_protocol_version(struct tracecmd_msg_handle *msg_handle)
 		msg_handle->version = V1_PROTOCOL;
 		tracecmd_plog("Use the v1 protocol\n");
 	} else {
-		if (memcmp(buf, "V3", n) != 0)
+		if (n > 3 || memcmp(buf, "V3", n) != 0)
 			die("Cannot handle the protocol %s", buf);
 		/* OK, let's use v3 protocol */
 		write(fd, V3_MAGIC, sizeof(V3_MAGIC));
-- 
2.44.0

[PATCH 37/38] trace-cmd record: close socket fd before retrying to connect

[Jerome Marchand]

In create_record(), if the client whose conection it just accepted
doesn't match, it tries to call get another connection without
releasing the socket file descriptor.

Close the file descriptor before retrying to connect.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index c3118546..0191ef2a 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -3651,6 +3651,7 @@ static int create_recorder(struct buffer_instance *instance, int cpu,
 				    !trace_net_cmp_connection_fd(fd, instance->host)) {
 					dprint("Client does not match '%s' for cpu:%d\n",
 					       instance->host, cpu);
+					close(fd);
 					goto again;
 				}
 			}
-- 
2.44.0

[PATCH 38/38] trace-cmd lib: prevent a memory leak in tracecmd_tsync_proto_getall()

[Jerome Marchand]

In tracecmd_tsync_proto_getall() if the allocation plist succeeded but
the allocation of plist->names didn't, the function just returns
without freeing plist. There is already a proper exit path that free
allocated ressource: just use it.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/lib/trace-cmd/trace-timesync.c b/lib/trace-cmd/trace-timesync.c
index 75c27bab..6ee4e643 100644
--- a/lib/trace-cmd/trace-timesync.c
+++ b/lib/trace-cmd/trace-timesync.c
@@ -290,7 +290,7 @@ int tracecmd_tsync_proto_getall(struct tracecmd_tsync_protos **protos, const cha
 		goto error;
 	plist->names = calloc(count, sizeof(char *));
 	if (!plist->names)
-		return -1;
+		goto error;
 
 	for (i = 0, proto = tsync_proto_list; proto && i < (count - 1); proto = proto->next) {
 		if (!(proto->roles & role))
-- 
2.44.0

Re: [PATCH 00/38] trace-cmd: fix misc issues found by static analysis

[Steven Rostedt]

On Wed,  5 Jun 2024 15:40:15 +0200
"Jerome Marchand" <jmarchan@redhat.com> wrote:

> A number of issues were found by running static analysers on the code
> of trace-cmd with openscanhub[1]. Mostly ressource leak, but some are
> more serious like memory corruption.
> 
> [1] https://fedoraproject.org/wiki/OpenScanHub

Thanks for the patches. I'll try to carve out some time to review them.

-- Steve

Re: [PATCH 06/38] trace-cmd lib: prevent a memory leak in handle_options()

[Steven Rostedt]

On Wed,  5 Jun 2024 15:40:21 +0200
"Jerome Marchand" <jmarchan@redhat.com> wrote:

> Free buf in the error path.
> 
> Fixes a RESOURCE_LEAK error (CWE-772)
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> ---
>  lib/trace-cmd/trace-input.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index ce4ecf43..2cf0d1c1 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -4030,7 +4030,7 @@ static int handle_options(struct tracecmd_input *handle)
>  		}
>  		ret = do_read_check(handle, buf, size);
>  		if (ret)
> -			goto out;
> +			goto out_free;
>  
>  		switch (option) {
>  		case TRACECMD_OPTION_DATE:
> @@ -4084,7 +4084,7 @@ static int handle_options(struct tracecmd_input *handle)
>  							     buf + 8, 4);
>  			ret = tsync_cpu_offsets_load(handle, buf + 12, size - 12);
>  			if (ret < 0)
> -				goto out;
> +				goto out_free;
>  			tracecmd_enable_tsync(handle, true);
>  			break;
>  		case TRACECMD_OPTION_CPUSTAT:
> @@ -4093,7 +4093,7 @@ static int handle_options(struct tracecmd_input *handle)
>  					   handle->cpustats_size + size + 1);
>  			if (!cpustats) {
>  				ret = -ENOMEM;
> -				goto out;
> +				goto out_free;
>  			}
>  			memcpy(cpustats + handle->cpustats_size, buf, size);
>  			handle->cpustats_size += size;
> @@ -4104,7 +4104,7 @@ static int handle_options(struct tracecmd_input *handle)
>  		case TRACECMD_OPTION_BUFFER_TEXT:
>  			ret = handle_buffer_option(handle, option, buf, size);
>  			if (ret < 0)
> -				goto out;
> +				goto out_free;
>  			break;
>  		case TRACECMD_OPTION_TRACECLOCK:
>  			tracecmd_parse_trace_clock(handle, buf, size);
> @@ -4183,6 +4183,8 @@ static int handle_options(struct tracecmd_input *handle)
>  
>  	ret = 0;
>  

The for (;;) loop ends with a free(buf) and then in the next iteration it can do:

		if (!HAS_SECTIONS(handle) && option == TRACECMD_OPTION_DONE)
			break;

> +out_free:
> +	free(buf);

Which will cause this to do a double free.

I'm going to not pull this patch.

-- Steve


>  out:
>  	if (compress)
>  		in_uncompress_reset(handle);

Re: [PATCH 08/38] trace-cmd lib: prevent a memory leak in create_event_list_item()

[Steven Rostedt]

On Wed,  5 Jun 2024 15:40:23 +0200
"Jerome Marchand" <jmarchan@redhat.com> wrote:

> Free ptr if malloc() fails.
> 
> Fixes a RESOURCE_LEAK error (CWE-772)
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> ---
>  lib/trace-cmd/trace-output.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> index 59498edc..4a9ecc5d 100644
> --- a/lib/trace-cmd/trace-output.c
> +++ b/lib/trace-cmd/trace-output.c
> @@ -955,7 +955,8 @@ create_event_list_item(struct tracecmd_output *handle,
>  	free(str);
>  	return;
>   err_mem:
> -	 tracecmd_warning("Insufficient memory");
> +	free(ptr);
> +	tracecmd_warning("Insufficient memory");

This function starts with:

	char *ptr;
	char *str;

	str = strdup(list->glob);
	if (!str)
		goto err_mem;

Where ptr is not defined yet.

I'm going to skip this patch.

-- Steve

>  }
>  
>  static int read_ftrace_files(struct tracecmd_output *handle, bool compress)

Re: [PATCH 13/38] trace-cmd record: prevent a memory leak in show_error()

[Steven Rostedt]

On Wed,  5 Jun 2024 15:40:28 +0200
"Jerome Marchand" <jmarchan@redhat.com> wrote:

> In show_error() the pointer p is used for several functions. At some
> point it is used to contain the error log file. It's not freed before
> being replaced by the result of read_file(path), which is not freed
> either. Free p in both case.

This isn't that big of a deal as this is just an application that is
about to exit anyway. But I'm fine with freeing it. That said:

> 
> Fixes a RESOURCE_LEAK error (CWE-772)
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> ---
>  tracecmd/trace-record.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index f05a58d1..3e29f922 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -2364,13 +2364,16 @@ static void show_error(const char *file,
> const char *type) goto read_file;
>  		}
>  		read_error_log(p);
> +		free(p);
>  		goto out;
>  	}
>  
>   read_file:
>  	p = read_file(path);
> -	if (p)
> +	if (p) {
>  		printf("%s", p);
> +		free(p);
> +	}
>  
>   out:

It would be much cleaner to just add:

	free(p);

here, and remove the above two calls.

>  	printf("Failed %s of %s\n", type, file);

I'll skip this patch too.

-- Steve

Re: [PATCH 15/38] trace-cmd lib: check the return value of do_lssek() in trace_get_options()

[Steven Rostedt]

Note the subject has a typo "lssek".


On Wed,  5 Jun 2024 15:40:30 +0200
"Jerome Marchand" <jmarchan@redhat.com> wrote:

> Check that do_lseek doesn't fail before calling malloc() with a -1
> argument.
> 
> This is flagged as an overrun error (CWE-119) by static anaysis
> because of the call to read() later, but I don't imagine that malloc
> would succeed.
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> ---
>  lib/trace-cmd/trace-output.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> index 5ba0a145..35904620 100644
> --- a/lib/trace-cmd/trace-output.c
> +++ b/lib/trace-cmd/trace-output.c
> @@ -2069,6 +2069,8 @@ __hidden void *trace_get_options(struct tracecmd_output *handle, size_t *len)
>  	}
>  
>  	offset = do_lseek(&out_handle, 0, SEEK_CUR);
> +	if(offset == (off_t)-1)

Nit, the above has whitespace issues:

	if (offset == (off_t)-1)

Could you resend this?

Thanks,

-- Steve

> +		goto out;
>  	buf = malloc(offset);
>  	if (!buf)
>  		goto out;

Re: [PATCH 22/38] trace-cmd dump: prevent buffer overrun in dump_clock()

[Steven Rostedt]

Note, please start the subject with a capital letter:

  trace-cmd dump: Prevent buffer overrun in dump_clock()

On Wed,  5 Jun 2024 15:40:37 +0200
"Jerome Marchand" <jmarchan@redhat.com> wrote:

> The clock isn't big enough to hold the string with the null
> terminating character. Worse, clock[size], which is out of range, is
> set to 0. Allocate a big enough buffer.
> 
> Fixes an OVERRUN error (CWE-119)
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> ---
>  tracecmd/trace-dump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tracecmd/trace-dump.c b/tracecmd/trace-dump.c
> index 11c1baf1..c0a282c9 100644
> --- a/tracecmd/trace-dump.c
> +++ b/tracecmd/trace-dump.c
> @@ -961,7 +961,7 @@ static void dump_clock(int fd)
>  	}
>  	if (read_file_number(fd, &size, 8))
>  		die("cannot read clock size");
> -	clock = calloc(1, size);
> +	clock = calloc(1, size+1);

Also we follow the Linux kernel syntax. Please add spaces.

	clock = calloc(1, size + 1);

Care to resend. I'll skip this patch as well.

Thanks,

-- Steve



>  	if (!clock)
>  		die("cannot allocate clock %lld bytes", size);
>  

Re: [PATCH 23/38] trace-cmd lib: prevent buffer overrun in read_string()

[Steven Rostedt]

On Wed,  5 Jun 2024 15:40:38 +0200
"Jerome Marchand" <jmarchan@redhat.com> wrote:


> 
> Fixes an OVERRUN error (CWE-119)
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> ---
>  lib/trace-cmd/trace-input.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 3284dbd4..c485acea 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -447,15 +447,13 @@ static char *read_string(struct tracecmd_input *handle)
>  		str = realloc(str, size);

Hmm, the above is a memory leak if it fails. That should be:

		tmp = realloc(str, size);
		if (!tmp) {
			free(str);
			return NULL;
		}
		str = tmp;

Not to do with your patch, but I think this code is generally wrong. :-/

Let's say BUFSIZ = 1024, and a string is 1124 in size, including the '\0'.

That is str[1123] = '\0'.

The loop would end with:

	size = 1024; i = 99;

	size += i + 1; // size = 1124;


>  		if (!str)
>  			return NULL;
> -		memcpy(str + (size - i), buf, i);
> -		str[size] = 0;
> +		memcpy(str + (size - i), buf, i + 1);

size - i = 1025

I think this is wrong, as we should be copying to str + 1024, and not str + 1025.

This should be fixed, but has nothing to do with this particular patch.

Thanks,

-- Steve


>  	} else {
>  		size = i + 1;
>  		str = malloc(size);
>  		if (!str)
>  			return NULL;
> -		memcpy(str, buf, i);
> -		str[i] = 0;
> +		memcpy(str, buf, i + 1);
>  	}
>  
>  	return str;

Re: [PATCH 29/38] trace-cmd record: prevent memory leak in setup_network()

[Steven Rostedt]

On Wed,  5 Jun 2024 15:40:44 +0200
"Jerome Marchand" <jmarchan@redhat.com> wrote:

> Because of the again label, msg_handle can be already allocated if we
> exit after we got a negative socket file descriptor. Free it there.
> 
> Fixes a RESOURCE_LEAK error (CWE-772)
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> ---
>  tracecmd/trace-record.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index b4cbd438..770e775b 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -3903,6 +3903,7 @@ static struct tracecmd_msg_handle *setup_network(struct buffer_instance *instanc
>  
>  	if (sfd < 0) {
>  		free(thost);
> +		free(msg_handle);

As msg_handle is created with:

	msg_handle = tracecmd_msg_handle_alloc(sfd, 0);

This should be:

	trace_msg_handle_close(msg_handle);

But may require:

			close(sfd);
+			msg_handle->fd = -1;
			free(host);
			host = NULL;
			goto again;

I'll skip this patch.

-- Steve


>  		return NULL;
>  	}
>  

Re: [PATCH 32/38] trace-cmd lib: prevent memory leak in tracecmd_create_event_hook()

[Steven Rostedt]

On Wed,  5 Jun 2024 15:40:47 +0200
"Jerome Marchand" <jmarchan@redhat.com> wrote:

> Free hook in the error path.
> 
> Fixes a RESOURCE_LEAK error (CWE-772)
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> ---
>  lib/trace-cmd/trace-hooks.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/trace-cmd/trace-hooks.c b/lib/trace-cmd/trace-hooks.c
> index a58b5356..47f25fb3 100644
> --- a/lib/trace-cmd/trace-hooks.c
> +++ b/lib/trace-cmd/trace-hooks.c
> @@ -151,6 +151,7 @@ struct hook_list *tracecmd_create_event_hook(const char *arg)
>  
>  invalid_tok:
>  	tracecmd_warning("Invalid hook format '%s'", arg);

Should we also have:

	free(hook->str);

here too?

-- Steve

> +	free(hook);
>  	return NULL;
>  }
>  

Re: [PATCH 33/38] trace-cmd record: prevent memory corruption in parse_record_options()

[Steven Rostedt]

On Wed,  5 Jun 2024 15:40:48 +0200
"Jerome Marchand" <jmarchan@redhat.com> wrote:

> In parse_record_options() we can end up using a deleted instance after
> options have been parsed. This can be triggered by the following
> command:
> $ trace-cmd record -v -e block -B foo  ls
> 
> We probably need a proper to avoid to end up in this situation, but in
> the mean time, check that the current instance isn't marked for
> deletion before calling remove_instances(). That at least prevent an
> hard to debug memory corruption bug.
> 
> Fixes a USE_AFTER_FREE error (CWE-416)
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> ---
>  tracecmd/trace-record.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 770e775b..dc3e5285 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -6909,6 +6909,8 @@ static void parse_record_options(int argc,
>  		}
>  	}
>  
> +	if (ctx->instance->delete)
> +		die("Instance to be deleted is still used");

This looks to only be an issue for record. Deletion of instances should
only be for the trace-cmd set command.

>  	remove_instances(del_list);
>  
>  	/* If --date is specified, prepend it to all guest VM flags */

I'll add this patch:

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 4e9ac598..1527be11 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -6748,7 +6748,8 @@ static void parse_record_options(int argc,
 			ctx->instance = allocate_instance(optarg);
 			if (!ctx->instance)
 				die("Failed to create instance");
-			ctx->instance->delete = negative;
+			if (IS_CMDSET(ctx))
+				ctx->instance->delete = negative;
 			negative = 0;
 			if (ctx->instance->delete) {
 				ctx->instance->next = del_list;

Which should fix the issue as well.

Thanks,

-- Steve

Re: [PATCH 34/38] trace-cmd mem: prevent a memory leak in trace_mem()

[Steven Rostedt]

On Wed,  5 Jun 2024 15:40:49 +0200
"Jerome Marchand" <jmarchan@redhat.com> wrote:

> --- a/tracecmd/trace-mem.c
> +++ b/tracecmd/trace-mem.c
> @@ -555,9 +555,10 @@ void trace_mem(int argc, char **argv)
>  
>  	ret = tracecmd_read_headers(handle, 0);
>  	if (ret)
> -		return;
> +		goto out;
>  
>  	do_trace_mem(handle);
>  
> +out:
>  	tracecmd_close(handle);
>  }
> -- 

Would be nicer to have:

	ret = tracecmd_read_headers(handle, 0);
	if (!ret)
		do_trace_mem(handle);

	tracecmd_close(handle);

-- Steve

Re: [PATCH 36/38] trace-cmd record: check the length of the protocol version received

[Steven Rostedt]

On Wed,  5 Jun 2024 15:40:51 +0200
"Jerome Marchand" <jmarchan@redhat.com> wrote:

> In check_protocol_version we compare the protocol version string with
> the expected one ("V3") with memcmp(). The received string could be
> longer than the constant string used for the comparison. That could
> lead to out of range access.
> 
> Check that the received protocol version is not too long.
> 
> Fixes a OVERRUN error (CWE-119)
> 
> 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 dc3e5285..c3118546 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -3810,7 +3810,7 @@ static void check_protocol_version(struct tracecmd_msg_handle *msg_handle)
>  		msg_handle->version = V1_PROTOCOL;
>  		tracecmd_plog("Use the v1 protocol\n");
>  	} else {
> -		if (memcmp(buf, "V3", n) != 0)
> +		if (n > 3 || memcmp(buf, "V3", n) != 0)
>  			die("Cannot handle the protocol %s", buf);

Actually, we may add more to it, so this should be:

		if (n < 3 || memcmp(buf, "V3", 3) != 0)

-- Steve

>  		/* OK, let's use v3 protocol */
>  		write(fd, V3_MAGIC, sizeof(V3_MAGIC));

Re: [PATCH 08/38] trace-cmd lib: prevent a memory leak in create_event_list_item()

[Jerome Marchand]

Hi Steven,

Sorry for the long delay. I was on PTO and then this got at the bottom 
of my pile.

On 17/07/2024 22:31, Steven Rostedt wrote:
> On Wed,  5 Jun 2024 15:40:23 +0200
> "Jerome Marchand" <jmarchan@redhat.com> wrote:
> 
>> Free ptr if malloc() fails.
>>
>> Fixes a RESOURCE_LEAK error (CWE-772)
>>
>> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
>> ---
>>   lib/trace-cmd/trace-output.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
>> index 59498edc..4a9ecc5d 100644
>> --- a/lib/trace-cmd/trace-output.c
>> +++ b/lib/trace-cmd/trace-output.c
>> @@ -955,7 +955,8 @@ create_event_list_item(struct tracecmd_output *handle,
>>   	free(str);
>>   	return;
>>    err_mem:
>> -	 tracecmd_warning("Insufficient memory");
>> +	free(ptr);
>> +	tracecmd_warning("Insufficient memory");
> 
> This function starts with:
> 
> 	char *ptr;
> 	char *str;
> 
> 	str = strdup(list->glob);
> 	if (!str)
> 		goto err_mem;
> 
> Where ptr is not defined yet.
> 
> I'm going to skip this patch.

After taking an other look at this, it looks like it was a false 
positive of the static analyzer anyway.

Jerome

> 
> -- Steve
> 
>>   }
>>   
>>   static int read_ftrace_files(struct tracecmd_output *handle, bool compress)
> 

Re: [PATCH 13/38] trace-cmd record: prevent a memory leak in show_error()

[Jerome Marchand]

On 17/07/2024 22:51, Steven Rostedt wrote:
> On Wed,  5 Jun 2024 15:40:28 +0200
> "Jerome Marchand" <jmarchan@redhat.com> wrote:
> 
>> In show_error() the pointer p is used for several functions. At some
>> point it is used to contain the error log file. It's not freed before
>> being replaced by the result of read_file(path), which is not freed
>> either. Free p in both case.
> 
> This isn't that big of a deal as this is just an application that is
> about to exit anyway. But I'm fine with freeing it. That said:
> 
>>
>> Fixes a RESOURCE_LEAK error (CWE-772)
>>
>> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
>> ---
>>   tracecmd/trace-record.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
>> index f05a58d1..3e29f922 100644
>> --- a/tracecmd/trace-record.c
>> +++ b/tracecmd/trace-record.c
>> @@ -2364,13 +2364,16 @@ static void show_error(const char *file,
>> const char *type) goto read_file;
>>   		}
>>   		read_error_log(p);
>> +		free(p);
>>   		goto out;
>>   	}
>>   
>>    read_file:
>>   	p = read_file(path);
>> -	if (p)
>> +	if (p) {
>>   		printf("%s", p);
>> +		free(p);
>> +	}
>>   
>>    out:
> 
> It would be much cleaner to just add:
> 
> 	free(p);
> 
> here, and remove the above two calls.

Good point. I'll update the description which is incorrect too.

Jerome

> 
>>   	printf("Failed %s of %s\n", type, file);
> 
> I'll skip this patch too.
> 
> -- Steve

Re: [PATCH 15/38] trace-cmd lib: check the return value of do_lssek() in trace_get_options()

[Jerome Marchand]

On 17/07/2024 23:10, Steven Rostedt wrote:
> 
> Note the subject has a typo "lssek".
> 
> 
> On Wed,  5 Jun 2024 15:40:30 +0200
> "Jerome Marchand" <jmarchan@redhat.com> wrote:
> 
>> Check that do_lseek doesn't fail before calling malloc() with a -1
>> argument.
>>
>> This is flagged as an overrun error (CWE-119) by static anaysis
>> because of the call to read() later, but I don't imagine that malloc
>> would succeed.
>>
>> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
>> ---
>>   lib/trace-cmd/trace-output.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
>> index 5ba0a145..35904620 100644
>> --- a/lib/trace-cmd/trace-output.c
>> +++ b/lib/trace-cmd/trace-output.c
>> @@ -2069,6 +2069,8 @@ __hidden void *trace_get_options(struct tracecmd_output *handle, size_t *len)
>>   	}
>>   
>>   	offset = do_lseek(&out_handle, 0, SEEK_CUR);
>> +	if(offset == (off_t)-1)
> 
> Nit, the above has whitespace issues:
> 
> 	if (offset == (off_t)-1)
> 
> Could you resend this?

Will do.

Jerome

> 
> Thanks,
> 
> -- Steve
> 
>> +		goto out;
>>   	buf = malloc(offset);
>>   	if (!buf)
>>   		goto out;
> 

Re: [PATCH 22/38] trace-cmd dump: prevent buffer overrun in dump_clock()

[Jerome Marchand]

On 18/07/2024 00:55, Steven Rostedt wrote:
> 
> Note, please start the subject with a capital letter:
> 
>    trace-cmd dump: Prevent buffer overrun in dump_clock()
> 
> On Wed,  5 Jun 2024 15:40:37 +0200
> "Jerome Marchand" <jmarchan@redhat.com> wrote:
> 
>> The clock isn't big enough to hold the string with the null
>> terminating character. Worse, clock[size], which is out of range, is
>> set to 0. Allocate a big enough buffer.
>>
>> Fixes an OVERRUN error (CWE-119)
>>
>> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
>> ---
>>   tracecmd/trace-dump.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tracecmd/trace-dump.c b/tracecmd/trace-dump.c
>> index 11c1baf1..c0a282c9 100644
>> --- a/tracecmd/trace-dump.c
>> +++ b/tracecmd/trace-dump.c
>> @@ -961,7 +961,7 @@ static void dump_clock(int fd)
>>   	}
>>   	if (read_file_number(fd, &size, 8))
>>   		die("cannot read clock size");
>> -	clock = calloc(1, size);
>> +	clock = calloc(1, size+1);
> 
> Also we follow the Linux kernel syntax. Please add spaces.
> 
> 	clock = calloc(1, size + 1);
> 
> Care to resend. I'll skip this patch as well.

Will do.

Jerome

> 
> Thanks,
> 
> -- Steve
> 
> 
> 
>>   	if (!clock)
>>   		die("cannot allocate clock %lld bytes", size);
>>   

Re: [PATCH 29/38] trace-cmd record: prevent memory leak in setup_network()

[Jerome Marchand]

On 18/07/2024 02:25, Steven Rostedt wrote:
> On Wed,  5 Jun 2024 15:40:44 +0200
> "Jerome Marchand" <jmarchan@redhat.com> wrote:
> 
>> Because of the again label, msg_handle can be already allocated if we
>> exit after we got a negative socket file descriptor. Free it there.
>>
>> Fixes a RESOURCE_LEAK error (CWE-772)
>>
>> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
>> ---
>>   tracecmd/trace-record.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
>> index b4cbd438..770e775b 100644
>> --- a/tracecmd/trace-record.c
>> +++ b/tracecmd/trace-record.c
>> @@ -3903,6 +3903,7 @@ static struct tracecmd_msg_handle *setup_network(struct buffer_instance *instanc
>>   
>>   	if (sfd < 0) {
>>   		free(thost);
>> +		free(msg_handle);
> 
> As msg_handle is created with:
> 
> 	msg_handle = tracecmd_msg_handle_alloc(sfd, 0);
> 
> This should be:
> 
> 	trace_msg_handle_close(msg_handle);
> 
> But may require:
> 
> 			close(sfd);
> +			msg_handle->fd = -1;
> 			free(host);
> 			host = NULL;
> 			goto again;
> 
> I'll skip this patch.

I'll send a fixed version.

Jerome

> 
> -- Steve
> 
> 
>>   		return NULL;
>>   	}
>>   

Re: [PATCH 32/38] trace-cmd lib: prevent memory leak in tracecmd_create_event_hook()

[Jerome Marchand]

On 18/07/2024 03:16, Steven Rostedt wrote:
> On Wed,  5 Jun 2024 15:40:47 +0200
> "Jerome Marchand" <jmarchan@redhat.com> wrote:
> 
>> Free hook in the error path.
>>
>> Fixes a RESOURCE_LEAK error (CWE-772)
>>
>> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
>> ---
>>   lib/trace-cmd/trace-hooks.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/lib/trace-cmd/trace-hooks.c b/lib/trace-cmd/trace-hooks.c
>> index a58b5356..47f25fb3 100644
>> --- a/lib/trace-cmd/trace-hooks.c
>> +++ b/lib/trace-cmd/trace-hooks.c
>> @@ -151,6 +151,7 @@ struct hook_list *tracecmd_create_event_hook(const char *arg)
>>   
>>   invalid_tok:
>>   	tracecmd_warning("Invalid hook format '%s'", arg);
> 
> Should we also have:
> 
> 	free(hook->str);
> 
> here too?

Yes indeed. This wasn't detected by the static analyzer. I'll send an 
updated patch.

Jerome

> 
> -- Steve
> 
>> +	free(hook);
>>   	return NULL;
>>   }
>>   

Re: [PATCH 34/38] trace-cmd mem: prevent a memory leak in trace_mem()

[Jerome Marchand]

On 18/07/2024 03:53, Steven Rostedt wrote:
> On Wed,  5 Jun 2024 15:40:49 +0200
> "Jerome Marchand" <jmarchan@redhat.com> wrote:
> 
>> --- a/tracecmd/trace-mem.c
>> +++ b/tracecmd/trace-mem.c
>> @@ -555,9 +555,10 @@ void trace_mem(int argc, char **argv)
>>   
>>   	ret = tracecmd_read_headers(handle, 0);
>>   	if (ret)
>> -		return;
>> +		goto out;
>>   
>>   	do_trace_mem(handle);
>>   
>> +out:
>>   	tracecmd_close(handle);
>>   }
>> -- 
> 
> Would be nicer to have:
> 
> 	ret = tracecmd_read_headers(handle, 0);
> 	if (!ret)
> 		do_trace_mem(handle);
> 
> 	tracecmd_close(handle);

Indeed. I'll send an updated patch.

Jerome

> 
> -- Steve

Re: [PATCH 36/38] trace-cmd record: check the length of the protocol version received

[Jerome Marchand]

On 18/07/2024 04:11, Steven Rostedt wrote:
> On Wed,  5 Jun 2024 15:40:51 +0200
> "Jerome Marchand" <jmarchan@redhat.com> wrote:
> 
>> In check_protocol_version we compare the protocol version string with
>> the expected one ("V3") with memcmp(). The received string could be
>> longer than the constant string used for the comparison. That could
>> lead to out of range access.
>>
>> Check that the received protocol version is not too long.
>>
>> Fixes a OVERRUN error (CWE-119)
>>
>> 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 dc3e5285..c3118546 100644
>> --- a/tracecmd/trace-record.c
>> +++ b/tracecmd/trace-record.c
>> @@ -3810,7 +3810,7 @@ static void check_protocol_version(struct tracecmd_msg_handle *msg_handle)
>>   		msg_handle->version = V1_PROTOCOL;
>>   		tracecmd_plog("Use the v1 protocol\n");
>>   	} else {
>> -		if (memcmp(buf, "V3", n) != 0)
>> +		if (n > 3 || memcmp(buf, "V3", n) != 0)
>>   			die("Cannot handle the protocol %s", buf);
> 
> Actually, we may add more to it, so this should be:
> 
> 		if (n < 3 || memcmp(buf, "V3", 3) != 0)

That's definitely more future proof. I'll send an updated version.

Jerome

> 
> -- Steve
> 
>>   		/* OK, let's use v3 protocol */
>>   		write(fd, V3_MAGIC, sizeof(V3_MAGIC));
> 

[PATCH 0/8 v2] trace-cmd: fix misc issues found by static analysis

[Jerome Marchand]

A number of issues were found by running static analysers on the code
of trace-cmd with openscanhub[1]. Mostly ressource leak, but some are
more serious like memory corruption.

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

Jerome Marchand (8):
  trace-cmd lib: Prevent a memory leak in handle_options()
  trace-cmd record: Prevent a memory leak in show_error()
  trace-cmd lib: Check the return value of do_lseek() in
    trace_get_options()
  trace-cmd dump: Prevent buffer overrun in dump_clock()
  trace-cmd record: Prevent memory leak in setup_network()
  trace-cmd lib: Prevent memory leak in tracecmd_create_event_hook()
  trace-cmd mem: Prevent a memory leak in trace_mem()
  trace-cmd record: Check the length of the protocol version received

 lib/trace-cmd/trace-hooks.c  | 2 ++
 lib/trace-cmd/trace-input.c  | 7 +++----
 lib/trace-cmd/trace-output.c | 2 ++
 tracecmd/trace-dump.c        | 2 +-
 tracecmd/trace-mem.c         | 6 ++----
 tracecmd/trace-record.c      | 5 ++++-
 6 files changed, 14 insertions(+), 10 deletions(-)

-- 
2.47.0

[PATCH 1/8] trace-cmd lib: Prevent a memory leak in handle_options()

[Jerome Marchand]

Buf isn't always fred in the error path. Instead of freing buf at the
end of the loop, free it in the exit path and before reallocating it.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 8b6e3d0c..ad662fc6 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -4006,7 +4006,7 @@ static int handle_options(struct tracecmd_input *handle)
 	char *cpustats = NULL;
 	struct hook_list *hook;
 	bool compress = false;
-	char *buf;
+	char *buf = NULL;
 	int cpus;
 	int ret;
 
@@ -4036,6 +4036,7 @@ static int handle_options(struct tracecmd_input *handle)
 		ret = read4(handle, &size);
 		if (ret)
 			goto out;
+		free(buf);
 		buf = malloc(size);
 		if (!buf) {
 			ret = -ENOMEM;
@@ -4189,14 +4190,12 @@ static int handle_options(struct tracecmd_input *handle)
 			tracecmd_warning("unknown option %d", option);
 			break;
 		}
-
-		free(buf);
-
 	}
 
 	ret = 0;
 
 out:
+	free(buf);
 	if (compress)
 		in_uncompress_reset(handle);
 	return ret;
-- 
2.47.0

[PATCH 2/8] trace-cmd record: Prevent a memory leak in show_error()

[Jerome Marchand]

In show_error() the pointer p is used for several functions. At first,
it contain a substring of path.

Then it is replaced by either an allocated string containing the path
to the error log file or the result of read_path(), neither of which
are freed when exiting.

Free p in both case in the exit path.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index a008cdfd..3c42cdf0 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -2374,6 +2374,7 @@ static void show_error(const char *file, const char *type)
 
  out:
 	printf("Failed %s of %s\n", type, file);
+	free(p);
 	free(path);
 	return;
 }
-- 
2.47.0

[PATCH 3/8] trace-cmd lib: Check the return value of do_lseek() in trace_get_options()

[Jerome Marchand]

Check that do_lseek doesn't fail before calling malloc() with a -1
argument.

This is flagged as an overrun error (CWE-119) by static anaysis
because of the call to read() later, but I don't imagine that malloc
would succeed.

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

diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 66e11ddc..8bc9325c 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -2070,6 +2070,8 @@ __hidden void *trace_get_options(struct tracecmd_output *handle, size_t *len)
 	}
 
 	offset = do_lseek(&out_handle, 0, SEEK_CUR);
+	if (offset == (off_t)-1)
+		goto out;
 	buf = malloc(offset);
 	if (!buf)
 		goto out;
-- 
2.47.0

[PATCH 4/8] trace-cmd dump: Prevent buffer overrun in dump_clock()

[Jerome Marchand]

The clock isn't big enough to hold the string with the null
terminating character. Worse, clock[size], which is out of range, is
set to 0. Allocate a big enough buffer.

Fixes an OVERRUN error (CWE-119)

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

diff --git a/tracecmd/trace-dump.c b/tracecmd/trace-dump.c
index 11c1baf1..0a21356e 100644
--- a/tracecmd/trace-dump.c
+++ b/tracecmd/trace-dump.c
@@ -961,7 +961,7 @@ static void dump_clock(int fd)
 	}
 	if (read_file_number(fd, &size, 8))
 		die("cannot read clock size");
-	clock = calloc(1, size);
+	clock = calloc(1, size + 1);
 	if (!clock)
 		die("cannot allocate clock %lld bytes", size);
 
-- 
2.47.0

[PATCH 5/8] trace-cmd record: Prevent memory leak in setup_network()

[Jerome Marchand]

Because of the again label, msg_handle can be already allocated if we
exit after we got a negative socket file descriptor. Free it there.
Also unassign msg_handle->fd as to not double close sfd.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 3c42cdf0..7e84e897 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -3904,6 +3904,7 @@ static struct tracecmd_msg_handle *setup_network(struct buffer_instance *instanc
 
 	if (sfd < 0) {
 		free(thost);
+		tracecmd_msg_handle_close(msg_handle);
 		return NULL;
 	}
 
@@ -3934,6 +3935,7 @@ static struct tracecmd_msg_handle *setup_network(struct buffer_instance *instanc
 		if (msg_handle->version == V1_PROTOCOL) {
 			/* reconnect to the server for using the v1 protocol */
 			close(sfd);
+			msg_handle->fd = -1;
 			free(host);
 			host = NULL;
 			goto again;
-- 
2.47.0

[PATCH 6/8] trace-cmd lib: Prevent memory leak in tracecmd_create_event_hook()

[Jerome Marchand]

Free hook and hook->str in the error path.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/lib/trace-cmd/trace-hooks.c b/lib/trace-cmd/trace-hooks.c
index a58b5356..aa12f6e9 100644
--- a/lib/trace-cmd/trace-hooks.c
+++ b/lib/trace-cmd/trace-hooks.c
@@ -151,6 +151,8 @@ struct hook_list *tracecmd_create_event_hook(const char *arg)
 
 invalid_tok:
 	tracecmd_warning("Invalid hook format '%s'", arg);
+	free(hook->str);
+	free(hook);
 	return NULL;
 }
 
-- 
2.47.0

[PATCH 7/8] trace-cmd mem: Prevent a memory leak in trace_mem()

[Jerome Marchand]

Close the tracecmd handle in the error path.

Fixes a RESOURCE_LEAK error (CWE-772)

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 tracecmd/trace-mem.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tracecmd/trace-mem.c b/tracecmd/trace-mem.c
index 3e1ac9f3..b8babbbc 100644
--- a/tracecmd/trace-mem.c
+++ b/tracecmd/trace-mem.c
@@ -554,10 +554,8 @@ void trace_mem(int argc, char **argv)
 		die("can't open %s\n", input_file);
 
 	ret = tracecmd_read_headers(handle, 0);
-	if (ret)
-		return;
-
-	do_trace_mem(handle);
+	if (!ret)
+		do_trace_mem(handle);
 
 	tracecmd_close(handle);
 }
-- 
2.47.0

[PATCH 8/8] trace-cmd record: Check the length of the protocol version received

[Jerome Marchand]

In check_protocol_version we compare the protocol version string with
the expected one ("V3") with memcmp(). The received string could be
longer than the constant string used for the comparison. That could
lead to out of range access.

Use the known length of the fixed "V3" string for the comparison and
check that the received protocol version is not too short.

Fixes a OVERRUN error (CWE-119)

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 7e84e897..6e9b4535 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -3811,7 +3811,7 @@ static void check_protocol_version(struct tracecmd_msg_handle *msg_handle)
 		msg_handle->version = V1_PROTOCOL;
 		tracecmd_plog("Use the v1 protocol\n");
 	} else {
-		if (memcmp(buf, "V3", n) != 0)
+		if (n < 3 || memcmp(buf, "V3", 3) != 0)
 			die("Cannot handle the protocol %s", buf);
 		/* OK, let's use v3 protocol */
 		write(fd, V3_MAGIC, sizeof(V3_MAGIC));
-- 
2.47.0