[foreign] RFR 8217414: Remove bounds check in BoundedPointer constructor

[Maurizio Cimadamore]

On 21/01/2019 14:29, Jorn Vernee wrote:
> Updated webrev: 
> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8217414/webrev.01
>
> I ended up not adding the check for Callbacks since AFAICT there is 
> currently no public API to create a Callback with an OOB pointer. 
> Should I add the check any ways?

That's fine, but I'm pretty sure that the added test will fail with the 
direct invoker.

You might want to tweak this method in DirectSignatureShuffler:

private static long pointerToLong(Pointer<?> value) throws 
IllegalAccessException {
         return value.addr();
     }

Another option, would be to always throw when somebody calls addr() and 
the pointer is out of bounds - this could actually be a better solution 
(which would also subsume the changes in References.OfPointer) ?

Maurizio

>
> Jorn
>
> Jorn Vernee schreef op 2019-01-21 14:54:
>> Ok, I see your point. Plus, if a user finds the API too restrictive
>> they could always fall back on native allocation APIs like malloc &
>> free, or possibly custom LayoutTypes when those are introduced.
>>
>> I think it will be sufficient to move the check to
>> `References.OfPointer` and `References.OfCallback`. But I will add
>> some tests to make sure as well.
>>
>> Jorn
>>
>> Maurizio Cimadamore schreef op 2019-01-21 14:42:
>>> On 21/01/2019 13:12, Jorn Vernee wrote:
>>>> There is a `checkAlive()` that gets called when a pointer is 
>>>> unboxed [1], I thought you were talking about that check. But, I 
>>>> don't think we should do a bounds check before passing a pointer to 
>>>> native code. The "one past the end of a container" pointer idiom is 
>>>> common and some APIs might expect to be passed such a pointer. It 
>>>> seems too restrictive to require that pointers passed to native 
>>>> code have to be in bounds (tbh I have my doubts about the 
>>>> liveliness check as well).
>>>
>>> The whole point of the Panama API is to provide enriched safety. Now,
>>> if users want to opt out of that, we might (in the future) provide
>>> knobs to do so, but I see no point for passing a pointer that is not
>>> alive around (given it is known to contain garbage). Similarly, I see
>>> very little point in passing a pointer that is out of bounds - granted
>>> there might be few use cases for that (e.g. pass a 'limit' pointer to
>>> a library), but I'd prefer to stick with stricter semantics and loosen
>>> that up on a use case by use case basis.
>>>
>>> I think the main point of this patch should be to _delay_ the check
>>> that is currently done on the constructor, not to remove it
>>> completely. It is ok if some piece of code temporarily produces an out
>>> of bound pointers (e.g. when iterating), but I think we should limit
>>> the exposure of that weird pointer to the rest of the machinery.
>>>
>>> Maurizio
>>>
>>>>
>>>> Jorn
>>>>
>>>> [1] : 
>>>> http://hg.openjdk.java.net/panama/dev/file/tip/src/java.base/share/classes/jdk/internal/foreign/memory/BoundedPointer.java#l75
>>>>
>>>> Maurizio Cimadamore schreef op 2019-01-21 13:56:
>>>>> I agree that the check should removed from the constructor - but we
>>>>> should make sure that checks happen when the pointer is used.
>>>>> Dereference is one situation, function call is another. I guess
>>>>> dereference is already covered (after all, a memory read will fail if
>>>>> outside the boundaries), but I don't think passing an out of bound
>>>>> pointer to a function is currently being checked, and it should if we
>>>>> remove the constructor eager check
>>>>>
>>>>> Maurizio
>>>>>
>>>>> On 21/01/2019 12:00, Jorn Vernee wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I have a patch addressing 2 vacuously passing tests 
>>>>>> (api/PointerTest and api/ArrayTest). The patch fixes the tests, 
>>>>>> and to make that happen removes the bounds check in 
>>>>>> BoundedPointer's constructor. As discussed offline; this check is 
>>>>>> not needed since we already de a check when dereferencing a 
>>>>>> pointer in BoundedMemoryRegion, and the check in the constructor 
>>>>>> is overly restrictive since it should be fine to create 
>>>>>> out-of-bounds pointers as long as it is not dereferenced. This 
>>>>>> does mean the tests needed to be tweaked since now an access 
>>>>>> exception is throw at a later point.
>>>>>>
>>>>>> Please review the following.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8217414
>>>>>> Webrev: 
>>>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8217414/webrev.00
>>>>>>
>>>>>> Thanks,
>>>>>> Jorn