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

[David Holmes]

On 18/01/2014 3:24 AM, Coleen Phillimore wrote:
>
> I see no problem with this change.  The spec says that we don't check
> for programming errors, but it doesn't say we must not check for
> programming errors.   We can decide case by case whether we want to add
> helpful checks that don't degrade performance, and this is one such
> check that would be useful.   I have reviewed it and will sponsor it for
> you.   It needs another reviewer.

<sigh> The problem is that most such checks look okay in isolation, but 
in combination we add a bunch of error checking that simply penalizes 
correct code. What makes this example special such that it deserves to 
be "fixed"? You'll have to figure that out to use as your yard-stick for 
the next such request.

David

> Thanks,
> Coleen
>
>
> On 1/10/2014 2:59 AM, David Holmes 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
>