Date: Fri, 17 Jul 2015 12:17:59 -0400
From: Steven Rostedt <rostedt@goodmis.org>
To: Wang Nan <wangnan0@huawei.com>
Cc: <hekuang@huawei.com>, <ast@plumgrid.com>, <acme@kernel.org>,
        <a.p.zijlstra@chello.nl>, <mingo@redhat.com>, <namhyung@kernel.org>,
        <jolsa@kernel.org>, <pi3orama@163.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] tools lib traceevent: Fix double free corruption in
 error processing code
Message-ID: <20150717121759.015998a8@gandalf.local.home>
In-Reply-To: <1437119926-17022-1-git-send-email-wangnan0@huawei.com>
References: <1437119926-17022-1-git-send-email-wangnan0@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4108
Lines: 105

On Fri, 17 Jul 2015 07:58:46 +0000
Wang Nan <wangnan0@huawei.com> wrote:

> When kernel introduces new function to 'format' file before traceevent
> update correspondingly, a double-free corruption occures if the newly
> intorduced function resides in following format:
> 
> # cat /sys/kernel/debug/tracing/events/bpf/bpf_output_data/format
> ...
> print fmt: "%s", __print_hex(__get_dynamic_array(buf), __get_dynamic_array_len(buf))
> ...
> 
> (where __get_dynamic_array_len is the new function)
> 
> And:
>  # perf record -e bpf:bpf_output_data ls
>    Warning: [bpf:bpf_output_data] function __get_dynamic_array_len not defined
>    Warning: Error: expected type 5 but read 0
>  *** Error in `/path/to/perf': double free or corruption (fasttop): 0x0000000001821210 ***
>  ======= Backtrace: =========
>  /lib64/libc.so.6(+0x6eeef)[0x7f86a4cf6eef]
>  /lib64/libc.so.6(+0x78cae)[0x7f86a4d00cae]
>  /lib64/libc.so.6(+0x79987)[0x7f86a4d01987]
>  /path/to/perf[0x51b0e2]
>  /path/to/perf[0x51afc2]
>  /path/to/perf[0x51fe67]
>  /path/to/perf[0x5200a5]
>  /path/to/perf(__pevent_parse_format+0x185)[0x525bc9]
>  /path/to/perf[0x525d82]
>  /path/to/perf(pevent_parse_format+0x3b)[0x525e15]
>  /path/to/perf[0x4ceb61]
>  /path/to/perf(perf_evsel__newtp_idx+0x9f)[0x48cabf]
>  /path/to/perf(parse_events_add_tracepoint+0x25b)[0x49782b]
>  /path/to/perf(parse_events_parse+0x10a3)[0x4c8243]
>  /path/to/perf(parse_events+0x75)[0x498ce5]
>  /path/to/perf(parse_events_option+0x41)[0x498df1]
>  /path/to/perf[0x493e9b]
>  /path/to/perf(parse_options+0x215)[0x4957c5]
>  /path/to/perf(cmd_record+0x6a)[0x43c4ba]
>  /path/to/perf[0x47b2a3]
>  /path/to/perf(main+0x5f6)[0x42fed6]
>  /lib64/libc.so.6(__libc_start_main+0xf5)[0x7f86a4ca9bd5]
>  /path/to/perf[0x430005]
>  ======= Memory map: ========
>  00400000-006b9000 r-xp 00000000 08:05 23596607                           /path/to/perf
>  008b9000-00903000 rw-p 002b9000 08:05 23596607                           /path/to/perf
> 
> This is caused by error processing code in process_hex() which frees
> arg->hex.field but doesn't set it to NULL. When event_read_print_args()
> freeing the hex arg, the dangling pointer will be free again and cause
> the above error. process_int_array() has similar problem.
> 
> This patch fixes the dangling pointer problem by not setting them until
> everything is okay.

I actually stumbled over this exact bug in trace-cmd but came up with a
much simpler solution:

-- Steve

>From 1d44d0f3361b70724b63c8aab20992704557e9c0 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
Date: Fri, 17 Jul 2015 12:07:23 -0400
Subject: [PATCH] tools lib traceevent: Set int_array fields to NULL if freeing from error

Had a bug where on error of parsing __print_array() where the
fields are freed after they were allocated, but since they were not
set to NULL, the freeing of the arg also tried to free the already
freed fields causing a double free.

Fix process_hex() while at it.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 tools/lib/traceevent/event-parse.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-trace.git/tools/lib/traceevent/event-parse.c
===================================================================
--- linux-trace.git.orig/tools/lib/traceevent/event-parse.c	2015-07-15 14:49:03.275290348 -0400
+++ linux-trace.git/tools/lib/traceevent/event-parse.c	2015-07-17 12:16:54.535359526 -0400
@@ -2559,6 +2559,7 @@ process_hex(struct event_format *event,
 
 free_field:
 	free_arg(arg->hex.field);
+	arg->hex.field = NULL;
 out:
 	*tok = NULL;
 	return EVENT_ERROR;
@@ -2583,8 +2584,10 @@ process_int_array(struct event_format *e
 
 free_size:
 	free_arg(arg->int_array.count);
+	arg->int_array.count = NULL;
 free_field:
 	free_arg(arg->int_array.field);
+	arg->int_array.field = NULL;
 out:
 	*tok = NULL;
 	return EVENT_ERROR;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/