Re: [PATCH] libtraceevent: Fix a double free in process_op()

On Tue, 25 Jun 2024 20:39:49 -0700
Namhyung Kim <[email protected]> wrote:

> When process_cond() failed, it freed the token but didn't reset the
> arg->op.op to NULL.  So it tried to free the arg->op.op again from
> free_arg() from the caller and resulted in a double free.
> 
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
>  src/event-parse.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/event-parse.c b/src/event-parse.c
> index 9f0522c..c327917 100644
> --- a/src/event-parse.c
> +++ b/src/event-parse.c
> @@ -2375,8 +2375,11 @@ process_op(struct tep_event *event, struct tep_print_arg *arg, char **tok)
>  
>  		/* it will set arg->op.right */
>  		type = process_cond(event, arg, tok);
> -		if (type == TEP_EVENT_ERROR)
> -			free(token);
> +		if (type == TEP_EVENT_ERROR) {
> +			/* arg->op.op (= token) will be freed at out_free */
> +			arg->op.op = NULL;
> +			goto out_free;
> +		}
>  
>  	} else if (strcmp(token, ">>") == 0 ||
>  		   strcmp(token, "<<") == 0 ||

Hmm, so this fixes:

76a0eb8d5a20c ("libtraceevent: Fix event-parse memory leak in process_cond")

Which added:

+               if (type == TEP_EVENT_ERROR)
+                       free(token);

because of a memory leak. But if token is arg->op, and we should be freeing
that arg, how did it end up being a leak?

Because args->op.left is allocated. Whatever frees that, should also have
freed args->op.op.

I'm getting a feeling that we are missing the real bug.

Perhaps the real fix was:

diff --git a/src/event-parse.c b/src/event-parse.c
index 9f0522c8d9e4..c7ffad116c3f 100644
--- a/src/event-parse.c
+++ b/src/event-parse.c
@@ -2375,8 +2375,6 @@ process_op(struct tep_event *event, struct tep_print_arg *arg, char **tok)
 
 		/* it will set arg->op.right */
 		type = process_cond(event, arg, tok);
-		if (type == TEP_EVENT_ERROR)
-			free(token);
 
 	} else if (strcmp(token, ">>") == 0 ||
 		   strcmp(token, "<<") == 0 ||
@@ -2587,6 +2585,9 @@ static int alloc_and_process_delim(struct tep_event *event, char *next_token,
 	if (type == TEP_EVENT_OP) {
 		type = process_op(event, field, &token);
 
+		if (type == TEP_EVENT_ERROR)
+			goto out_error;
+
 		if (consolidate_op_arg(field) < 0)
 			type = TEP_EVENT_ERROR;
 
-- Steve