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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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

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

dl
Reply | Threaded
Open this post in threaded view
|

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

Vladimir Kozlov
On 12/13/17 12:45 PM, [hidden email] 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 ;)
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?

Thanks,
Vladimir

>
> dl
Reply | Threaded
Open this post in threaded view
|

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

dean.long
On 12/13/17 1:08 PM, Vladimir Kozlov wrote:

> On 12/13/17 12:45 PM, [hidden email] 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.

dl

>
> Thanks,
> Vladimir
>
>>
>> dl

Reply | Threaded
Open this post in threaded view
|

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

Vladimir Kozlov
On 12/13/17 3:09 PM, [hidden email] wrote:

> On 12/13/17 1:08 PM, Vladimir Kozlov wrote:
>
>> On 12/13/17 12:45 PM, [hidden email] 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
>
Reply | Threaded
Open this post in threaded view
|

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

dean.long
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, [hidden email] wrote:
>> On 12/13/17 1:08 PM, Vladimir Kozlov wrote:
>>
>>> On 12/13/17 12:45 PM, [hidden email] 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
>>

Reply | Threaded
Open this post in threaded view
|

Re: 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, [hidden email] 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, [hidden email] wrote:
>>> On 12/13/17 1:08 PM, Vladimir Kozlov wrote:
>>>
>>>> On 12/13/17 12:45 PM, [hidden email] 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
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

dean.long
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, [hidden email] 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, [hidden email] wrote:
>>>> On 12/13/17 1:08 PM, Vladimir Kozlov wrote:
>>>>
>>>>> On 12/13/17 12:45 PM, [hidden email] 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
>>>>
>>