[PATCH] trace-input: Fix a memory double free

[Chen Ditang]

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 <***@cn.fujitsu.com>
---
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);
- }
- 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))

[Steven Rostedt]

On Thu, 27 Feb 2014 13:18:48 +0800

Post by Chen Ditang
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]
---
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;
-
- 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

Post by Chen Ditang
- }
- 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 ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ditang Chen]

Post by Steven Rostedt
On Thu, 27 Feb 2014 13:18:48 +0800
=20

Reads a wrong trace.dat file, the read_cpu_data() function returns a=

=20

Post by Steven Rostedt

failure, it will cause memory double free.
# ./trace-graph ../../trace.dat
version =3D 6
File possibly truncated. Need at least 18446744073709551614, but fil=

e size is 3564371.

Post by Steven Rostedt

*** Error in `./trace-graph': double free or corruption (fasttop): 0=

x000000000262a6e0 ***

Post by Steven Rostedt

=3D=3D=3D=3D=3D=3D=3D Backtrace: =3D=3D=3D=3D=3D=3D=3D=3D=3D
/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]
---
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)

Post by Steven Rostedt

=20
handle->cpu_data[cpu].kbuf =3D 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);
=20
@@ -1857,22 +1857,14 @@ static int read_cpu_data(struct tracecmd_inp=

ut *handle)

Post by Steven Rostedt

"Need at least %llu, but file size is %zu.\n",
offset + size, handle->total_file_size);
errno =3D EINVAL;
- goto out_free;
+ return -1;
}
=20
if (init_cpu(handle, cpu))
- goto out_free;
+ return -1;
}
=20
return 0;
-
- for ( ; cpu >=3D 0; cpu--) {
- free_page(handle, cpu);
- kbuffer_free(handle->cpu_data[cpu].kbuf);

=20
These frees are still required. But you did uncover a real bug.
=20
kbuffer_free(handle->cpu_data[cpu].kbuf);
handle->cpu_data[cpu].kbuf =3D NULL;
=20
Because free() and all the other freeing functions should allow for
NULL to be passed, and it should then be ignored.

Thanks=EF=BC=8CI=E2=80=98ll send another patch to fix it.

Post by Steven Rostedt
=20
-- Steve
=20

- }
- return -1;
-
}
=20
static int read_data_and_size(struct tracecmd_input *handle,
@@ -2209,7 +2201,7 @@ void tracecmd_close(struct tracecmd_input *han=

dle)

Post by Steven Rostedt

/* 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);
=20
if (!list_empty(&handle->cpu_data[cpu].pages))

=20
=20

--
To unsubscribe from this list: send the line "unsubscribe linux-trace-u=
sers" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ditang Chen]

Reads a wrong trace.dat file, the read_cpu_data() function returns a
failure, it will cause memory double free. we should init kbuf back to
NULL after free kbuf.

and if kbuffer_alloc() fail, do not need to free any more.


Signed-off-by: Ditang Chen <***@cn.fujitsu.com>
---
trace-input.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/trace-input.c b/trace-input.c
index 6eef168..7bebbfe 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);

@@ -1870,6 +1870,7 @@ static int read_cpu_data(struct tracecmd_input *handle)
for ( ; cpu >= 0; cpu--) {
free_page(handle, cpu);
kbuffer_free(handle->cpu_data[cpu].kbuf);
+ handle->cpu_data[cpu].kbuf = NULL;
}
return -1;

@@ -2209,7 +2210,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))

[Steven Rostedt]

On Fri, 28 Feb 2014 11:24:19 +0800

Post by Chen Ditang
Reads a wrong trace.dat file, the read_cpu_data() function returns a
failure, it will cause memory double free. we should init kbuf back to
NULL after free kbuf.
and if kbuffer_alloc() fail, do not need to free any more.
---
trace-input.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/trace-input.c b/trace-input.c
index 6eef168..7bebbfe 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);
@@ -1870,6 +1870,7 @@ static int read_cpu_data(struct tracecmd_input *handle)
for ( ; cpu >= 0; cpu--) {
free_page(handle, cpu);
kbuffer_free(handle->cpu_data[cpu].kbuf);
+ handle->cpu_data[cpu].kbuf = NULL;
}

This is actually the only change needed. The above is in a loop and we
may have allocated pages, so it should still do the goto_free.

Post by Chen Ditang
return -1;
@@ -2209,7 +2210,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) {

Hmm, this gives a tight coupling between read_cpu_data() and here, as
it depends on kbuf being the first allocated. But I see that that also
denotes that init_cpu() wasn't called to initialize the .pages list.

OK, keep this change too. That is, please send another patch without
the first hunk.

Thanks,

-- Steve

Post by Chen Ditang
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 ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ditang Chen]

Reads a wrong trace.dat file, the read_cpu_data() function returns a
failure, it will cause memory double free. we should init kbuf back to
NULL after free kbuf.

Signed-off-by: Ditang Chen <***@cn.fujitsu.com>
---
trace-input.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/trace-input.c b/trace-input.c
index 6eef168..b91ea50 100644
--- a/trace-input.c
+++ b/trace-input.c
@@ -1870,6 +1870,7 @@ static int read_cpu_data(struct tracecmd_input *handle)
for ( ; cpu >= 0; cpu--) {
free_page(handle, cpu);
kbuffer_free(handle->cpu_data[cpu].kbuf);
+ handle->cpu_data[cpu].kbuf = NULL;
}
return -1;

@@ -2209,7 +2210,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))

[Steven Rostedt]

On Fri, 28 Feb 2014 13:50:42 +0800

Post by Chen Ditang
Reads a wrong trace.dat file, the read_cpu_data() function returns a
failure, it will cause memory double free. we should init kbuf back to
NULL after free kbuf.

Thanks, I applied it and I'm currently testing it.

Just a note. This patch has severe whitespace issues (tabs converted to
spaces). But because it was small, I was able to cope.

-- Steve

Post by Chen Ditang
---
trace-input.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/trace-input.c b/trace-input.c
index 6eef168..b91ea50 100644
--- a/trace-input.c
+++ b/trace-input.c
@@ -1870,6 +1870,7 @@ static int read_cpu_data(struct tracecmd_input *handle)
for ( ; cpu >= 0; cpu--) {
free_page(handle, cpu);
kbuffer_free(handle->cpu_data[cpu].kbuf);
+ handle->cpu_data[cpu].kbuf = NULL;
}
return -1;
@@ -2209,7 +2210,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 ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html