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

[Jorn Vernee]

Maurizio Cimadamore schreef op 2019-01-21 16:15:
> 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.

Ah OK, I guess the direct invoker just side-steps LayoutTypes altogether 
(I have not spent that much time with that code).

> 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) ?

We need to be able to retrieve the address as part of 
BoundedPointer::equals, which is needed to make the iterate code work, 
so doing the check in `addr()` would still be too early.

I'll have a look at the direct invoker.

Jorn

> 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