Repository: netty/netty
Issue: Result of ByteBufUtil.compare(ByteBuf a, ByteBuf b) is dependent on ByteOrder of supplied ByteBufs
Labels: defect

This should be similar to memcmp() or strcmp() as described in the java-doc.

This is because the [ByteBufUtil.compare(ByteBuf, ByteBuf)](https://github.com/netty/netty/blob/4.1/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java#L226) compare the `ByteBuf`s as integers and not raw bytes.

```
Sample Code
========
  public static void main(String[] args) {
    ByteBuf buf1 = Unpooled.wrappedBuffer("1234".getBytes()).order(ByteOrder.LITTLE_ENDIAN);
    ByteBuf buf2 = Unpooled.wrappedBuffer("4321".getBytes()).order(ByteOrder.LITTLE_ENDIAN);
    System.out.println("\"1234\".compareTo(\"4321\") = " + buf1.compareTo(buf2));

    buf1 = Unpooled.wrappedBuffer("1234".getBytes()).order(ByteOrder.BIG_ENDIAN);
    buf2 = Unpooled.wrappedBuffer("4321".getBytes()).order(ByteOrder.BIG_ENDIAN);
    System.out.println("\"1234\".compareTo(\"4321\") = " + buf1.compareTo(buf2));
  }

Output
=====
"1234".compareTo("4321") = 1
"1234".compareTo("4321") = -1
```


Issue comments:
@adityakishore so you basically say both should return 1. Right ?

@adityakishore ups -1 I mean :)

Let me fix this.

Yes, both should return -1 if we want to retain the memcmp/strcmp behavior.

For that, both buffer should be compared as BIG_ENDIAN if we still want to retain the performance optimization of comparing 4 bytes at a time.

@adityakishore PTAL https://github.com/netty/netty/pull/5732

fixed by #5732

Thanks for fixing this.


Pull request review comments:
nit: remove space before `=`

if you change `uintCount` to something like `aEnd` you could get rid of `i` and a decrement on every iteration.

``` java
for (;aIndex < aEnd; aIndex += 4, bIndex +=4) {
..
}
```

consider removing `va` and `vb` for the following ... trade an aritmatic operation for potentially a conditional

``` java
long cmp = bufferA.getUnsignedInt(aIndex) - bufferB.getUnsignedInt(bIndex);
if (cmp == 0) {
  continue;
}
return cmp;
```

I think this could overflow ...

> Am 23.08.2016 um 19:36 schrieb Scott Mitchell notifications@github.com:
> 
> In buffer/src/main/java/io/netty/buffer/ByteBufUtil.java:
> 
> > ```
> >      }
> > 
> >      return aLen - bLen;
> >  }
> > ```
> > -    private static int compareUintBigEndian(
> > -            ByteBuf bufferA, ByteBuf bufferB, int aIndex, int bIndex, int uintCount) {
> > -        for (int i = uintCount; i > 0; --i, aIndex += 4, bIndex += 4) {
> > -            long va = bufferA.getUnsignedInt(aIndex);
> > -            long vb = bufferB.getUnsignedInt(bIndex);
> > -            if (va == vb) {
> > -                // fast-path
> > -                continue;
> > -            }
> > -            if (va > vb) {
> >   consider removing va and vb for the following ... trade an aritmatic operation for potentially a conditional
> 
> long cmp = bufferA.getUnsignedInt(aIndex) - bufferB.getUnsignedInt(bIndex);
> if (cmp == 0) {
>   continue;
> }
> return cmp;
> —
> You are receiving this because you were assigned.
> Reply to this email directly, view it on GitHub, or mute the thread.

we are using `long` to represent an "unsigned int" ... seems like we should not overflow in this situation?

is `aEnd` correct here? does `uintCount` need to be multiplied by 4 to be in unit of `bytes`?

maybe consider passing in `uintCountIncrement` from outside?

@Scottmitch good catch... Let me fix it.


Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/ByteBuf.java b/buffer/src/main/java/io/netty/buffer/ByteBuf.java
index e72fb21f34c..f51caabd627 100644
--- a/buffer/src/main/java/io/netty/buffer/ByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/ByteBuf.java
@@ -2332,7 +2332,7 @@ public abstract class ByteBuf implements ReferenceCounted, Comparable<ByteBuf> {
 
     /**
      * Compares the content of the specified buffer to the content of this
-     * buffer.  Comparison is performed in the same manner with the string
+     * buffer. Comparison is performed in the same manner with the string
      * comparison functions of various languages such as {@code strcmp},
      * {@code memcmp} and {@link String#compareTo(String)}.
      */
diff --git a/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java b/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java
index 591bc7db911..79403c61be1 100644
--- a/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java
+++ b/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java
@@ -229,54 +229,83 @@ public static int compare(ByteBuf bufferA, ByteBuf bufferB) {
         final int minLength = Math.min(aLen, bLen);
         final int uintCount = minLength >>> 2;
         final int byteCount = minLength & 3;
-
         int aIndex = bufferA.readerIndex();
         int bIndex = bufferB.readerIndex();
 
-        if (bufferA.order() == bufferB.order()) {
-            for (int i = uintCount; i > 0; i --) {
-                long va = bufferA.getUnsignedInt(aIndex);
-                long vb = bufferB.getUnsignedInt(bIndex);
-                if (va > vb) {
-                    return 1;
-                }
-                if (va < vb) {
-                    return -1;
-                }
-                aIndex += 4;
-                bIndex += 4;
+        if (uintCount > 0) {
+            boolean bufferAIsBigEndian = bufferA.order() == ByteOrder.BIG_ENDIAN;
+            final long res;
+            int uintCountIncrement = uintCount << 2;
+
+            if (bufferA.order() == bufferB.order()) {
+                res = bufferAIsBigEndian ? compareUintBigEndian(bufferA, bufferB, aIndex, bIndex, uintCountIncrement) :
+                        compareUintLittleEndian(bufferA, bufferB, aIndex, bIndex, uintCountIncrement);
+            } else {
+                res = bufferAIsBigEndian ? compareUintBigEndianA(bufferA, bufferB, aIndex, bIndex, uintCountIncrement) :
+                        compareUintBigEndianB(bufferA, bufferB, aIndex, bIndex, uintCountIncrement);
             }
-        } else {
-            for (int i = uintCount; i > 0; i --) {
-                long va = bufferA.getUnsignedInt(aIndex);
-                long vb = swapInt(bufferB.getInt(bIndex)) & 0xFFFFFFFFL;
-                if (va > vb) {
-                    return 1;
-                }
-                if (va < vb) {
-                    return -1;
-                }
-                aIndex += 4;
-                bIndex += 4;
+            if (res != 0) {
+                // Ensure we not overflow when cast
+                return (int) Math.min(Integer.MAX_VALUE, res);
             }
+            aIndex += uintCountIncrement;
+            bIndex += uintCountIncrement;
         }
 
-        for (int i = byteCount; i > 0; i --) {
-            short va = bufferA.getUnsignedByte(aIndex);
-            short vb = bufferB.getUnsignedByte(bIndex);
-            if (va > vb) {
-                return 1;
-            }
-            if (va < vb) {
-                return -1;
+        for (int aEnd = aIndex + byteCount; aIndex < aEnd; ++aIndex, ++bIndex) {
+            int comp = bufferA.getUnsignedByte(aIndex) - bufferB.getUnsignedByte(bIndex);
+            if (comp != 0) {
+                return comp;
             }
-            aIndex ++;
-            bIndex ++;
         }
 
         return aLen - bLen;
     }
 
+    private static long compareUintBigEndian(
+            ByteBuf bufferA, ByteBuf bufferB, int aIndex, int bIndex, int uintCountIncrement) {
+        for (int aEnd = aIndex + uintCountIncrement; aIndex < aEnd; aIndex += 4, bIndex += 4) {
+            long comp = bufferA.getUnsignedInt(aIndex) - bufferB.getUnsignedInt(bIndex);
+            if (comp != 0) {
+                return comp;
+            }
+        }
+        return 0;
+    }
+
+    private static long compareUintLittleEndian(
+            ByteBuf bufferA, ByteBuf bufferB, int aIndex, int bIndex, int uintCountIncrement) {
+        for (int aEnd = aIndex + uintCountIncrement; aIndex < aEnd; aIndex += 4, bIndex += 4) {
+            long comp = bufferA.getUnsignedIntLE(aIndex) - bufferB.getUnsignedIntLE(bIndex);
+            if (comp != 0) {
+                return comp;
+            }
+        }
+        return 0;
+    }
+
+    private static long compareUintBigEndianA(
+            ByteBuf bufferA, ByteBuf bufferB, int aIndex, int bIndex, int uintCountIncrement) {
+        for (int aEnd = aIndex + uintCountIncrement; aIndex < aEnd; aIndex += 4, bIndex += 4) {
+            long comp =  bufferA.getUnsignedInt(aIndex) - bufferB.getUnsignedIntLE(bIndex);
+            if (comp != 0) {
+                return comp;
+            }
+        }
+        return 0;
+    }
+
+    private static long compareUintBigEndianB(
+            ByteBuf bufferA, ByteBuf bufferB, int aIndex, int bIndex, int uintCountIncrement) {
+        for (int aEnd = aIndex + uintCountIncrement; aIndex < aEnd; aIndex += 4, bIndex += 4) {
+            long comp =  bufferA.getUnsignedIntLE(aIndex) - bufferB.getUnsignedInt(bIndex);
+            if (comp != 0) {
+                return comp;
+            }
+        }
+        return 0;
+    }
+
     /**
      * The default implementation of {@link ByteBuf#indexOf(int, int, byte)}.
      * This method is useful when implementing a new buffer type.


Test patch:
diff --git a/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java b/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java
index 47bca1f5ebd..f4e6b90c799 100644
--- a/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java
+++ b/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java
@@ -31,6 +31,7 @@
 import java.io.IOException;
 import java.io.RandomAccessFile;
 import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
 import java.nio.ReadOnlyBufferException;
 import java.nio.channels.Channels;
 import java.nio.channels.FileChannel;
@@ -1822,6 +1823,28 @@ public void testCompareTo() {
                 buffer.retainedSlice(0, 31).compareTo(wrappedBuffer(value).order(LITTLE_ENDIAN)) < 0);
     }
 
+    @Test
+    public void testCompareTo2() {
+        byte[] bytes = {1, 2, 3, 4};
+        byte[] bytesReversed = {4, 3, 2, 1};
+
+        ByteBuf buf1 = newBuffer(4).clear().writeBytes(bytes).order(ByteOrder.LITTLE_ENDIAN);
+        ByteBuf buf2 = newBuffer(4).clear().writeBytes(bytesReversed).order(ByteOrder.LITTLE_ENDIAN);
+        ByteBuf buf3 = newBuffer(4).clear().writeBytes(bytes).order(ByteOrder.BIG_ENDIAN);
+        ByteBuf buf4 = newBuffer(4).clear().writeBytes(bytesReversed).order(ByteOrder.BIG_ENDIAN);
+        try {
+            assertEquals(buf1.compareTo(buf2), buf3.compareTo(buf4));
+            assertEquals(buf2.compareTo(buf1), buf4.compareTo(buf3));
+            assertEquals(buf1.compareTo(buf3), buf2.compareTo(buf4));
+            assertEquals(buf3.compareTo(buf1), buf4.compareTo(buf2));
+        } finally {
+            buf1.release();
+            buf2.release();
+            buf3.release();
+            buf4.release();
+        }
+    }
+
     @Test
     public void testToString() {
         buffer.clear();