Need reviewer: a couple of hprof fixes for Parfait

[Kelly O'Hair]

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.holmes at 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
>>