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

[Jorn Vernee]

Henry Jen schreef op 2018-10-30 18:46:
> 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.

Yes, that's a good point. That will allow for a 1 byte memory region at 
0xFFFFFFFF.

I've also added a test for that exact case.

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

Yes! And I think when min is positive the length maximum is always 
Long.MAX_VALUE.

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

That won't work for the case where min and length are both positive, but 
min + length is also positive. In that case the check should also pass.

The way I have it right now, since length is always positive, min + 
length can only overflow to positive if min is negative, so that's also 
the check :) But I've changed it to check for min + length > 0 to allow 
for a 1 byte region if min == -1L.

> What do you think?

Good suggestions, here is the updated webrev: 
http://cr.openjdk.java.net/~jvernee/panama/webrevs/8212987/webrev.04/index.html

I've also removed the spurious import in PointerTest, which 
auto-complete put in there when auto-completing to @Transient instead of 
@Test :)

> Cheers,
> Henry
> 

Thanks for the reviews,
Jorn

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