[foreign] RFR 8224835: Pointer validity checks are applied inconsistently

[Maurizio Cimadamore]

Looks good

Maurizio

On 29/05/2019 16:01, Jorn Vernee wrote:
> Little update: 
> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8224835/webrev.03/
>
> Where I've also added 2 test cases to OutOfBoundsTest for the bounds 
> checking for arrays and structs. And some cases to PointerTest which 
> check that addr() is performing the needed checks.
>
> Cheers,
> Jorn
>
> Jorn Vernee schreef op 2019-05-29 15:51:
>> I feel your sore point. tbh, I think if we go for checks for structs
>> and arrays we should do a bounds check as well. This will make sure
>> that half of the struct/array is not out of bounds. e.g. in the array
>> case, I think a user could do an unsafe cast to make an array pointer
>> larger, and not get an exception when calling get(lastElement) if the
>> element type is a struct or array.
>>
>> One of the niceties of not doing the check for structs/arrays is that
>> this avoids redundant bounds checking when first calling get on a
>> struct/array pointer, and then again when accessing a field/element.
>> But, I think maybe this could also be solved later on with a JIT
>> intrinsic that merges the two checks (though I've heard that for those
>> kinds of larger -> smaller bounds checks this is pretty tricky to do
>> in general).
>>
>> I've added the checks back in:
>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8224835/webrev.01/
>>
>> I've implemented this by calling checkAccess(Pointer.AccessMode.NONE),
>> which effectively does no read/write access check, since we don't know
>> at that point if the pointer will be written to, or read from yet.
>>
>> Changed files are References (adding the checks), Pointer (adding the
>> NONE access mode) and PointerScopeTest (sharpening the tests again).
>>
>> Cheers,
>> Jorn
>>
>> Maurizio Cimadamore schreef op 2019-05-29 14:54:
>>> Looks very good - the changes to Pointer:addr alone (and associated
>>> cleanups) are worth :-)
>>>
>>> I understand the choice with respect to liveness checks for structs
>>> and arrays - this reflects our big type vs. small type distinction.
>>> The sore point to note there is that you'd only get notified of a
>>> failure if the element you are accessing is, again, not a struct or an
>>> array. What if we added a call to checkAlive in the struct and array
>>> constructors?
>>>
>>> Maurizio
>>>
>>> On 29/05/2019 13:26, Jorn Vernee wrote:
>>>> Hi,
>>>>
>>>> Please review the following:
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8224835
>>>> Webrev: 
>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8224835/webrev.00/
>>>>
>>>> This replaces the various public checkXXX methods on BoundedPointer 
>>>> with a single public checkAccess(AccessMode) method, which is then 
>>>> used everywhere outside of BoundedPointer. This makes sure that 
>>>> every place does all the needed checks, and not only one or the other.
>>>>
>>>> I've also checked all the use sites, and noticed that the 
>>>> IllegalAccessException that Pointer::addr declared as being thrown 
>>>> was never actually thrown. Instead an AccessControlException was 
>>>> being thrown. So I've updated the relevant signatures as well, and 
>>>> this allowed for the removal of a bunch of try/catch blocks.
>>>>
>>>> I've also removed the checks in References.OfStruct::get and 
>>>> References.OfArray::get, since we're not actually dereferencing the 
>>>> pointer at that point, only wrapping it. The checks will still 
>>>> happen later on when accessing a field or element (i.e. when 
>>>> actually dereferencing the pointer).
>>>>
>>>> Thanks,
>>>> Jorn