Re: Questionable array size check in operf_counter.cpp

On 12/18/2014 11:20 AM, William Cohen wrote:
> I ran the oprofile currently in the git resository through coverity to see if there were any issues in the code.  One thing that did look questionable was:
> 
> 
> 	Error: BAD_SIZEOF (CWE-467): [#def3]
> oprofile-1.1.0git/libperf_events/operf_counter.cpp:772: bad_sizeof: The expression "sizeof (this->poll_data) / sizeof (this->poll_data[0])" is suspicious. Note that "this->poll_data" is a pointer and therefore the division will not return the number of array elements which may have been the intent.
> 
> The method making a larger size array and copying the data over. It is attempting to get the size of the array with:
> 
> 	num_mmaps = sizeof(poll_data)/sizeof(poll_data[0]);
Yup, dumb coding error, resulting in a fairly serious negative effect downstream in not copying all of the old poll_data, as you pointed out.  Fortunately, this isn't a real common scenario . . . that is:
      - If operf is started on a multi-threaded app (i.e., one that uses
        pthreads) with the "--pid" option
      - If said app has already started at least one thread
      - If the app either creates new threads or forks new processes
        after operf has started

> 
> It appears this was added in commit eb75586eed1d105a22b12d879a31ec32d6a94913
> 
> It looks like the class should have the information about the size of poll_data in there. Should the operf_record class include a num_mmaps field so the size of poll_data is known? Or could the some other field (such as poll_count) be used to track that information?
Agreed -- operf_record should have a 'num_mmaps' member.

-Maynard
> 
> -Will
> 


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk