Re: [PATCH] Fix seg fault due to incorrect array size initialization

[Maynard J. <may...@us...>]

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;