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

[Henry Jen]

I wonder if it can be simplified a little bit, let’s say address is indeed 0xFFFFFFFF, we should be able to have a memory region of length 1.

When the min(real address) is negative, the cap of length for unknown is pretty much just to negate the value of min.

min + length should be <= 0. We can only access min + offset - 1, so that we won’t access 0x0 in negative case.

What do you think?

Cheers,
Henry



> On Oct 30, 2018, at 9:49 AM, Jorn Vernee <jbvernee at xs4all.nl> wrote:
> 
> 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