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

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Thu Nov 1 11:07:16 UTC 2018 

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