From: Maynard J. <may...@us...> - 2013-03-04 23:22:50
|
Fix seg fault due to incorrect array size initialization The operf_sfile.cpp:create_sfile function creates and initializes the array of 'struct operf_sfile' objects used for writing sample data to oprofile formatted sample files. This function creates an array of these objects, but was incorrectly creating an array of size 'OP_MAX_COUNTER'. Since operf can multiplex events, we aren't limited to OP_MAX_COUNTER events to profile simultaneously, so if the user specifies more than OP_MAX_COUNTER events, the code that accesses this array was going off the end and sometimes seg faulting. This patch fixes the problem by defining OP_MAX_EVENTS to be '24' and using that as the array size. Furthermore, if the user tries to specify more than 24 events to profile, an error message is displayed: Number of events specified is greater than allowed maximum of <n> and operf aborts. Signed-off-by: Maynard Johnson <may...@us...> --- libperf_events/operf_sfile.cpp | 10 +++++----- libperf_events/operf_sfile.h | 3 ++- libperf_events/operf_utils.h | 4 +++- pe_profiling/operf.cpp | 9 +++++++-- 4 files changed, 17 insertions(+), 9 deletions(-) diff --git a/libperf_events/operf_sfile.cpp b/libperf_events/operf_sfile.cpp index bda9138..fbe2df1 100644 --- a/libperf_events/operf_sfile.cpp +++ b/libperf_events/operf_sfile.cpp @@ -151,7 +151,7 @@ create_sfile(unsigned long hash, struct operf_transient const * trans, sf->start_addr = trans->start_addr; sf->end_addr = trans->end_addr; - for (i = 0 ; i < op_nr_counters ; ++i) + for (i = 0 ; i < op_nr_events ; ++i) odb_init(&sf->files[i]); // TODO: handle extended @@ -218,7 +218,7 @@ void operf_sfile_dup(struct operf_sfile * to, struct operf_sfile * from) memcpy(to, from, sizeof (struct operf_sfile)); - for (i = 0 ; i < op_nr_counters ; ++i) + for (i = 0 ; i < op_nr_events ; ++i) odb_init(&to->files[i]); // TODO: handle extended @@ -246,7 +246,7 @@ static odb_t * get_file(struct operf_transient const * trans, int is_cg) return opd_ext_operf_sfile_get(trans, is_cg); */ - if (trans->event >= (int)op_nr_counters) { + if (trans->event >= (int)op_nr_events) { fprintf(stderr, "%s: Invalid counter %d\n", __FUNCTION__, trans->event); abort(); @@ -410,7 +410,7 @@ static int close_sfile(struct operf_sfile * sf, void * data __attribute__((unuse size_t i; /* it's OK to close a non-open odb file */ - for (i = 0; i < op_nr_counters; ++i) + for (i = 0; i < op_nr_events; ++i) odb_close(&sf->files[i]); // TODO: handle extended @@ -432,7 +432,7 @@ static int sync_sfile(struct operf_sfile * sf, void * data __attribute__((unused { size_t i; - for (i = 0; i < op_nr_counters; ++i) + for (i = 0; i < op_nr_events; ++i) odb_sync(&sf->files[i]); // TODO: handle extended diff --git a/libperf_events/operf_sfile.h b/libperf_events/operf_sfile.h index 9b91180..f639212 100644 --- a/libperf_events/operf_sfile.h +++ b/libperf_events/operf_sfile.h @@ -29,6 +29,7 @@ struct operf_kernel_image; #define INVALID_IMAGE "INVALID IMAGE" #define VMA_SHIFT 13 + /** * Each set of sample files (where a set is over the physical counter * types) will have one of these for it. We match against the @@ -61,7 +62,7 @@ struct operf_sfile { /** true if this file should be ignored in profiles */ int ignored; /** opened sample files */ - odb_t files[OP_MAX_COUNTERS]; + odb_t files[OP_MAX_EVENTS]; /** extended sample files */ odb_t * ext_files; /** hash table of opened cg sample files */ diff --git a/libperf_events/operf_utils.h b/libperf_events/operf_utils.h index 2a3cf59..2f3fc95 100644 --- a/libperf_events/operf_utils.h +++ b/libperf_events/operf_utils.h @@ -45,13 +45,15 @@ extern bool throttled; #define MMAP_WINDOW_SZ (32 * 1024 * 1024ULL) #endif +#define OP_MAX_EVENTS 24 + /* A macro to be used for ppc64 architecture-specific code. The '__powerpc__' macro * is defined for both ppc64 and ppc32 architectures, so we must further qualify by * including the 'HAVE_LIBPFM' macro, since that macro will be defined only for ppc64. */ #define PPC64_ARCH (HAVE_LIBPFM) && ((defined(__powerpc__) || defined(__powerpc64__))) -extern unsigned int op_nr_counters; +extern unsigned int op_nr_events; static inline size_t align_64bit(u64 x) { diff --git a/pe_profiling/operf.cpp b/pe_profiling/operf.cpp index e948009..ffd2fbf 100644 --- a/pe_profiling/operf.cpp +++ b/pe_profiling/operf.cpp @@ -67,7 +67,7 @@ operf_read operfRead; op_cpu cpu_type; double cpu_speed; char op_samples_current_dir[PATH_MAX]; -uint op_nr_counters; +uint op_nr_events; verbose vmisc("misc"); uid_t my_uid; bool no_vmlinux; @@ -1291,6 +1291,11 @@ out: static void _process_events_list(void) { string cmd = OP_BINDIR; + if (operf_options::evts.size() > OP_MAX_EVENTS) { + cerr << "Number of events specified is greater than allowed maximum of " + << OP_MAX_EVENTS << "." << endl; + exit(EXIT_FAILURE); + } cmd += "/ophelp --check-events "; for (unsigned int i = 0; i < operf_options::evts.size(); i++) { FILE * fp; @@ -1761,7 +1766,7 @@ static void process_args(int argc, char * const argv[]) } else { _process_events_list(); } - op_nr_counters = events.size(); + op_nr_events = events.size(); if (operf_options::vmlinux.empty()) { no_vmlinux = true; -- 1.7.1 |
From: Maynard J. <may...@us...> - 2013-03-07 15:24:08
|
On 03/04/2013 05:22 PM, Maynard Johnson wrote: > Fix seg fault due to incorrect array size initialization I forgot to mention when I posted this patch that you can reproduce the error by doing something like the following: operf -l -e CPU_CLK_UNHALTED:500000,CPU_CLK_UNHALTED:500000,CPU_CLK_UNHALTED:500000,\ CPU_CLK_UNHALTED:500000,CPU_CLK_UNHALTED:500000,CPU_CLK_UNHALTED:500000,\ CPU_CLK_UNHALTED:500000,CPU_CLK_UNHALTED:500000,CPU_CLK_UNHALTED:500000,\ CPU_CLK_UNHALTED:500000 ./my_app This is a simple straightforward fix to an obvious bug, so I've committed the patch. But if anyone has questions or issues with it, please let me know. -Maynard > > The operf_sfile.cpp:create_sfile function creates and initializes the > array of 'struct operf_sfile' objects used for writing sample data to > oprofile formatted sample files. This function creates an array of > these objects, but was incorrectly creating an array of size 'OP_MAX_COUNTER'. > Since operf can multiplex events, we aren't limited to OP_MAX_COUNTER > events to profile simultaneously, so if the user specifies more than > OP_MAX_COUNTER events, the code that accesses this array was going > off the end and sometimes seg faulting. > > This patch fixes the problem by defining OP_MAX_EVENTS to be '24' and > using that as the array size. Furthermore, if the user tries to specify > more than 24 events to profile, an error message is displayed: > Number of events specified is greater than allowed maximum of <n> > and operf aborts. > > Signed-off-by: Maynard Johnson <may...@us...> > --- > libperf_events/operf_sfile.cpp | 10 +++++----- > libperf_events/operf_sfile.h | 3 ++- > libperf_events/operf_utils.h | 4 +++- > pe_profiling/operf.cpp | 9 +++++++-- > 4 files changed, 17 insertions(+), 9 deletions(-) > > diff --git a/libperf_events/operf_sfile.cpp b/libperf_events/operf_sfile.cpp > index bda9138..fbe2df1 100644 > --- a/libperf_events/operf_sfile.cpp > +++ b/libperf_events/operf_sfile.cpp > @@ -151,7 +151,7 @@ create_sfile(unsigned long hash, struct operf_transient const * trans, > sf->start_addr = trans->start_addr; > sf->end_addr = trans->end_addr; > > - for (i = 0 ; i < op_nr_counters ; ++i) > + for (i = 0 ; i < op_nr_events ; ++i) > odb_init(&sf->files[i]); > > // TODO: handle extended > @@ -218,7 +218,7 @@ void operf_sfile_dup(struct operf_sfile * to, struct operf_sfile * from) > > memcpy(to, from, sizeof (struct operf_sfile)); > > - for (i = 0 ; i < op_nr_counters ; ++i) > + for (i = 0 ; i < op_nr_events ; ++i) > odb_init(&to->files[i]); > > // TODO: handle extended > @@ -246,7 +246,7 @@ static odb_t * get_file(struct operf_transient const * trans, int is_cg) > return opd_ext_operf_sfile_get(trans, is_cg); > */ > > - if (trans->event >= (int)op_nr_counters) { > + if (trans->event >= (int)op_nr_events) { > fprintf(stderr, "%s: Invalid counter %d\n", __FUNCTION__, > trans->event); > abort(); > @@ -410,7 +410,7 @@ static int close_sfile(struct operf_sfile * sf, void * data __attribute__((unuse > size_t i; > > /* it's OK to close a non-open odb file */ > - for (i = 0; i < op_nr_counters; ++i) > + for (i = 0; i < op_nr_events; ++i) > odb_close(&sf->files[i]); > > // TODO: handle extended > @@ -432,7 +432,7 @@ static int sync_sfile(struct operf_sfile * sf, void * data __attribute__((unused > { > size_t i; > > - for (i = 0; i < op_nr_counters; ++i) > + for (i = 0; i < op_nr_events; ++i) > odb_sync(&sf->files[i]); > > // TODO: handle extended > diff --git a/libperf_events/operf_sfile.h b/libperf_events/operf_sfile.h > index 9b91180..f639212 100644 > --- a/libperf_events/operf_sfile.h > +++ b/libperf_events/operf_sfile.h > @@ -29,6 +29,7 @@ struct operf_kernel_image; > #define INVALID_IMAGE "INVALID IMAGE" > > #define VMA_SHIFT 13 > + > /** > * Each set of sample files (where a set is over the physical counter > * types) will have one of these for it. We match against the > @@ -61,7 +62,7 @@ struct operf_sfile { > /** true if this file should be ignored in profiles */ > int ignored; > /** opened sample files */ > - odb_t files[OP_MAX_COUNTERS]; > + odb_t files[OP_MAX_EVENTS]; > /** extended sample files */ > odb_t * ext_files; > /** hash table of opened cg sample files */ > diff --git a/libperf_events/operf_utils.h b/libperf_events/operf_utils.h > index 2a3cf59..2f3fc95 100644 > --- a/libperf_events/operf_utils.h > +++ b/libperf_events/operf_utils.h > @@ -45,13 +45,15 @@ extern bool throttled; > #define MMAP_WINDOW_SZ (32 * 1024 * 1024ULL) > #endif > > +#define OP_MAX_EVENTS 24 > + > /* A macro to be used for ppc64 architecture-specific code. The '__powerpc__' macro > * is defined for both ppc64 and ppc32 architectures, so we must further qualify by > * including the 'HAVE_LIBPFM' macro, since that macro will be defined only for ppc64. > */ > #define PPC64_ARCH (HAVE_LIBPFM) && ((defined(__powerpc__) || defined(__powerpc64__))) > > -extern unsigned int op_nr_counters; > +extern unsigned int op_nr_events; > > static inline size_t align_64bit(u64 x) > { > diff --git a/pe_profiling/operf.cpp b/pe_profiling/operf.cpp > index e948009..ffd2fbf 100644 > --- a/pe_profiling/operf.cpp > +++ b/pe_profiling/operf.cpp > @@ -67,7 +67,7 @@ operf_read operfRead; > op_cpu cpu_type; > double cpu_speed; > char op_samples_current_dir[PATH_MAX]; > -uint op_nr_counters; > +uint op_nr_events; > verbose vmisc("misc"); > uid_t my_uid; > bool no_vmlinux; > @@ -1291,6 +1291,11 @@ out: > static void _process_events_list(void) > { > string cmd = OP_BINDIR; > + if (operf_options::evts.size() > OP_MAX_EVENTS) { > + cerr << "Number of events specified is greater than allowed maximum of " > + << OP_MAX_EVENTS << "." << endl; > + exit(EXIT_FAILURE); > + } > cmd += "/ophelp --check-events "; > for (unsigned int i = 0; i < operf_options::evts.size(); i++) { > FILE * fp; > @@ -1761,7 +1766,7 @@ static void process_args(int argc, char * const argv[]) > } else { > _process_events_list(); > } > - op_nr_counters = events.size(); > + op_nr_events = events.size(); > > if (operf_options::vmlinux.empty()) { > no_vmlinux = true; |