On Jun 12, 2012, at 7:41 PM, David Holmes wrote: > On 13/06/2012 3: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. > > I like this better, but I don't see any check in table_get_info(), other than > the assertion.
It will naturally just return the info field of the table element, which can be NULL. But most importantly, with tables that have info_size==0, this table_get_info() function is never called. For Parfait, it just won't see that a function does a literal "return NULL", it never knows that the info field in the table elements can be NULL. -kto > > David > ----- > >> >> New webrev was generated and I displaced the old one: >> http://cr.openjdk.java.net/~ohair/openjdk8/parfait_hprof_fixes/webrev/ >> >> 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 >>
