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

Jorn Vernee jbvernee at xs4all.nl
Thu Nov 1 16:18:56 UTC 2018 

If we just use `length = 0 - min` then we get a negative length if `min` 
is positive. Or do you want to treat `length` as unsigned as well?

Jorn

Henry Jen schreef op 2018-11-01 17:06:
> max length 0 - min won’t work for min == 0, that’s everything. We need
> to use test again max address to be 0-min-1, then comparison with
> length and max address should be < 0. You got the idea.
> 
> Cheers,
> Henry
> 
>> On Nov 1, 2018, at 8:21 AM, Henry Jen <henry.jen at oracle.com> wrote:
>> 
>> I was hesitate to reply as the fix is kind of hackish, but not ready 
>> to suggest we go all the way to enable full range of 64-bit, after 
>> all, Long.MAX_VALUE is giving us 8EB and that’s seems way more than 
>> enough. But to completely address rather than simply make it work for 
>> special cases, we probably should just treat address as unsigned and 
>> use Long.compareUnsigned to do comparison.
>> 
>> Therefore, max length for a memory always 0 - min. The length should 
>> be Long.compareUnsigned(length, maxLength) <= 0, with all other offset 
>> operation compare against length established at construction time with 
>> Long.compareUnsigned, that should be enough to deal with this.
>> 
>> However, taking offset with large(negative) number seems really 
>> confusing.
>> 
>> What do you think?
>> 
>> Cheers,
>> Henry
>> 
>>> On Nov 1, 2018, at 6:02 AM, Maurizio Cimadamore 
>>> <maurizio.cimadamore at oracle.com> wrote:
>>> 
>>> 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