On Thu, 27 Feb 2014 13:18:48 +0800 Chen Ditang <[email protected]> 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 <[email protected]> > --- > 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
