RFR(XS): 8031427: AllocObject and Unsafe.allocateInstance segfault for primitive types

[Coleen Phillimore]

On 01/21/2014 07:29 AM, Gilles Duboscq wrote:
> Hi all,
>
> David, in general, I understand your concerns regarding the cost of a
> check that will only be useful for incorrect usages.
> For this precise case, I think the exception can be useful for
> developers using JNI or Unsafe while the cost of the check is probably
> quite small compared to the other checks already in place.
>
> Also, the code produced by C2's intrinsic for Unsafe.allocateInstance
> contains the same additional check.

I agree,  for this case, it is consistent and add safety without 
additional cost.  Also, the crash is an artifact of how we handle 
primitive types so probably would take time to debug and discover why 
this crashed.   In general, JNI and Unsafe do not check their arguments 
and there will need to be sufficient motivation to add checking.   I 
will sponsor this change for you.

Thanks,
Coleen

> -Gilles
>
> On Fri, Jan 10, 2014 at 8:59 AM, David Holmes <david.holmes at oracle.com> wrote:
>> Hi Gilles,
>>
>>
>> On 9/01/2014 10:58 PM, Gilles Duboscq wrote:
>>> Hello,
>>>
>>> As i wrote in a bug report [1]:
>>> Currently, calling AllocObject or Unsafe.allocateInstance with a
>>> primitive type (e.g. unsafe.allocateInstance(int.class)) leads to a
>>> segmentation fault because Classes representing primitive types do not
>>> have an associated Klass.
>>>
>>> While AllocObject or allocateInstance should in principle never be
>>> called with a Class representing a primitive, it should probably not
>>> crash in such a case since it throws sensible exceptions for many other
>>> invalid arguments (interfaces, abstract types, arrays, j.l.Class).
>>
>> As per the bug report both Unsafe and JNI require the caller to pass the
>> correct parameters to a method and if they do not then crashes are entirely
>> possible. The intent is to avoid penalizing correct code with unnecessary
>> checks.
>>
>> The existing checks (check_valid_for_instantiation) arise simply from the
>> fact that the underlying allocation routine is used by other parts of the VM
>> that do require more extensive checks.
>>
>>
>>> The proposed fix [2] just throws an InstantiationException if the Klass
>>> is NULL.
>>
>> While this may seem quite innocuous I am concerned about setting a precedent
>> for "fixing" unsafe/JNI when used incorrectly.
>>
>> David
>>
>>
>>> -Gilles
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8031427
>>> [2] http://lafo.ssw.uni-linz.ac.at/alloc_object.diff