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

[Maurizio Cimadamore]

On 21/01/2019 15:27, Jorn Vernee wrote:
> 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.

That seems a non-issue - e.g. internally we could have two addr() 
functions, a checked one (exposed by the API) and an unchecked one, 
which can be used as an impl aid for Pointer::equals, no?

Maurizio

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