RFR(XS) 8191852: Null pointer dereference in ciKlass::get_Klass of ciKlass.hpp:58

[Vladimir Kozlov]

Good.

One tiny thing. Can you use 'Klass* ' instead of 'Klass *'?:

Klass *field_holder

No need for new review.

Thanks,
Vladimir

On 12/21/17 5:59 PM, dean.long at oracle.com wrote:
> New webrev:
> 
> http://cr.openjdk.java.net/~dlong/8191852/webrev.2
> 
> My experiment to simplify get_instance_klass() and friends failed. 
> Parfait failed to identify possible null dereferences, even new ones 
> that I introduced as a test, so went with the minimal fix instead. It 
> only addresses the following error:
> 
> Error: Null pointer dereference
>     Null pointer dereference [null-pointer-deref] (CWE 476):
>        Read from null pointer ((ciMetadata*)this)
>          at line 58 of src/hotspot/share/ci/ciKlass.hpp in function 
> 'ciKlass::get_Klass'.
>            Null pointer introduced at line 207 of 
> src/hotspot/share/ci/ciEnv.hpp in function 'ciEnv::get_instance_klass'.
>            Constant 'NULL' passed into function ciKlass::get_Klass, 
> argument this, from call at line 240 of src/hotspot/share/ci/ciField.cpp 
> in function 'ciField::initialize_from'.
>            Function ciEnv::get_instance_klass may return constant 'NULL' 
> at line 207, called at line 237.
> 
> dl
> 
> 
> On 12/13/17 4:20 PM, Vladimir Kozlov wrote:
>> On 12/13/17 3:09 PM, dean.long at oracle.com wrote:
>>> On 12/13/17 1:08 PM, Vladimir Kozlov wrote:
>>>
>>>> On 12/13/17 12:45 PM, dean.long at oracle.com wrote:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8191852
>>>>> http://cr.openjdk.java.net/~dlong/8191852/webrev/
>>>>>
>>>>> Our static analysis tool was complaining about a possible null 
>>>>> pointer dereference in ciKlass::get_Klass(), because of this code:
>>>>>
>>>>> 237.      _holder = 
>>>>> CURRENT_ENV->get_instance_klass(fd->field_holder());
>>>>> [...]
>>>>> 240.      Klass* k = _holder->get_Klass();
>>>>>
>>>>> so I added NULL checks in get_instance_klass and a few other 
>>>>> similar functions.
>>>>
>>>> No, you don't ;)
>>>
>>> Yes, you're right.  Sorry about that :-)
>>>
>>>> You replaced NULL checks which return NULL with asserts. It is not 
>>>> the same. Are you sure that in all those cases we will not get NULL?
>>>
>>> It didn't show up in my testing.  But what I could try is to remove 
>>> the asserts and rerun the static analysis.  Then get_instance_klass() 
>>> would be:
>>>
>>>    ciInstanceKlass* get_instance_klass(Klass* o) {
>>>      return get_metadata(o)->as_instance_klass();
>>>    }
>>>
>>> In the first example above, static analysis would need to verify that 
>>> fd->field_holder() can never return NULL.  If that sounds reasonable, 
>>> I'll rerun the static analysis.
>>
>> Yes, it is reasonable.
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> dl
>>>
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>>
>>>>> dl
>>>
>