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

[dean.long at oracle.com]

On 12/22/17 12:22 PM, Vladimir Kozlov wrote:

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

Sure.

> No need for new review.
>

Thanks Vladimir.

dl

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