Support signed offsets

Both Structs and Lists support two's complement "Signed" offsets: > B (30 bits) = Offset, in words, from the end > of the pointer to the start of the struct's > data section. Signed. > B (30 bits) = Offset, in words, from the end > of the pointer to the start of the first > element of the list. Signed. The prior code only supported positive offsets because it used the Java >>> operator, which is an unsigned shift operator. The Java >> operator is the signed shift operator. Audited the remaining uses of the shift operator; they were correct and they are documented as such. Positive offsets are only guaranteed in a Canonical message.
capnproto · Sep 1, 2023 · 5ed5ec6 · 5ed5ec6
1 parent 44a975b
commit 5ed5ec6
Show file tree

Hide file tree

Showing 5 changed files with 64 additions and 1 deletion.
diff --git a/runtime/src/main/java/org/capnproto/FarPointer.java b/runtime/src/main/java/org/capnproto/FarPointer.java
@@ -29,10 +29,14 @@ public static int getSegmentId(long ref) {
     }
 
     public static int positionInSegment(long ref) {
+        /* The [ref] Offset (the "C" section) is "Unsigned",
+           so use unsigned >>> operator. */
         return WirePointer.offsetAndKind(ref) >>> 3;
     }
 
     public static boolean isDoubleFar(long ref) {
+        /* The [ref] Offset (the "C" section) is "Unsigned",
+           so use unsigned >>> operator. */
         return ((WirePointer.offsetAndKind(ref) >>> 2) & 1) != 0;
     }
 

diff --git a/runtime/src/main/java/org/capnproto/ListPointer.java b/runtime/src/main/java/org/capnproto/ListPointer.java
@@ -29,6 +29,9 @@ public static byte elementSize(long ref) {
     }
 
     public static int elementCount(long ref) {
+        /* The [ref] List Size (the "D" section) is not specified
+           as Signed or Unsigned, but number of elements is inherently non-negative.
+           So use unsigned >>> operator. */
         return WirePointer.upper32Bits(ref) >>> 3;
     }
 

diff --git a/runtime/src/main/java/org/capnproto/StructPointer.java b/runtime/src/main/java/org/capnproto/StructPointer.java
@@ -30,6 +30,9 @@ public static int dataSize(long ref) {
     }
 
     public static int ptrCount(long ref) {
+        /* The [ref] Pointer Section Size (the "D" section) is not specified
+           as Signed or Unsigned, but section size is inherently non-negative.
+           So use unsigned >>> operator. */
         return WirePointer.upper32Bits(ref) >>> 16;
     }
 

diff --git a/runtime/src/main/java/org/capnproto/WirePointer.java b/runtime/src/main/java/org/capnproto/WirePointer.java
@@ -42,7 +42,9 @@ public static byte kind(long wirePointer) {
     }
 
     public static int target(int offset, long wirePointer) {
-        return offset + 1 + (offsetAndKind(wirePointer) >>> 2);
+        /* The [wirePointer] Offset (the "B" section) is "Signed",
+           so use signed >> operator. */
+        return offset + 1 + (offsetAndKind(wirePointer) >> 2);
     }
 
     public static void setKindAndTarget(ByteBuffer buffer, int offset, byte kind, int targetOffset) {

diff --git a/runtime/src/test/java/org/capnproto/SegmentReaderTest.java b/runtime/src/test/java/org/capnproto/SegmentReaderTest.java
@@ -1,9 +1,11 @@
 package org.capnproto;
 
+import org.capnproto.WireHelpers.FollowFarsResult;
 import org.hamcrest.MatcherAssert;
 import org.junit.Test;
 
 import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
 
 import static org.hamcrest.CoreMatchers.is;
 
@@ -29,4 +31,53 @@ public void twoWordsAtLastWordShouldNotBeInBounds() {
         SegmentReader segmentReader = new SegmentReader(byteBuffer, null);
         MatcherAssert.assertThat(segmentReader.isInBounds(7, 2), is(false));
     }
+
+    @Test
+    public void validSegmentWithNegativeOffsetShouldBeInBounds() {
+        int refOffset;
+        long ref;
+        int refTarget;
+        int dataSizeWords;
+        int wordSize;
+
+        /*
+        Binary data:
+            echo -n
+              'FAAAAAEAAQDJqtK2cBpJhZ2LUEVMkYblyarStnAaSYWdi1BFTJGG4e3///8CAQAAAgAAAAAAAAD0////AAABAA=='
+            | base64 -d
+
+        Verify it is valid with:
+            capnp decode --flat dksdk_std_schema.capnp GenericReturn
+        where the .capnp comes from
+        https://gitlab.com/diskuv/dksdk-schema/-/blob/afbf9564a60f2670f6b9dfb3c423fc55dd4c3013/src/dksdk_std_schema.capnp
+        */
+        ByteBuffer byteBuffer = ByteBuffer.wrap(new byte[] {
+            (byte)0x14, (byte)0x00, (byte)0x00, (byte)0x00, (byte)0x01, (byte)0x00, (byte)0x01, (byte)0x00,
+            (byte)0xC9, (byte)0xAA, (byte)0xD2, (byte)0xB6, (byte)0x70, (byte)0x1A, (byte)0x49, (byte)0x85,
+            (byte)0x9D, (byte)0x8B, (byte)0x50, (byte)0x45, (byte)0x4C, (byte)0x91, (byte)0x86, (byte)0xE5,
+            (byte)0xC9, (byte)0xAA, (byte)0xD2, (byte)0xB6, (byte)0x70, (byte)0x1A, (byte)0x49, (byte)0x85,
+            (byte)0x9D, (byte)0x8B, (byte)0x50, (byte)0x45, (byte)0x4C, (byte)0x91, (byte)0x86, (byte)0xE1,
+            (byte)0xED, (byte)0xFF, (byte)0xFF, (byte)0xFF, (byte)0x02, (byte)0x01, (byte)0x00, (byte)0x00,
+            (byte)0x02, (byte)0x00, (byte)0x00, (byte)0x00, (byte)0x00, (byte)0x00, (byte)0x00, (byte)0x00,
+            (byte)0xF4, (byte)0xFF, (byte)0xFF, (byte)0xFF, (byte)0x00, (byte)0x00, (byte)0x01, (byte)0x00            
+        }).order(ByteOrder.LITTLE_ENDIAN);
+        SegmentReader segment = new SegmentReader(byteBuffer, null);
+
+        /* Read root Struct: GenericReturn. */
+        refOffset = 0; /* At the root STRUCT POINTER */
+        ref = segment.get(refOffset);
+        refTarget = WirePointer.target(refOffset, ref);
+        dataSizeWords = StructPointer.dataSize(ref);
+        wordSize = dataSizeWords + StructPointer.ptrCount(ref);
+        MatcherAssert.assertThat(segment.isInBounds(refTarget, wordSize), is(true));
+
+        /* Read inner Struct: ComObject. */
+        refOffset = refTarget + dataSizeWords; /* At the inner STRUCT POINTER */
+        ref = segment.get(refOffset);
+        refTarget = WirePointer.target(refOffset, ref);
+        dataSizeWords = StructPointer.dataSize(ref);
+        wordSize = dataSizeWords + StructPointer.ptrCount(ref);
+        MatcherAssert.assertThat(segment.isInBounds(refTarget, wordSize), is(true));    
+    }
+
 }