Skip to contentSkip to conversation

perf report: Fix a memory corrupton issue when enabling --branch-history

PATCH In review Jin Yao 4 participants 8 messages Feb 13, 2018 – Feb 17, 2018 perf events

lore
Share & tools
lore

Jin Yao

author
#0 lore raw

PATCH 1/1 perf report: Fix a memory corrupton issue when enabling --branch-history

Following command lines will cause perf crash.

perf record -j call -g -a <application>
perf report --branch-history

*** Error in `perf': double free or corruption (!prev): 0x00000000104aa040 *** ======= Backtrace: ========= /lib/x86_64-linux-gnu/libc.so.6(+0x77725)[0x7f6b37254725] /lib/x86_64-linux-gnu/libc.so.6(+0x7ff4a)[0x7f6b3725cf4a] /lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f6b37260abc] perf[0x51b914]
perf(hist_entry_iter__add+0x1e5)[0x51f305]
perf[0x43cf01]
perf[0x4fa3bf]
perf[0x4fa923]
perf[0x4fd396]
perf[0x4f9614]
perf(perf_session__process_events+0x89e)[0x4fc38e] perf(cmd_report+0x15d2)[0x43f202]
perf[0x4a059f]
perf(main+0x631)[0x427b71] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f6b371fd830] perf(_start+0x29)[0x427d89]

The memory corruption happens at:

iter_add_next_cumulative_entry()

{
        ...
        for (i = 0; i < iter->curr; i++) {
        ...
}

Whatever in iter_next_cumulative_entry() or in iter_add_next_cumulative_entry(), they all don't check if iter->curr exceeds the array 'he_cache[]'.

If there are too many nodes in callchain, it's possible that iter->curr > iter->max_stack, then memory corruption occurs.

This patch will reallocate array 'he_cache[]' in iter_next_cumulative_entry() if necessary (the case of too many nodes in callchain).

Signed-off-by Jin Yao <[email protected]>
---
 tools/perf/util/hist.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
tools/perf/util/hist.c +21 -0
index b614095..71f07d2 100644
@@ -926,11 +926,32 @@ iter_next_cumulative_entry(struct hist_entry_iter *iter,
 			   struct addr_location *al)
 {
 	struct callchain_cursor_node *node;
+	struct hist_entry **tmp;
+	int i;
 
 	node = callchain_cursor_current(&callchain_cursor);
 	if (node == NULL)
 		return 0;
 
+	/*
+	 * If there are too many nodes in callchain,
+	 * increase the size of he_cache[].
+	 */
+	if (iter->curr == iter->max_stack) {
+		i = 2 * iter->max_stack + 1;
+		tmp = realloc(iter->priv, sizeof(struct hist_entry *) * i);
+		if (tmp == NULL) {
+			/*
+			 * No need to free iter->priv here. It will be
+			 * freed in iter_finish_cumulative_entry.
+			 */
+			return 0;
+		}
+
+		iter->priv = tmp;
+		iter->max_stack = i;
+	}
+
 	return fill_callchain_info(al, node, iter->hide_unresolved);
 }
Signature · 1 line
2.7.4
#1 lore raw
Quoted 33 lines · Jin Yao

Following command lines will cause perf crash.

perf record -j call -g -a <application>
perf report --branch-history

*** Error in `perf': double free or corruption (!prev): 0x00000000104aa040 *** ======= Backtrace: ========= /lib/x86_64-linux-gnu/libc.so.6(+0x77725)[0x7f6b37254725] /lib/x86_64-linux-gnu/libc.so.6(+0x7ff4a)[0x7f6b3725cf4a] /lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f6b37260abc] perf[0x51b914]
perf(hist_entry_iter__add+0x1e5)[0x51f305]
perf[0x43cf01]
perf[0x4fa3bf]
perf[0x4fa923]
perf[0x4fd396]
perf[0x4f9614]
perf(perf_session__process_events+0x89e)[0x4fc38e] perf(cmd_report+0x15d2)[0x43f202]
perf[0x4a059f]
perf(main+0x631)[0x427b71] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f6b371fd830] perf(_start+0x29)[0x427d89]

The memory corruption happens at:

iter_add_next_cumulative_entry()

{
        ...
        for (i = 0; i < iter->curr; i++) {
        ...
}
Quoted 8 lines

Whatever in iter_next_cumulative_entry() or in iter_add_next_cumulative_entry(), they all don't check if iter->curr exceeds the array 'he_cache[]'.

If there are too many nodes in callchain, it's possible that iter->curr > iter->max_stack, then memory corruption occurs.

This patch will reallocate array 'he_cache[]' in iter_next_cumulative_entry() if necessary (the case of too many nodes in callchain).

right, the max_stack does not say how many nodes end up in callchain_cursor at the end.. good catch, please mention that also in the changelog

however we know the final count from callchain_cursor itself, the attached patch might do the same job, right?

also could we now get rid of iter->max_stack?

thanks,
jirka

---

tools/perf/util/hist.c +1 -1
index b6140950301e..b50b7b70dcca 100644
@@ -879,7 +879,7 @@ iter_prepare_cumulative_entry(struct hist_entry_iter *iter,
 	 * cumulated only one time to prevent entries more than 100%
 	 * overhead.
 	 */
-	he_cache = malloc(sizeof(*he_cache) * (iter->max_stack + 1));
+	he_cache = malloc(sizeof(*he_cache) * (callchain_cursor.nr + 1));
 	if (he_cache == NULL)
 		return -ENOMEM;
author
#2 lore raw
Quoted 40 lines · Jiri Olsa
Quoted 39 lines · Jin Yao

Following command lines will cause perf crash.

perf record -j call -g -a <application>
perf report --branch-history

*** Error in `perf': double free or corruption (!prev): 0x00000000104aa040 *** ======= Backtrace: ========= /lib/x86_64-linux-gnu/libc.so.6(+0x77725)[0x7f6b37254725] /lib/x86_64-linux-gnu/libc.so.6(+0x7ff4a)[0x7f6b3725cf4a] /lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f6b37260abc] perf[0x51b914]
perf(hist_entry_iter__add+0x1e5)[0x51f305]
perf[0x43cf01]
perf[0x4fa3bf]
perf[0x4fa923]
perf[0x4fd396]
perf[0x4f9614]
perf(perf_session__process_events+0x89e)[0x4fc38e] perf(cmd_report+0x15d2)[0x43f202]
perf[0x4a059f]
perf(main+0x631)[0x427b71] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f6b371fd830] perf(_start+0x29)[0x427d89]

The memory corruption happens at:

iter_add_next_cumulative_entry()

{
         ...
         for (i = 0; i < iter->curr; i++) {
         ...
}

Whatever in iter_next_cumulative_entry() or in iter_add_next_cumulative_entry(), they all don't check if iter->curr exceeds the array 'he_cache[]'.

If there are too many nodes in callchain, it's possible that iter->curr > iter->max_stack, then memory corruption occurs.

Quoted 7 lines
Quoted 2 lines

This patch will reallocate array 'he_cache[]' in iter_next_cumulative_entry() if necessary (the case of too many nodes in callchain).

right, the max_stack does not say how many nodes end up in callchain_cursor at the end.. good catch, please mention that also in the changelog

max_stack looks only to limit the number of calls but not for other branches.

Quoted 3 lines

however we know the final count from callchain_cursor itself, the attached patch might do the same job, right?

I think the attached patch is ok.

Quoted 2 lines

also could we now get rid of iter->max_stack?

From my opinion, the option '--max-stack' in perf report looks not very necessary. While it's just my personal opinion, need to hear from more people. :)

Thanks
Jin Yao

Quoted 19 lines

thanks,
jirka

---

tools/perf/util/hist.c +1 -1
index b6140950301e..b50b7b70dcca 100644
@@ -879,7 +879,7 @@ iter_prepare_cumulative_entry(struct hist_entry_iter *iter,
  	 * cumulated only one time to prevent entries more than 100%
  	 * overhead.
  	 */
-	he_cache = malloc(sizeof(*he_cache) * (iter->max_stack + 1));
+	he_cache = malloc(sizeof(*he_cache) * (callchain_cursor.nr + 1));
  	if (he_cache == NULL)
  		return -ENOMEM;
author
#3 lore raw
Quoted 72 lines · Yao
Quoted 40 lines · Jiri Olsa
Quoted 39 lines · Jin Yao

Following command lines will cause perf crash.

perf record -j call -g -a <application>
perf report --branch-history

*** Error in `perf': double free or corruption (!prev): 0x00000000104aa040 *** ======= Backtrace: ========= /lib/x86_64-linux-gnu/libc.so.6(+0x77725)[0x7f6b37254725] /lib/x86_64-linux-gnu/libc.so.6(+0x7ff4a)[0x7f6b3725cf4a] /lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f6b37260abc] perf[0x51b914]
perf(hist_entry_iter__add+0x1e5)[0x51f305]
perf[0x43cf01]
perf[0x4fa3bf]
perf[0x4fa923]
perf[0x4fd396]
perf[0x4f9614]
perf(perf_session__process_events+0x89e)[0x4fc38e] perf(cmd_report+0x15d2)[0x43f202]
perf[0x4a059f]
perf(main+0x631)[0x427b71] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f6b371fd830] perf(_start+0x29)[0x427d89]

The memory corruption happens at:

iter_add_next_cumulative_entry()
{
         ...
         for (i = 0; i < iter->curr; i++) {
         ...
}

Whatever in iter_next_cumulative_entry() or in iter_add_next_cumulative_entry(), they all don't check if iter->curr exceeds the array 'he_cache[]'.

If there are too many nodes in callchain, it's possible that iter->curr > iter->max_stack, then memory corruption occurs.

Quoted 7 lines
Quoted 2 lines

This patch will reallocate array 'he_cache[]' in iter_next_cumulative_entry() if necessary (the case of too many nodes in callchain).

right, the max_stack does not say how many nodes end up in callchain_cursor at the end.. good catch, please mention that also in the changelog

max_stack looks only to limit the number of calls but not for other branches.

Quoted 3 lines

however we know the final count from callchain_cursor itself, the attached patch might do the same job, right?

I think the attached patch is ok.

Quoted 2 lines

also could we now get rid of iter->max_stack?

From my opinion, the option '--max-stack' in perf report looks not very
necessary. While it's just my personal opinion, need to hear from more people. :)

Thanks
Jin Yao

Quoted 4 lines

thanks,
jirka

Quoted 15 lines
Quoted 7 lines

---

tools/perf/util/hist.c +0 -0
index b6140950301e..b50b7b70dcca 100644

tools/perf/util/hist.c iter_prepare_cumulative_entry(struct · line 885

Quoted 8 lines
tools/perf/util/hist.c +1 -1
@@ -879,7 +879,7 @@ iter_prepare_cumulative_entry(struct 
 hist_entry_iter *iter,
        * cumulated only one time to prevent entries more than 100%
        * overhead.
        */
-    he_cache = malloc(sizeof(*he_cache) * (iter->max_stack + 1));
+    he_cache = malloc(sizeof(*he_cache) * (callchain_cursor.nr + 1));
       if (he_cache == NULL)
           return -ENOMEM;

Hi Jiri,

I guess you will post this patch, right?

Thanks
Jin Yao

#4 lore raw

On Fri, Feb 16, 2018 at 10:25:31AM +0800, Jin, Yao wrote:

SNIP

Quoted 29 lines
Quoted 11 lines

From my opinion, the option '--max-stack' in perf report looks not very
necessary. While it's just my personal opinion, need to hear from more people. :)

Thanks
Jin Yao

Quoted 4 lines

thanks,
jirka

Quoted 15 lines
Quoted 7 lines

---

tools/perf/util/hist.c +0 -0
index b6140950301e..b50b7b70dcca 100644

tools/perf/util/hist.c iter_prepare_cumulative_entry(struct · line 885

Quoted 8 lines
tools/perf/util/hist.c +1 -1
@@ -879,7 +879,7 @@ iter_prepare_cumulative_entry(struct
 hist_entry_iter *iter,
        * cumulated only one time to prevent entries more than 100%
        * overhead.
        */
-    he_cache = malloc(sizeof(*he_cache) * (iter->max_stack + 1));
+    he_cache = malloc(sizeof(*he_cache) * (callchain_cursor.nr + 1));
       if (he_cache == NULL)
           return -ENOMEM;

Hi Jiri,

Quoted 1 line

I guess you will post this patch, right?

yep, later today

jirka

#5 lore raw

PATCH 1/1 perf report: Fix memory corruption in --branch-history mode --branch-history

Quoted 33 lines · Jiri Olsa

On Fri, Feb 16, 2018 at 10:25:31AM +0800, Jin, Yao wrote:

SNIP

Quoted 29 lines
Quoted 11 lines

From my opinion, the option '--max-stack' in perf report looks not very
necessary. While it's just my personal opinion, need to hear from more people. :)

Thanks
Jin Yao

Quoted 4 lines

thanks,
jirka

Quoted 15 lines
Quoted 7 lines

---

tools/perf/util/hist.c +0 -0
index b6140950301e..b50b7b70dcca 100644

tools/perf/util/hist.c iter_prepare_cumulative_entry(struct · line 885

Quoted 8 lines
tools/perf/util/hist.c +1 -1
@@ -879,7 +879,7 @@ iter_prepare_cumulative_entry(struct
 hist_entry_iter *iter,
        * cumulated only one time to prevent entries more than 100%
        * overhead.
        */
-    he_cache = malloc(sizeof(*he_cache) * (iter->max_stack + 1));
+    he_cache = malloc(sizeof(*he_cache) * (callchain_cursor.nr + 1));
       if (he_cache == NULL)
           return -ENOMEM;

Hi Jiri,

Quoted 3 lines
Quoted 1 line

I guess you will post this patch, right?

yep, later today

here it is.. I think we want this change now to fix the crash, and some more fixes later to ensure that the branch stack code follows properly the logic of --max-stack, which is not the case now

thanks,
jirka

---
Jin Yao reported memory corrupton in perf report with branch info used for stack trace:

Quoted 15 lines

Following command lines will cause perf crash.

perf record -j call -g -a <application>
perf report --branch-history

*** Error in `perf': double free or corruption (!prev): 0x00000000104aa040 *** ======= Backtrace: ========= /lib/x86_64-linux-gnu/libc.so.6(+0x77725)[0x7f6b37254725] /lib/x86_64-linux-gnu/libc.so.6(+0x7ff4a)[0x7f6b3725cf4a] /lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f6b37260abc] perf[0x51b914]
perf(hist_entry_iter__add+0x1e5)[0x51f305]
perf[0x43cf01]
perf[0x4fa3bf]
perf[0x4fa923]

Quoted 8 lines

perf[0x4fd396]
perf[0x4f9614] perf(perf_session__process_events+0x89e)[0x4fc38e] perf(cmd_report+0x15d2)[0x43f202]
perf[0x4a059f]
perf(main+0x631)[0x427b71] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f6b371fd830] perf(_start+0x29)[0x427d89]

For the cumulative output, we allocate he_cache array based on the --max-stack option value and populate it with data from callchain_cursor.

The --max-stack option value does not ensure now the limit for number of callchain_cursor nodes, so the cumulative iter code will allocate smaller array than it's actually needed and cause above corruption.

I think the --max-stack limit does not apply here anyway, because we add callchain data as normal hist entries, while the --max-stack control the limit of single entry callchain depth.

Using the callchain_cursor.nr as he_cache array count to fix this. Also removing struct hist_entry_iter::max_stack, because there's no longer any use for it.

We need more fixes to ensure that the branch stack code follows properly the logic of --max-stack, which is not the case at the moment.

Reported-by Jin Yao <[email protected]>
Original-patch-by Jin Yao <[email protected]>
Link http://lkml.kernel.org/n/[email protected] open here
Signed-off-by Jiri Olsa <[email protected]>
---
 tools/perf/util/hist.c | 4 +---
 tools/perf/util/hist.h | 1 -
 2 files changed, 1 insertion(+), 4 deletions(-)
tools/perf/util/hist.c +1 -3
index b6140950301e..44a8456cea10 100644
@@ -879,7 +879,7 @@ iter_prepare_cumulative_entry(struct hist_entry_iter *iter,
 	 * cumulated only one time to prevent entries more than 100%
 	 * overhead.
 	 */
-	he_cache = malloc(sizeof(*he_cache) * (iter->max_stack + 1));
+	he_cache = malloc(sizeof(*he_cache) * (callchain_cursor.nr + 1));
 	if (he_cache == NULL)
 		return -ENOMEM;
 

@@ -1045,8 +1045,6 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al,
 	if (err)
 		return err;
 
-	iter->max_stack = max_stack_depth;
-
 	err = iter->ops->prepare_entry(iter, al);
 	if (err)
 		goto out;
tools/perf/util/hist.h +0 -1
index 02721b579746..e869cad4d89f 100644
@@ -107,7 +107,6 @@ struct hist_entry_iter {
 	int curr;
 
 	bool hide_unresolved;
-	int max_stack;
 
 	struct perf_evsel *evsel;
 	struct perf_sample *sample;
Signature · 1 line
2.13.6
maintainer Applied
#6 lore raw

Re: [PATCH] perf report: Fix memory corruption in --branch-history mode --branch-history

Em Fri, Feb 16, 2018 at 01:36:19PM +0100, Jiri Olsa escreveu:

Quoted 4 lines

here it is.. I think we want this change now to fix the crash, and some more fixes later to ensure that the branch stack code follows properly the logic of --max-stack, which is not the case now

Thanks, applied.

- Arnaldo

merge bot
#7 lore raw

[tip:perf/core] perf report: Fix memory corruption in --branch-history mode --branch-history

In reply to Jiri Olsa
perf report: Fix memory corruption in --branch-history mode --branch-history
Commit-ID:  e3ebaa465136ecfedf9c6f4671df02bf625f8125
Gitweb:     https://git.kernel.org/tip/e3ebaa465136ecfedf9c6f4671df02bf625f8125
Author:     Jiri Olsa <[email protected]>
AuthorDate: Fri, 16 Feb 2018 13:36:19 +0100
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 16 Feb 2018 14:55:47 -0300

perf report: Fix memory corruption in --branch-history mode --branch-history

Jin Yao reported memory corrupton in perf report with branch info used for stack trace:

Quoted 15 lines

Following command lines will cause perf crash.

perf record -j call -g -a <application>
perf report --branch-history

*** Error in `perf': double free or corruption (!prev): 0x00000000104aa040 *** ======= Backtrace: ========= /lib/x86_64-linux-gnu/libc.so.6(+0x77725)[0x7f6b37254725] /lib/x86_64-linux-gnu/libc.so.6(+0x7ff4a)[0x7f6b3725cf4a] /lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f6b37260abc] perf[0x51b914]
perf(hist_entry_iter__add+0x1e5)[0x51f305]
perf[0x43cf01]
perf[0x4fa3bf]
perf[0x4fa923]

Quoted 8 lines

perf[0x4fd396]
perf[0x4f9614] perf(perf_session__process_events+0x89e)[0x4fc38e] perf(cmd_report+0x15d2)[0x43f202]
perf[0x4a059f]
perf(main+0x631)[0x427b71] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f6b371fd830] perf(_start+0x29)[0x427d89]

For the cumulative output, we allocate the he_cache array based on the --max-stack option value and populate it with data from 'callchain_cursor'.

The --max-stack option value does not ensure now the limit for number of callchain_cursor nodes, so the cumulative iter code will allocate smaller array than it's actually needed and cause above corruption.

I think the --max-stack limit does not apply here anyway, because we add callchain data as normal hist entries, while the --max-stack control the limit of single entry callchain depth.

Using the callchain_cursor.nr as he_cache array count to fix this. Also removing struct hist_entry_iter::max_stack, because there's no longer any use for it.

We need more fixes to ensure that the branch stack code follows properly the logic of --max-stack, which is not the case at the moment.

Original-patch-by Jin Yao <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
Reported-by Jin Yao <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Kan Liang <[email protected]>
Cc Peter Zijlstra <[email protected]>
Link http://lkml.kernel.org/r/20180216123619.GA9945@krava open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/hist.c | 4 +---
 tools/perf/util/hist.h | 1 -
 2 files changed, 1 insertion(+), 4 deletions(-)
tools/perf/util/hist.c +1 -3
index b614095..44a8456 100644
@@ -879,7 +879,7 @@ iter_prepare_cumulative_entry(struct hist_entry_iter *iter,
 	 * cumulated only one time to prevent entries more than 100%
 	 * overhead.
 	 */
-	he_cache = malloc(sizeof(*he_cache) * (iter->max_stack + 1));
+	he_cache = malloc(sizeof(*he_cache) * (callchain_cursor.nr + 1));
 	if (he_cache == NULL)
 		return -ENOMEM;
 

@@ -1045,8 +1045,6 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al,
 	if (err)
 		return err;
 
-	iter->max_stack = max_stack_depth;
-
 	err = iter->ops->prepare_entry(iter, al);
 	if (err)
 		goto out;
tools/perf/util/hist.h +0 -1
index 02721b57..e869cad 100644
@@ -107,7 +107,6 @@ struct hist_entry_iter {
 	int curr;
 
 	bool hide_unresolved;
-	int max_stack;
 
 	struct perf_evsel *evsel;
 	struct perf_sample *sample;