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

Jorn Vernee jbvernee at xs4all.nl
Thu Nov 1 12:13:17 UTC 2018 

I could check the sign with something like this:

     final long SIGN_BIT = Long.MIN_VALUE;
     if((min & SIGN_BIT) == ((min + length) & SIGN_BIT)) {

(`signum` actually does something else than just extracting the sign bit 
[1, 2])

But the way I do it right now there's also a short-circuit if min is not 
less than 0:

     if(min < 0 && min + length > 0) {

If the problem is clarity, I can factor this into a helper method and 
end up with something like this:

     if(canLengthCauseOverflow(min, length)) {

We want to disallow negative addresses overflowing to positive ones, but 
allow positive addresses overflowing to negative ones, basically 
treating the address as an unsigned integer. In unsigned land there is 
only one kind of overflow, from FFFFFFFF (all 1s) to 0, which translates 
into negative-to-positive overflow in signed land (assuming 2's 
complement numbers), so we want to only disallow that one. Actually, 
AFAICT the previous implementation already allowed to-negative (i.e. 
signed) overflow since you could construct a memory region with min = 
Long.MAX_VALUE and length = Long.MAX_VALUE.

Cheers,
Jorn

[1] : https://en.wikipedia.org/wiki/Sign_function
[2] : 
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/Long.html#signum(long)

Maurizio Cimadamore schreef op 2018-11-01 12:07:
> After taking a fresh look at this - would it suffice to check that
> 
> signum(min + length) = signum(min)
> 
> I think that, since signum(0) == signum(x), where x > 0, that should
> cover for the FFFFFFFF case with length 1. And it also should prevent
> positive addresses turning into negative ones and viceversa?
> 
> Maurizio
> 
> On 31/10/2018 11:02, Jorn Vernee wrote:
>> Oh. I ran the tests, but I'm getting a pass there since Windows does 
>> not use the direct invoker, which means a different factory method on 
>> BoundedPointer is getting called, which doesn't offset into 
>> EVERYTHING. I'm afraid that's not something I can test right now by 
>> myself.
>> 
>> I tried to fix it by removing EVERYTHING in favor of calling 
>> BoundedMemoryRegion.of(offset). But when testing that I'm seeing a 
>> bunch of crashes in `V ~BufferBlob::invoke_native_blob` [1]. I think 
>> that is the generated downcall stub that's crashing right? Not sure 
>> what that's about, but I guess maybe some objects are now getting GC'd 
>> too early since the memory region is no longer saved in a static 
>> field, or maybe I still have a bug in that stub generation code :/ (I 
>> took another look, but everything looks OK to me).
>> 
>> So instead I've made EVERYTHING an anonymous subclass which overrides 
>> checkBounds to be empty, since technically any offset is in bounds.
>> 
>> I also realized the calculation still needed a special case for 
>> Long.MIN_VALUE since that can't be negated.
>> 
>> Can you test the new webrev? 
>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8212987/webrev.05/
>> 
>> Thanks,
>> Jorn
>> 
>> [1] : 
>> http://hg.openjdk.java.net/panama/dev/file/f1afd0ae2932/src/hotspot/share/prims/universalNativeInvoker.cpp#l44
>> 
>> Henry Jen schreef op 2018-10-31 01:05:
>>> That looks good, but we still have a little issue to iron out.
>>> 
>>> New test failed because of BoundedPointer is offsetting EVERYTHING
>>> region, where length set to Long.MAX_VALUE.
>>> 
>>> Cheers,
>>> Henry
>>> 
>>>> test PointerTest.test1ByteRegionAtMaxHeapOffset(): failure
>>>> java.lang.IndexOutOfBoundsException: offset=0xffffffffffffffff 
>>>> length=0x7fffffffffffffff
>>>>     at 
>>>> java.base/jdk.internal.foreign.memory.BoundedMemoryRegion.checkBounds(BoundedMemoryRegion.java:153)
>>>>     at 
>>>> java.base/jdk.internal.foreign.memory.BoundedMemoryRegion.checkRange(BoundedMemoryRegion.java:158)
>>>>     at 
>>>> java.base/jdk.internal.foreign.memory.BoundedPointer.<init>(BoundedPointer.java:77)
>>>>     at 
>>>> java.base/jdk.internal.foreign.memory.BoundedPointer.<init>(BoundedPointer.java:68)
>>>>     at 
>>>> java.base/jdk.internal.foreign.memory.BoundedPointer.createNativeVoidPointer(BoundedPointer.java:175)
>>>>     at 
>>>> java.base/jdk.internal.foreign.invokers.DirectSignatureShuffler.longToPointer(DirectSignatureShuffler.java:335)
>>>>     at 
>>>> PointerTest$pointers$Impl/0x00000007c035b440.get_1_byte_pointer(Unknown 
>>>> Source)
>>>>     at 
>>>> PointerTest.test1ByteRegionAtMaxHeapOffset(PointerTest.java:297)
>>>>     at 
>>>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native 
>>>> Metho
>>> 
>>>> On Oct 30, 2018, at 11:52 AM, Jorn Vernee <jbvernee at xs4all.nl> 
>>>> wrote:
>>>> 
>>>> 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

More information about the panama-dev mailing list