Need reviewer: a couple of hprof fixes for Parfait

[Kelly O'Hair]

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