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

Henry Jen henry.jen at oracle.com
Thu Nov 1 16:06:49 UTC 2018 

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