From a01519e4f86690323647b5db45d9ffcb184b1a84 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 19 Aug 2016 11:19:30 +0200 Subject: [PATCH] [#5718] Result of ByteBufUtil.compare(ByteBuf a, ByteBuf b) is dependent on ByteOrder of supplied ByteBufs Motivation: Result of ByteBufUtil.compare(ByteBuf a, ByteBuf b) is dependent on ByteOrder of supplied ByteBufs which should not be the case (as stated in the javadocs). Modifications: Ensure we get a consistent behavior when calling ByteBufUtil.compare(ByteBuf a, ByteBuf b) and not depend on ByteOrder. Result: ByteBufUtil.compare(ByteBuf a, ByteBuf b) and so AbstractByteBuf.compare(...) works correctly as stated in the javadocs. --- .../main/java/io/netty/buffer/ByteBuf.java | 2 +- .../java/io/netty/buffer/ByteBufUtil.java | 99 ++++++++++++------- .../io/netty/buffer/AbstractByteBufTest.java | 23 +++++ 3 files changed, 88 insertions(+), 36 deletions(-) diff --git a/buffer/src/main/java/io/netty/buffer/ByteBuf.java b/buffer/src/main/java/io/netty/buffer/ByteBuf.java index e72fb21f34..f51caabd62 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 { /** * 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 591bc7db91..79403c61be 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 final class ByteBufUtil { 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; + for (int aEnd = aIndex + byteCount; aIndex < aEnd; ++aIndex, ++bIndex) { + int comp = bufferA.getUnsignedByte(aIndex) - bufferB.getUnsignedByte(bIndex); + if (comp != 0) { + return comp; } - if (va < vb) { - return -1; - } - 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. diff --git a/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java b/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java index 47bca1f5eb..f4e6b90c79 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.File; 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 abstract class AbstractByteBufTest { 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();