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

Jorn Vernee jbvernee at xs4all.nl
Thu Nov 1 12:53:28 UTC 2018 

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