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

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Thu Nov 1 13:02:15 UTC 2018 

Well, it just makes it a bit harder, but still doable? What we are 
saying is that it's ok to go from PLUS to MINUS but not viceversa.

That said, I guess there are tricky case where you can go from PLUS to 
PLUS via overflow and this mechanism would not detect it (e.g. if length 
is Long.MAX_VALUE).

Maurizio

On 01/11/2018 12:53, Jorn Vernee wrote:
> Actually, I realized that the sign bit check doesn't work since it 
> prohibits the case where min is positive but min + length is negative, 
> which should be allowed since that is just signed overflow, which we 
> don't want to block.
>
> Jorn
>
> Jorn Vernee schreef op 2018-11-01 13:13:
>> 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