Need reviewer: a couple of hprof fixes for Parfait

[David Holmes]

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.

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
>