From 2e4454d7c4646d785dbb627651aa7235d03b1ac9 Mon Sep 17 00:00:00 2001 From: Nick Dimiduk Date: Mon, 26 Aug 2013 10:12:45 -0700 Subject: [PATCH] HBASE-9332 OrderedBytes error decoding strings The length calculation used when decoding Strings is incorrect. Update the unit tests to detect this failure across all encodings and fix bug. Silence findbugs warning in SimplePositionedByteRange around overriding equality. Correct Javadoc describing the role of position in the same. --- .../org/apache/hadoop/hbase/util/OrderedBytes.java | 12 +- .../hbase/util/SimplePositionedByteRange.java | 6 +- .../apache/hadoop/hbase/util/TestOrderedBytes.java | 202 ++++++++++++--------- 3 files changed, 130 insertions(+), 90 deletions(-) diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/OrderedBytes.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/OrderedBytes.java index 74775ae..b99fc7b 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/OrderedBytes.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/OrderedBytes.java @@ -973,18 +973,18 @@ public class OrderedBytes { byte[] a = src.getBytes(); final int offset = src.getOffset(), start = src.getPosition(); final byte terminator = ord.apply(TERM); - int i = offset + start; - for (; a[i] != terminator; i++) + int rawStartPos = offset + start, rawTermPos = rawStartPos; + for (; a[rawTermPos] != terminator; rawTermPos++) ; - src.setPosition(i - offset + 1); // advance position to TERM + 1 + src.setPosition(rawTermPos - offset + 1); // advance position to TERM + 1 if (DESCENDING == ord) { // make a copy so that we don't disturb encoded value with ord. - byte[] copy = new byte[i - offset - 1]; - System.arraycopy(a, offset + start, copy, 0, copy.length); + byte[] copy = new byte[rawTermPos - rawStartPos]; + System.arraycopy(a, rawStartPos, copy, 0, copy.length); ord.apply(copy); return new String(copy, UTF8); } else { - return new String(a, offset + start, i - offset - 1, UTF8); + return new String(a, rawStartPos, rawTermPos - rawStartPos, UTF8); } } diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/SimplePositionedByteRange.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/SimplePositionedByteRange.java index 8a61548..c13539d 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/SimplePositionedByteRange.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/SimplePositionedByteRange.java @@ -28,11 +28,13 @@ import com.google.common.annotations.VisibleForTesting; /** * Extends the basic {@link SimpleByteRange} implementation with position * support. {@code position} is considered transient, not fundamental to the - * definition of the range, and does not participate in comparison or copy - * operations. + * definition of the range, and does not participate in + * {@link #compareTo(ByteRange)}, {@link #hashCode()}, or + * {@link #equals(Object)}. {@code Position} is retained by copy operations. */ @InterfaceAudience.Public @InterfaceStability.Evolving +@edu.umd.cs.findbugs.annotations.SuppressWarnings("EQ_DOESNT_OVERRIDE_EQUALS") public class SimplePositionedByteRange extends SimpleByteRange implements PositionedByteRange { /** diff --git a/hbase-common/src/test/java/org/apache/hadoop/hbase/util/TestOrderedBytes.java b/hbase-common/src/test/java/org/apache/hadoop/hbase/util/TestOrderedBytes.java index 5e3282a..509d76e 100644 --- a/hbase-common/src/test/java/org/apache/hadoop/hbase/util/TestOrderedBytes.java +++ b/hbase-common/src/test/java/org/apache/hadoop/hbase/util/TestOrderedBytes.java @@ -160,27 +160,31 @@ public class TestOrderedBytes { */ for (Order ord : new Order[] { Order.ASCENDING, Order.DESCENDING }) { for (int i = 0; i < I_VALS.length; i++) { - // allocate a buffer 2-bytes larger than necessary and place our range over the center. - byte[] a = new byte[I_LENGTHS[i] + 2]; - PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, I_LENGTHS[i]); + // allocate a buffer 3-bytes larger than necessary to detect over/underflow + byte[] a = new byte[I_LENGTHS[i] + 3]; + PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, I_LENGTHS[i] + 1); + buf1.setPosition(1); // verify encode assertEquals("Surprising return value.", I_LENGTHS[i], OrderedBytes.encodeNumeric(buf1, I_VALS[i], ord)); - assertEquals("Surprising serialized length.", I_LENGTHS[i], buf1.getPosition()); + assertEquals("Broken test: serialization did not consume entire buffer.", + buf1.getLength(), buf1.getPosition()); + assertEquals("Surprising serialized length.", I_LENGTHS[i], buf1.getPosition() - 1); assertEquals("Buffer underflow.", 0, a[0]); + assertEquals("Buffer underflow.", 0, a[1]); assertEquals("Buffer overflow.", 0, a[a.length - 1]); // verify skip - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Surprising return value.", I_LENGTHS[i], OrderedBytes.skip(buf1)); - assertEquals("Did not skip enough bytes.", I_LENGTHS[i], buf1.getPosition()); + assertEquals("Did not skip enough bytes.", I_LENGTHS[i], buf1.getPosition() - 1); // verify decode - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Deserialization failed.", I_VALS[i].longValue(), OrderedBytes.decodeNumericAsLong(buf1)); - assertEquals("Did not consume enough bytes.", I_LENGTHS[i], buf1.getPosition()); + assertEquals("Did not consume enough bytes.", I_LENGTHS[i], buf1.getPosition() - 1); } } @@ -223,27 +227,31 @@ public class TestOrderedBytes { */ for (Order ord : new Order[] { Order.ASCENDING, Order.DESCENDING }) { for (int i = 0; i < D_VALS.length; i++) { - // allocate a buffer 2-bytes larger than necessary and place our range over the center. - byte[] a = new byte[D_LENGTHS[i] + 2]; - PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, D_LENGTHS[i]); + // allocate a buffer 3-bytes larger than necessary to detect over/underflow + byte[] a = new byte[D_LENGTHS[i] + 3]; + PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, D_LENGTHS[i] + 1); + buf1.setPosition(1); // verify encode assertEquals("Surprising return value.", D_LENGTHS[i], OrderedBytes.encodeNumeric(buf1, D_VALS[i], ord)); - assertEquals("Surprising serialized length.", D_LENGTHS[i], buf1.getPosition()); + assertEquals("Broken test: serialization did not consume entire buffer.", + buf1.getLength(), buf1.getPosition()); + assertEquals("Surprising serialized length.", D_LENGTHS[i], buf1.getPosition() - 1); assertEquals("Buffer underflow.", 0, a[0]); + assertEquals("Buffer underflow.", 0, a[1]); assertEquals("Buffer overflow.", 0, a[a.length - 1]); // verify skip - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Surprising return value.", D_LENGTHS[i], OrderedBytes.skip(buf1)); - assertEquals("Did not skip enough bytes.", D_LENGTHS[i], buf1.getPosition()); + assertEquals("Did not skip enough bytes.", D_LENGTHS[i], buf1.getPosition() - 1); // verify decode - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Deserialization failed.", D_VALS[i].doubleValue(), OrderedBytes.decodeNumericAsDouble(buf1), MIN_EPSILON); - assertEquals("Did not consume enough bytes.", D_LENGTHS[i], buf1.getPosition()); + assertEquals("Did not consume enough bytes.", D_LENGTHS[i], buf1.getPosition() - 1); } } @@ -286,31 +294,35 @@ public class TestOrderedBytes { */ for (Order ord : new Order[] { Order.ASCENDING, Order.DESCENDING }) { for (int i = 0; i < BD_VALS.length; i++) { - // allocate a buffer 2-bytes larger than necessary and place our range over the center. - byte[] a = new byte[BD_LENGTHS[i] + 2]; - PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, BD_LENGTHS[i]); + // allocate a buffer 3-bytes larger than necessary to detect over/underflow + byte[] a = new byte[BD_LENGTHS[i] + 3]; + PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, BD_LENGTHS[i] + 1); + buf1.setPosition(1); // verify encode assertEquals("Surprising return value.", BD_LENGTHS[i], OrderedBytes.encodeNumeric(buf1, BD_VALS[i], ord)); - assertEquals("Surprising serialized length.", BD_LENGTHS[i], buf1.getPosition()); + assertEquals("Broken test: serialization did not consume entire buffer.", + buf1.getLength(), buf1.getPosition()); + assertEquals("Surprising serialized length.", BD_LENGTHS[i], buf1.getPosition() - 1); assertEquals("Buffer underflow.", 0, a[0]); + assertEquals("Buffer underflow.", 0, a[1]); assertEquals("Buffer overflow.", 0, a[a.length - 1]); // verify skip - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Surprising return value.", BD_LENGTHS[i], OrderedBytes.skip(buf1)); - assertEquals("Did not skip enough bytes.", BD_LENGTHS[i], buf1.getPosition()); + assertEquals("Did not skip enough bytes.", BD_LENGTHS[i], buf1.getPosition() - 1); // verify decode - buf1.setPosition(0); + buf1.setPosition(1); BigDecimal decoded = OrderedBytes.decodeNumericAsBigDecimal(buf1); if (null == BD_VALS[i]) { assertEquals(BD_VALS[i], decoded); } else { assertEquals("Deserialization failed.", 0, BD_VALS[i].compareTo(decoded)); } - assertEquals("Did not consume enough bytes.", BD_LENGTHS[i], buf1.getPosition()); + assertEquals("Did not consume enough bytes.", BD_LENGTHS[i], buf1.getPosition() - 1); } } } @@ -360,27 +372,31 @@ public class TestOrderedBytes { */ for (Order ord : new Order[] { Order.ASCENDING, Order.DESCENDING }) { for (int i = 0; i < vals.length; i++) { - // allocate a buffer 2-bytes larger than necessary and place our range over the center. - byte[] a = new byte[5 + 2]; - PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, 5); + // allocate a buffer 3-bytes larger than necessary to detect over/underflow + byte[] a = new byte[5 + 3]; + PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, 5 + 1); + buf1.setPosition(1); // verify encode assertEquals("Surprising return value.", 5, OrderedBytes.encodeInt32(buf1, vals[i], ord)); - assertEquals("Surprising serialized length.", 5, buf1.getPosition()); + assertEquals("Broken test: serialization did not consume entire buffer.", + buf1.getLength(), buf1.getPosition()); + assertEquals("Surprising serialized length.", 5, buf1.getPosition() - 1); assertEquals("Buffer underflow.", 0, a[0]); + assertEquals("Buffer underflow.", 0, a[1]); assertEquals("Buffer overflow.", 0, a[a.length - 1]); // verify skip - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Surprising return value.", 5, OrderedBytes.skip(buf1)); - assertEquals("Did not skip enough bytes.", 5, buf1.getPosition()); + assertEquals("Did not skip enough bytes.", 5, buf1.getPosition() - 1); // verify decode - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Deserialization failed.", vals[i].intValue(), OrderedBytes.decodeInt32(buf1)); - assertEquals("Did not consume enough bytes.", 5, buf1.getPosition()); + assertEquals("Did not consume enough bytes.", 5, buf1.getPosition() - 1); } } @@ -423,27 +439,31 @@ public class TestOrderedBytes { */ for (Order ord : new Order[] { Order.ASCENDING, Order.DESCENDING }) { for (int i = 0; i < vals.length; i++) { - // allocate a buffer 2-bytes larger than necessary and place our range over the center. - byte[] a = new byte[9 + 2]; - PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, 9); + // allocate a buffer 3-bytes larger than necessary to detect over/underflow + byte[] a = new byte[9 + 3]; + PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, 9 + 1); + buf1.setPosition(1); // verify encode assertEquals("Surprising return value.", 9, OrderedBytes.encodeInt64(buf1, vals[i], ord)); - assertEquals("Surprising serialized length.", 9, buf1.getPosition()); + assertEquals("Broken test: serialization did not consume entire buffer.", + buf1.getLength(), buf1.getPosition()); + assertEquals("Surprising serialized length.", 9, buf1.getPosition() - 1); assertEquals("Buffer underflow.", 0, a[0]); + assertEquals("Buffer underflow.", 0, a[1]); assertEquals("Buffer overflow.", 0, a[a.length - 1]); // verify skip - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Surprising return value.", 9, OrderedBytes.skip(buf1)); - assertEquals("Did not skip enough bytes.", 9, buf1.getPosition()); + assertEquals("Did not skip enough bytes.", 9, buf1.getPosition() - 1); // verify decode - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Deserialization failed.", vals[i].longValue(), OrderedBytes.decodeInt64(buf1)); - assertEquals("Did not consume enough bytes.", 9, buf1.getPosition()); + assertEquals("Did not consume enough bytes.", 9, buf1.getPosition() - 1); } } @@ -487,28 +507,32 @@ public class TestOrderedBytes { */ for (Order ord : new Order[] { Order.ASCENDING, Order.DESCENDING }) { for (int i = 0; i < vals.length; i++) { - // allocate a buffer 2-bytes larger than necessary and place our range over the center. - byte[] a = new byte[5 + 2]; - PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, 5); + // allocate a buffer 3-bytes larger than necessary to detect over/underflow + byte[] a = new byte[5 + 3]; + PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, 5 + 1); + buf1.setPosition(1); // verify encode assertEquals("Surprising return value.", 5, OrderedBytes.encodeFloat32(buf1, vals[i], ord)); - assertEquals("Surprising serialized length.", 5, buf1.getPosition()); + assertEquals("Broken test: serialization did not consume entire buffer.", + buf1.getLength(), buf1.getPosition()); + assertEquals("Surprising serialized length.", 5, buf1.getPosition() - 1); assertEquals("Buffer underflow.", 0, a[0]); + assertEquals("Buffer underflow.", 0, a[1]); assertEquals("Buffer overflow.", 0, a[a.length - 1]); // verify skip - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Surprising return value.", 5, OrderedBytes.skip(buf1)); - assertEquals("Did not skip enough bytes.", 5, buf1.getPosition()); + assertEquals("Did not skip enough bytes.", 5, buf1.getPosition() - 1); // verify decode - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Deserialization failed.", Float.floatToIntBits(vals[i].floatValue()), Float.floatToIntBits(OrderedBytes.decodeFloat32(buf1))); - assertEquals("Did not consume enough bytes.", 5, buf1.getPosition()); + assertEquals("Did not consume enough bytes.", 5, buf1.getPosition() - 1); } } @@ -553,28 +577,32 @@ public class TestOrderedBytes { */ for (Order ord : new Order[] { Order.ASCENDING, Order.DESCENDING }) { for (int i = 0; i < vals.length; i++) { - // allocate a buffer 2-bytes larger than necessary and place our range over the center. - byte[] a = new byte[9 + 2]; - PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, 9); + // allocate a buffer 3-bytes larger than necessary to detect over/underflow + byte[] a = new byte[9 + 3]; + PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, 9 + 1); + buf1.setPosition(1); // verify encode assertEquals("Surprising return value.", 9, OrderedBytes.encodeFloat64(buf1, vals[i], ord)); - assertEquals("Surprising serialized length.", 9, buf1.getPosition()); + assertEquals("Broken test: serialization did not consume entire buffer.", + buf1.getLength(), buf1.getPosition()); + assertEquals("Surprising serialized length.", 9, buf1.getPosition() - 1); assertEquals("Buffer underflow.", 0, a[0]); + assertEquals("Buffer underflow.", 0, a[1]); assertEquals("Buffer overflow.", 0, a[a.length - 1]); // verify skip - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Surprising return value.", 9, OrderedBytes.skip(buf1)); - assertEquals("Did not skip enough bytes.", 9, buf1.getPosition()); + assertEquals("Did not skip enough bytes.", 9, buf1.getPosition() - 1); // verify decode - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Deserialization failed.", Double.doubleToLongBits(vals[i].doubleValue()), Double.doubleToLongBits(OrderedBytes.decodeFloat64(buf1))); - assertEquals("Did not consume enough bytes.", 9, buf1.getPosition()); + assertEquals("Did not consume enough bytes.", 9, buf1.getPosition() - 1); } } @@ -619,26 +647,30 @@ public class TestOrderedBytes { */ for (Order ord : new Order[] { Order.ASCENDING, Order.DESCENDING }) { for (int i = 0; i < vals.length; i++) { - // allocate a buffer 2-bytes larger than necessary and place our range over the center. - byte[] a = new byte[expectedLengths[i] + 2]; - PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, expectedLengths[i]); + // allocate a buffer 3-bytes larger than necessary to detect over/underflow + byte[] a = new byte[expectedLengths[i] + 3]; + PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, expectedLengths[i] + 1); + buf1.setPosition(1); // verify encode assertEquals("Surprising return value.", expectedLengths[i], OrderedBytes.encodeString(buf1, vals[i], ord)); - assertEquals("Surprising serialized length.", expectedLengths[i], buf1.getPosition()); + assertEquals("Broken test: serialization did not consume entire buffer.", + buf1.getLength(), buf1.getPosition()); + assertEquals("Surprising serialized length.", expectedLengths[i], buf1.getPosition() - 1); assertEquals("Buffer underflow.", 0, a[0]); + assertEquals("Buffer underflow.", 0, a[1]); assertEquals("Buffer overflow.", 0, a[a.length - 1]); // verify skip - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Surprising return value.", expectedLengths[i], OrderedBytes.skip(buf1)); - assertEquals("Did not skip enough bytes.", expectedLengths[i], buf1.getPosition()); + assertEquals("Did not skip enough bytes.", expectedLengths[i], buf1.getPosition() - 1); // verify decode - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Deserialization failed.", vals[i], OrderedBytes.decodeString(buf1)); - assertEquals("Did not consume enough bytes.", expectedLengths[i], buf1.getPosition()); + assertEquals("Did not consume enough bytes.", expectedLengths[i], buf1.getPosition() - 1); } } @@ -717,28 +749,31 @@ public class TestOrderedBytes { */ for (Order ord : new Order[] { Order.ASCENDING, Order.DESCENDING }) { for (byte[] val : vals) { - // allocate a buffer 2-bytes larger than necessary and place our range over the center. + // allocate a buffer 3-bytes larger than necessary to detect over/underflow int expectedLen = OrderedBytes.blobVarEncodedLength(val.length); - byte[] a = new byte[expectedLen + 2]; - PositionedByteRange buf1 = - new SimplePositionedByteRange(a, 1, expectedLen); + byte[] a = new byte[expectedLen + 3]; + PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, expectedLen + 1); + buf1.setPosition(1); // verify encode assertEquals("Surprising return value.", expectedLen, OrderedBytes.encodeBlobVar(buf1, val, ord)); - assertEquals("Surprising serialized length.", expectedLen, buf1.getPosition()); + assertEquals("Broken test: serialization did not consume entire buffer.", + buf1.getLength(), buf1.getPosition()); + assertEquals("Surprising serialized length.", expectedLen, buf1.getPosition() - 1); assertEquals("Buffer underflow.", 0, a[0]); + assertEquals("Buffer underflow.", 0, a[1]); assertEquals("Buffer overflow.", 0, a[a.length - 1]); // verify skip - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Surprising return value.", expectedLen, OrderedBytes.skip(buf1)); - assertEquals("Did not skip enough bytes.", expectedLen, buf1.getPosition()); + assertEquals("Did not skip enough bytes.", expectedLen, buf1.getPosition() - 1); // verify decode - buf1.setPosition(0); + buf1.setPosition(1); assertArrayEquals("Deserialization failed.", val, OrderedBytes.decodeBlobVar(buf1)); - assertEquals("Did not consume enough bytes.", expectedLen, buf1.getPosition()); + assertEquals("Did not consume enough bytes.", expectedLen, buf1.getPosition() - 1); } } @@ -789,28 +824,31 @@ public class TestOrderedBytes { */ for (Order ord : new Order[] { Order.ASCENDING, Order.DESCENDING }) { for (byte[] val : vals) { - // allocate a buffer 2-bytes larger than necessary and place our range over the center. + // allocate a buffer 3-bytes larger than necessary to detect over/underflow int expectedLen = val.length + (Order.ASCENDING == ord ? 1 : 2); - byte[] a = new byte[expectedLen + 2]; - PositionedByteRange buf1 = - new SimplePositionedByteRange(a, 1, expectedLen); + byte[] a = new byte[expectedLen + 3]; + PositionedByteRange buf1 = new SimplePositionedByteRange(a, 1, expectedLen + 1); + buf1.setPosition(1); // verify encode assertEquals("Surprising return value.", expectedLen, OrderedBytes.encodeBlobCopy(buf1, val, ord)); - assertEquals("Surprising serialized length.", expectedLen, buf1.getPosition()); + assertEquals("Broken test: serialization did not consume entire buffer.", + buf1.getLength(), buf1.getPosition()); + assertEquals("Surprising serialized length.", expectedLen, buf1.getPosition() - 1); assertEquals("Buffer underflow.", 0, a[0]); + assertEquals("Buffer underflow.", 0, a[1]); assertEquals("Buffer overflow.", 0, a[a.length - 1]); // verify skip - buf1.setPosition(0); + buf1.setPosition(1); assertEquals("Surprising return value.", expectedLen, OrderedBytes.skip(buf1)); - assertEquals("Did not skip enough bytes.", expectedLen, buf1.getPosition()); + assertEquals("Did not skip enough bytes.", expectedLen, buf1.getPosition() - 1); // verify decode - buf1.setPosition(0); + buf1.setPosition(1); assertArrayEquals("Deserialization failed.", val, OrderedBytes.decodeBlobCopy(buf1)); - assertEquals("Did not consume enough bytes.", expectedLen, buf1.getPosition()); + assertEquals("Did not consume enough bytes.", expectedLen, buf1.getPosition() - 1); } } -- 1.8.3.4