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

[Jorn Vernee]

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

Yeah I guess that works as well. hashCode() would also need to be 
updated in that case. But, I feel that it weakens the semantics for 
Pointer a bit; the address can be used for equals and hashCode while a 
pointer is OOB, but we can't look at it's address (even if we don't want 
to use the address). Doing the check at the point where we are actually 
unboxing the pointer seems like the more natural choice to me.

Jorn

> 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