Re: [PATCH] perf report: Fix a memory corrupton issue when enabling --branch-history
On Fri, Feb 16, 2018 at 10:25:31AM +0800, Jin, Yao wrote: SNIP > > 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 > > > > > thanks, > > > jirka > > > > > > > > > --- > > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c > > > index b6140950301e..b50b7b70dcca 100644 > > > --- a/tools/perf/util/hist.c > > > +++ b/tools/perf/util/hist.c > > > @@ -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? yep, later today jirka
Re: [PATCH] perf report: Fix a memory corrupton issue when enabling --branch-history
On 2/13/2018 10:00 PM, Jin, Yao wrote: On 2/13/2018 5:45 PM, Jiri Olsa wrote: On Tue, Feb 13, 2018 at 04:44:28PM +0800, Jin Yao wrote: Following command lines will cause perf crash. perf record -j call -g -a perf report --branch-history *** Error in `perf': double free or corruption (!prev): 0x104aa040 *** === 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). 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. 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. 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 thanks, jirka --- diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index b6140950301e..b50b7b70dcca 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -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
Re: [PATCH] perf report: Fix a memory corrupton issue when enabling --branch-history
On 2/13/2018 5:45 PM, Jiri Olsa wrote: On Tue, Feb 13, 2018 at 04:44:28PM +0800, Jin Yao wrote: Following command lines will cause perf crash. perf record -j call -g -a perf report --branch-history *** Error in `perf': double free or corruption (!prev): 0x104aa040 *** === 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). 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. 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. 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 thanks, jirka --- diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index b6140950301e..b50b7b70dcca 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -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;
Re: [PATCH] perf report: Fix a memory corrupton issue when enabling --branch-history
On Tue, Feb 13, 2018 at 04:44:28PM +0800, Jin Yao wrote: > Following command lines will cause perf crash. > > perf record -j call -g -a > perf report --branch-history > > *** Error in `perf': double free or corruption (!prev): 0x104aa040 *** > === 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). 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 --- diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index b6140950301e..b50b7b70dcca 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -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;
[PATCH] perf report: Fix a memory corrupton issue when enabling --branch-history
Following command lines will cause perf crash. perf record -j call -g -a perf report --branch-history *** Error in `perf': double free or corruption (!prev): 0x104aa040 *** === 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 --- tools/perf/util/hist.c | 21 + 1 file changed, 21 insertions(+) diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index b614095..71f07d2 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -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); } -- 2.7.4