[PATCH 0/4] libtraceevent: fix misc issues found by static analysis

[PATCH 0/4] libtraceevent: fix misc issues found by static analysis

[Jerome Marchand]

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

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

Jerome Marchand (4):
  libtraceevent: close shared object in the error path of load_plugin()
  libtraceevent: prevent a memory leak in process_fields()
  libtraceevent: prevent a memory leak in tep_plugin_add_option()
  libtraceevent: don't return a pointer to a local variable
    get_field_str()

 src/event-parse.c  |  2 +-
 src/event-plugin.c | 10 ++++++----
 src/parse-filter.c |  3 ++-
 3 files changed, 9 insertions(+), 6 deletions(-)

-- 
2.45.1

[PATCH 1/4] libtraceevent: close shared object in the error path of load_plugin()

[Jerome Marchand]

The handle returned by dlopen() isn't close if an error occures
afterward. Call dlclose() in the error path.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/src/event-plugin.c b/src/event-plugin.c
index f42243f..7f94107 100644
--- a/src/event-plugin.c
+++ b/src/event-plugin.c
@@ -474,7 +474,7 @@ load_plugin(struct tep_handle *tep, const char *path,
 		while (options->name) {
 			ret = update_option(alias, options);
 			if (ret < 0)
-				goto out_free;
+				goto out_close;
 			options++;
 		}
 	}
@@ -483,13 +483,13 @@ load_plugin(struct tep_handle *tep, const char *path,
 	if (!func) {
 		tep_warning("could not find func '%s' in plugin '%s'\n%s\n",
 			    TEP_PLUGIN_LOADER_NAME, plugin, dlerror());
-		goto out_free;
+		goto out_close;
 	}
 
 	list = malloc(sizeof(*list));
 	if (!list) {
 		tep_warning("could not allocate plugin memory\n");
-		goto out_free;
+		goto out_close;
 	}
 
 	list->next = *plugin_list;
@@ -501,6 +501,8 @@ load_plugin(struct tep_handle *tep, const char *path,
 	func(tep);
 	return;
 
+out_close:
+	dlclose(handle);
  out_free:
 	free(plugin);
 }
-- 
2.45.1

[PATCH 2/4] libtraceevent: prevent a memory leak in process_fields()

[Jerome Marchand]

One of the error path after field was allocated go to the wrong label.
Go to out_free_field if the allocation of arg fails.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/src/event-parse.c b/src/event-parse.c
index b625621..9f0522c 100644
--- a/src/event-parse.c
+++ b/src/event-parse.c
@@ -2963,7 +2963,7 @@ process_fields(struct tep_event *event, struct tep_print_flag_sym **list, char *
 		free_arg(arg);
 		arg = alloc_arg();
 		if (!arg)
-			goto out_free;
+			goto out_free_field;
 
 		free_token(token);
 		type = process_arg(event, arg, &token);
-- 
2.45.1

[PATCH 3/4] libtraceevent: prevent a memory leak in tep_plugin_add_option()

[Jerome Marchand]

If parse_option_name() fails, plugin, which now points to the previous
value of option_str isn't freed. Go to out_free if that happen.

Fixes a RESOURCE_LEAK error (CWE-772)

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

diff --git a/src/event-plugin.c b/src/event-plugin.c
index 7f94107..c944204 100644
--- a/src/event-plugin.c
+++ b/src/event-plugin.c
@@ -327,7 +327,7 @@ int tep_plugin_add_option(const char *name, const char *val)
 		return -ENOMEM;
 
 	if (parse_option_name(&option_str, &plugin) < 0)
-		return -ENOMEM;
+		goto out_free;
 
 	/* If the option exists, update the val */
 	for (op = trace_plugin_options; op; op = op->next) {
-- 
2.45.1

[PATCH 4/4] libtraceevent: don't return a pointer to a local variable get_field_str()

[Jerome Marchand]

The function get_field_str() can return a pointer to string on the
stack. Replace it by a global variable.

Fixes a RETURN_LOCAL error (CWE-562)

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

diff --git a/src/parse-filter.c b/src/parse-filter.c
index e448ee2..f0c0ae1 100644
--- a/src/parse-filter.c
+++ b/src/parse-filter.c
@@ -1698,6 +1698,8 @@ static int test_num(struct tep_event *event, struct tep_filter_arg *arg,
 	}
 }
 
+char hex[64];
+
 static const char *get_field_str(struct tep_filter_arg *arg, struct tep_record *record)
 {
 	struct tep_event *event;
@@ -1705,7 +1707,6 @@ static const char *get_field_str(struct tep_filter_arg *arg, struct tep_record *
 	unsigned long long addr;
 	const char *val = NULL;
 	unsigned int size;
-	char hex[64];
 
 	/* If the field is not a string convert it */
 	if (arg->str.field->flags & TEP_FIELD_IS_STRING) {
-- 
2.45.1

Re: [PATCH 4/4] libtraceevent: don't return a pointer to a local variable get_field_str()

[Steven Rostedt]

On Fri,  7 Jun 2024 18:05:42 +0200
"Jerome Marchand" <jmarchan@redhat.com> wrote:

> The function get_field_str() can return a pointer to string on the
> stack. Replace it by a global variable.
> 
> Fixes a RETURN_LOCAL error (CWE-562)
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> ---
>  src/parse-filter.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/parse-filter.c b/src/parse-filter.c
> index e448ee2..f0c0ae1 100644
> --- a/src/parse-filter.c
> +++ b/src/parse-filter.c
> @@ -1698,6 +1698,8 @@ static int test_num(struct tep_event *event, struct tep_filter_arg *arg,
>  	}
>  }
>  
> +char hex[64];
> +
>  static const char *get_field_str(struct tep_filter_arg *arg, struct tep_record *record)
>  {
>  	struct tep_event *event;
> @@ -1705,7 +1707,6 @@ static const char *get_field_str(struct tep_filter_arg *arg, struct tep_record *
>  	unsigned long long addr;
>  	const char *val = NULL;
>  	unsigned int size;
> -	char hex[64];

Instead of making hex a global variable (which is incorrect for
multiple reasons, and can caus bugs elsewhere). Just make it a static
variable here.

	static char hex[64];

I'll fix this and give you a reported-by.

-- Steve


>  
>  	/* If the field is not a string convert it */
>  	if (arg->str.field->flags & TEP_FIELD_IS_STRING) {