On Jun 12, 2012, at 12:55 PM, Daniel D. Daugherty wrote: > On 6/12/12 11:23 AM, Kelly O'Hair wrote: >> Ok, I have to confess, I had to look at this hprof table logic more. >> >> This generic "table" is used all over the place in hprof, so I had to dust >> off some brain cells. >> The info structures are extra structures or extra "info" for each table >> element, and it's perfectly >> valid for the info structure size to be 0. In particular, string tables have >> info_size==0. >> But when dealing with those info_size==0 tables, the info data is never >> asked for or accessed, >> if it was hprof would be seriously broken with null pointer accesses all >> over the place. >> So I am not convinced we are fixing any kind of bug here, just shutting up a >> static analysis >> tool that can't follow the non-obvious code flows. >> >> Parfait is seeing a NULL pointer returned here, and assumes that all calls >> to get_info could >> return a NULL pointer, that is in fact wrong, because if info_size==0, this >> call will never be made. >> Parfait doesn't know that. Not unexpected. And even without this check, if >> info_size==0, it >> will return a NULL pointer anyway. >> >> So I have changed my fix. >> >> The static function get_info() in hprof_table.c is only called twice in that >> file, once by the >> extern function table_get_info(), and once by a table walker. >> I have removed the "return NULL;" from get_info() and made the checks where >> it is called instead. >> I think this will remove the Parfait errors, but may generate some new ones, >> so we could be playing >> a bit of a whack-a-mole game here. >> >> New webrev was generated and I displaced the old one: >> http://cr.openjdk.java.net/~ohair/openjdk8/parfait_hprof_fixes/webrev/ > > Thumbs up with this version of the fix also, but I was also OK > with the last version so that may not help ease your mind... :-) > > I see the new guard logic in the table walker. I'm presuming > that you made sure that functions called by the table walker > are OK with a NULL info pointer.
Inside the table walker, get_info() was returning a NULL before, so the walker is behaving the same way it did before. > > I don't see any guard logic in table_get_info(), but this assert: > > 914 HPROF_ASSERT(ltable->info_size > 0); > > covers the (non-product bits) case. Calling table_get_info() kind of assumes you are asking for an info structure, and that info structures exist. Again, no change from the way it always was. The only real change is get_info(), which no longer explicitly returns NULL. There is really nothing wrong with this hprof_table.c code, it's just a matter of getting tools like Fortify or Parfait to agree that there is nothing wrong. :^( I'm not so sure how I feel about twisting valid logic around to convince a static analysis tool that it is valid logic :^( But I guess it's just like making changes to code to get rid of benign compiler warning messages. -kto > > Dan > >> Generic data structures where a pointer can be NULL sometimes and not-NULL >> other times >> is not something Parfait (or perhaps any static analysis tool) can deal with >> very well. >> >> -kto >> >> On Jun 12, 2012, at 8:06 AM, Kelly Ohair wrote: >> >>> if size is zero it returns null >>> thats the cascading error that parfait is finding >>> asserts turn into nothing with product builds so i need a dead stop here to >>> shut up parfait :( >>> >>> >>> Sent from my iPhone >>> >>> On Jun 11, 2012, at 22:56, David Holmes<david.hol...@oracle.com> wrote: >>> >>>> On 12/06/2012 12:48 PM, Kelly O'Hair wrote: >>>>> Need reviewer. >>>>> >>>>> I was asked to look at some Parfait errors in hprof code: >>>>> >>>>> 7176138: Fixes for missing close() calls and possible null pointer >>>>> reference instead of fatal error >>>>> http://cr.openjdk.java.net/~ohair/openjdk8/parfait_hprof_fixes/webrev/ >>>> >>>> hprof_table.c >>>> >>>> 211 if ( ltable->info_size == 0 ) { >>>> 212 HPROF_ERROR(JNI_TRUE, "Table is empty and should never be."); >>>> 213 return NULL; >>>> >>>> It is not obvious this should be a fatal error. Elsewhere this is handled >>>> as an assert. >>>> >>>> David
