On 02/28/2014 04:50 AM, Steven Rostedt wrote:
> 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.
Thanks，I‘ll send another patch to fix it.
>
> -- 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
|