Re: [PATCH v2] Fix double free issue in event_read_print_args

On Mon,  1 Jul 2024 10:24:46 +0800
Tw <[email protected]> wrote:

> The corner case is that when we encounter a invalid right argument of a condition operation.
> Currently, we free token immediately, but it will also be freed when free `arg->op.op`.
> 
> BTW, the crash calltrace as follows:

OK, so someone else sent a patch for the same bug, but the cause of the bug
ended up being something different.

https://lore.kernel.org/linux-trace-devel/[email protected]/
https://lore.kernel.org/linux-trace-devel/[email protected]


> 
> Program received signal SIGSEGV, Segmentation fault.
> get_meta (p=<optimized out>) at /home/tw/code/zig/build/stage3/lib/zig/libc/musl/src/malloc/mallocng/meta.h:141
> 141     /home/tw/code/zig/build/stage3/lib/zig/libc/musl/src/malloc/mallocng/meta.h: No such file or directory.
> (gdb) bt
>     at /home/tw/code/zig/build/stage3/lib/zig/libc/musl/src/malloc/mallocng/meta.h:141
>     at /home/tw/code/zig/build/stage3/lib/zig/libc/musl/src/malloc/mallocng/free.c:105
>     at /tmp/.cache/zig/p/12207a2e4477bf4414e7df3eb2172c698ab916695a0d3eefbf16f65b0c969dd81184/src/event-parse.c:1128
>     list@entry=0x7ff7b18768)
>     at /tmp/.cache/zig/p/12207a2e4477bf4414e7df3eb2172c698ab916695a0d3eefbf16f65b0c969dd81184/src/event-parse.c:1417
>     at /tmp/.cache/zig/p/12207a2e4477bf4414e7df3eb2172c698ab916695a0d3eefbf16f65b0c969dd81184/src/event-parse.c:3895
>     sys=<optimized out>)
>     at /tmp/.cache/zig/p/12207a2e4477bf4414e7df3eb2172c698ab916695a0d3eefbf16f65b0c969dd81184/src/event-parse.c:7824
>     size=<optimized out>, sys=sys@entry=0x7ff7ff51c0 "kvm")
>     at /tmp/.cache/zig/p/12207a2e4477bf4414e7df3eb2172c698ab916695a0d3eefbf16f65b0c969dd81184/src/event-parse.c:7882
>     buf=0x7ff7b0c610 "kvm_sys_access", size=549616874800, sys=0x7fffffe0b2 "me", sys@entry=0x7ff7ff51c0 "kvm")
>     at /tmp/.cache/zig/p/12207a2e4477bf4414e7df3eb2172c698ab916695a0d3eefbf16f65b0c969dd81184/src/event-parse.c:7945
>     tracing_dir=tracing_dir@entry=0x7ff7ffc660 "/sys/kernel/tracing", system=system@entry=0x7ff7ff51c0 "kvm",
>     check=false)
>     at /tmp/.cache/zig/p/1220c1c006cbf05434d240b65f343c84f3d7f837fbef31f2cade733ec911cc3ed76b/src/tracefs-events.c:1062
>     system=0x7ff7ff51c0 "kvm")
>     at /tmp/.cache/zig/p/1220c1c006cbf05434d240b65f343c84f3d7f837fbef31f2cade733ec911cc3ed76b/src/tracefs-events.c:1084
>     tep=tep@entry=0x7ff7ffc830, sys_names=sys_names@entry=0x0, parsing_failures=0x0,
>     parsing_failures@entry=0x7fffffe7b0)
>     at /tmp/.cache/zig/p/1220c1c006cbf05434d240b65f343c84f3d7f837fbef31f2cade733ec911cc3ed76b/src/tracefs-events.c:1284
>     sys_names@entry=0x7ffffff880)
>     at /tmp/.cache/zig/p/1220c1c006cbf05434d240b65f343c84f3d7f837fbef31f2cade733ec911cc3ed76b/src/tracefs-events.c:1355
>     tracing_dir=0x6500006c6f6f62 <error: Cannot access memory at address 0x6500006c6f6f62>)
>     at /tmp/.cache/zig/p/1220c1c006cbf05434d240b65f343c84f3d7f837fbef31f2cade733ec911cc3ed76b/src/tracefs-events.c:1377
> 
> Signed-off-by: Tw <[email protected]>
> ---
>  src/event-parse.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/src/event-parse.c b/src/event-parse.c
> index 9f0522c..1f51ee9 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);

This should be removed, but how to fix the bug for the reason it was added
in the first place must be done too.

>  
>  	} else if (strcmp(token, ">>") == 0 ||
>  		   strcmp(token, "<<") == 0 ||
> @@ -3787,7 +3785,7 @@ static int event_read_print_args(struct tep_event *event, struct tep_print_arg *
>  {
>  	enum tep_event_type type = TEP_EVENT_ERROR;
>  	struct tep_print_arg *arg;
> -	char *token;
> +	char *token = NULL;

This is unneeded because token is assigned later:

		if (type == TEP_EVENT_NEWLINE) {
			type = read_token_item(event->tep, &token);
			continue;
		}

where read_token_item() has:

  static enum tep_event_type read_token_item(struct tep_handle *tep, char **tok)
  {
	enum tep_event_type type;

	for (;;) {
		type = __read_token(tep, tok);


And:

  static enum tep_event_type __read_token(struct tep_handle *tep, char **tok)
  {
	char buf[BUFSIZ];
	int ch, last_ch, quote_ch, next_ch;
	int i = 0;
	int tok_size = 0;
	enum tep_event_type type;

	*tok = NULL;

So that does the "token = NULL", and if that first if doesn't get hit in
event_read_print_args(), it then does:

		type = process_arg(event, arg, &token);

Where process_arg() is:

  static enum tep_event_type
  process_arg(struct tep_event *event, struct tep_print_arg *arg, char **tok)
  {
	enum tep_event_type type;
	char *token;

	type = read_token(event->tep, &token);

Which assigns token.

Hence, initializing "token" is not needed.

>  	int args = 0;
>  
>  	do {
> @@ -3817,6 +3815,7 @@ static int event_read_print_args(struct tep_event *event, struct tep_print_arg *
>  		if (type == TEP_EVENT_OP) {
>  			type = process_op(event, arg, &token);
>  			free_token(token);
> +			token = NULL;
>  
>  			if (consolidate_op_arg(arg) < 0)
>  				type = TEP_EVENT_ERROR;

Now this part does have the bug. But that's fixed in the links I showed.
The key thing was to look at the result of "type". It was not about freeing
token, because if the type returned was TEP_EVENT_ERROR, that
consolidate_op_arg() should never had been called.

Thanks,

-- Steve