On Thu, 27 Feb 2014 13:18:48 +0800
Chen Ditang <chendt.fnst@xxxxxxxxxxxxxx> wrote:
> Reads a wrong trace.dat file, the read_cpu_data() function returns a
> failure, it will cause memory double free.
>
> # ./trace-graph ../../trace.dat
> version = 6
> File possibly truncated. Need at least 18446744073709551614, but file size is 3564371.
> *** Error in `./trace-graph': double free or corruption (fasttop): 0x000000000262a6e0 ***
> ======= Backtrace: =========
> /lib64/libc.so.6[0x387b27cef8]
> ./trace-graph(kbuffer_free+0x18)[0x434f78]
> ./trace-graph(tracecmd_close+0xca)[0x432f55]
> ./trace-graph(tracecmd_open_fd+0x5d)[0x432e22]
> ./trace-graph(tracecmd_open+0x3c)[0x432e65]
> ./trace-graph(trace_graph+0x148)[0x40a685]
> ./trace-graph(main+0x20)[0x40adee]
> /lib64/libc.so.6(__libc_start_main+0xf5)[0x387b221b75]
> ./trace-graph[0x40a029]
>
> Signed-off-by: Ditang Chen <chendt.fnst@xxxxxxxxxxxxxx>
> ---
> trace-input.c | 16 ++++------------
> 1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/trace-input.c b/trace-input.c
> index 6eef168..8493495 100644
> --- a/trace-input.c
> +++ b/trace-input.c
> @@ -1841,7 +1841,7 @@ static int read_cpu_data(struct tracecmd_input *handle)
>
> handle->cpu_data[cpu].kbuf = kbuffer_alloc(long_size, endian);
> if (!handle->cpu_data[cpu].kbuf)
> - goto out_free;
> + return -1;
> if (pevent->old_format)
> kbuffer_set_old_format(handle->cpu_data[cpu].kbuf);
>
> @@ -1857,22 +1857,14 @@ static int read_cpu_data(struct tracecmd_input *handle)
> "Need at least %llu, but file size is %zu.\n",
> offset + size, handle->total_file_size);
> errno = EINVAL;
> - goto out_free;
> + return -1;
> }
>
> if (init_cpu(handle, cpu))
> - goto out_free;
> + return -1;
> }
>
> return 0;
> -
> - out_free:
> - for ( ; cpu >= 0; cpu--) {
> - free_page(handle, cpu);
> - kbuffer_free(handle->cpu_data[cpu].kbuf);
These frees are still required. But you did uncover a real bug.
Though, the real fix to it is to init kbuf back to NULL:
kbuffer_free(handle->cpu_data[cpu].kbuf);
handle->cpu_data[cpu].kbuf = NULL;
Because free() and all the other freeing functions should allow for
NULL to be passed, and it should then be ignored.
-- Steve
> - }
> - return -1;
> -
> }
>
> static int read_data_and_size(struct tracecmd_input *handle,
> @@ -2209,7 +2201,7 @@ void tracecmd_close(struct tracecmd_input *handle)
> /* The tracecmd_peek_data may have cached a record */
> free_next(handle, cpu);
> free_page(handle, cpu);
> - if (handle->cpu_data) {
> + if (handle->cpu_data && handle->cpu_data[cpu].kbuf) {
> kbuffer_free(handle->cpu_data[cpu].kbuf);
>
> if (!list_empty(&handle->cpu_data[cpu].pages))
--
To unsubscribe from this list: send the line "unsubscribe linux-trace-users" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
|