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

[dean.long at oracle.com]

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