[foreign] RFR 8212987 : Binder should allows negative values for native pointers

[Jorn Vernee]

Updated webrev: 
http://cr.openjdk.java.net/~jvernee/panama/webrevs/8212987/webrev.03/
(beware that the BoundedMemoryRegion diff turned out a bit confusing)

- Changed BoundeMemoryRegion to use factory methods.
- Factories that don't take a length calculate a max length that can not 
overflow (in the unsigned sense).
- Moved BoundedMemoryRegion anonymous class from Pointer to 
BoundedMemoryRegion, since constructor is now private.
- Added one more test to check for the error in the case where the 
automatic length calculated for a native pointer is shorter than the 
length required by the carrier type.
- Switched PointerTest to TestNG, since that makes it easier to test for 
exceptions.

I've added a check for the overflow to BoundedMemoryRegion, but when 
trying to test for it it turned out that BoundedPointer was already 
catching that case. I've left the check there as it will still be useful 
to catch programming errors when an explicit length is being passed.

Trying to treat a signed long as unsigned is somewhat confusing, maybe 
we should add an UnsignedLong utility class that abstracts this away in 
a later RFR, especially if we start allowing 'unsigned' longs in more 
places.

Jorn

Jorn Vernee schreef op 2018-10-30 11:26:
> You're right about `toUnsignedString`. For some reason I thought
> `toHexString` had the `-` as a prefix if the argument is negative.
> 
> About the length constraint; My initial thought was just to trust the
> pointer returned from native, but I guess we should do better. In the
> specific case I'm testing, void *, the length that is being used is
> Long.MAX_VALUE, basically the length is unknown, so I don't think I
> can do a range check on min + length in the constructor in that case.
> 
> As far as I can see, the `min` value is only used in a few min +
> offset calculations before passing to Unsafe put/get routines, I think
> we'd want to allow overflow to negative there, but not overflow to
> positive, basically treating the long as if it was unsigned.
> 
> I think I will switch BoundedMemoryRegion to having factory methods
> that do the validation, and then have ones where the length is known,
> which do a range check, and ones where the length is unknown, that cap
> the it to where it can not overflow. Then I can use the latter for
> void *. If a bad pointer is passed from native, the known-length case
> should fail on construction of the memory region, and the
> unknown-length case should fail when dereferencing (since there is
> already a bounds check being done).
> 
> What do you think?
> 
> Thanks,
> Jorn
> 
> Henry Jen schreef op 2018-10-30 01:59:
>> BTW, the change in toString is not needed, as toUnsignedString(l, 16)
>> is same as toHexString(l)?
>> 
>> Cheers,
>> Henry
>> 
>> 
>>> On Oct 29, 2018, at 5:56 PM, Henry Jen <henry.jen at oracle.com> wrote:
>>> 
>>> I am afraid this is not that simple, we need to at least protect 
>>> length from overflow the address to >= 0.
>>> 
>>> I haven’t completely check the implementation, we need to make sure 
>>> all other operation will not expand or shift the region, which I 
>>> believe is the constraint of the design. If that’s indeed the case, 
>>> guard at construction is probably good enough.
>>> 
>>> Cheers,
>>> Henry
>>> 
>>>> On Oct 29, 2018, at 10:04 AM, Jorn Vernee <jbvernee at xs4all.nl> 
>>>> wrote:
>>>> 
>>>> Hello,
>>>> 
>>>> Please review this patch which tweaks the binder to allow negative 
>>>> values for native pointers.
>>>> 
>>>> The problem was previously discussed here: 
>>>> http://mail.openjdk.java.net/pipermail/panama-dev/2018-October/002994.html
>>>> 
>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8212987
>>>> Webrev : http://cr.openjdk.java.net/~jvernee/panama/webrevs/8212987/
>>>> 
>>>> As a reminder, I'm not a committer, so someone else will have to 
>>>> push this.
>>>> 
>>>> Thanks,
>>>> Jorn
>>>