ACCUMULO-1958 Make Range constructor safe The public six-argument Range constructor lacked a check for the stop key being before the start key. This change adds the check, plus a similar, new protected constructor without the check for use by constructors which do not need it. Checks are also included for construction from Thrift and population via readFields.
Signed-off-by: Eric Newton <eric.new...@gmail.com> Project: http://git-wip-us.apache.org/repos/asf/accumulo/repo Commit: http://git-wip-us.apache.org/repos/asf/accumulo/commit/cc68925e Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/cc68925e Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/cc68925e Branch: refs/heads/1.5.1-SNAPSHOT Commit: cc68925ec08cb0ff14f30118526fb486449baf84 Parents: ff29f08 Author: Bill Havanki <bhava...@cloudera.com> Authored: Fri Dec 6 10:43:34 2013 -0500 Committer: Eric Newton <eric.new...@gmail.com> Committed: Thu Dec 12 11:19:59 2013 -0500 ---------------------------------------------------------------------- .../org/apache/accumulo/core/data/Range.java | 58 +++++++++++++++++++- .../apache/accumulo/core/data/RangeTest.java | 58 ++++++++++++++++++++ 2 files changed, 113 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/accumulo/blob/cc68925e/src/core/src/main/java/org/apache/accumulo/core/data/Range.java ---------------------------------------------------------------------- diff --git a/src/core/src/main/java/org/apache/accumulo/core/data/Range.java b/src/core/src/main/java/org/apache/accumulo/core/data/Range.java index 7ef0dc5..a6dce11 100644 --- a/src/core/src/main/java/org/apache/accumulo/core/data/Range.java +++ b/src/core/src/main/java/org/apache/accumulo/core/data/Range.java @@ -18,6 +18,7 @@ package org.apache.accumulo.core.data; import java.io.DataInput; import java.io.DataOutput; +import java.io.InvalidObjectException; import java.io.IOException; import java.util.ArrayList; import java.util.Collection; @@ -170,10 +171,54 @@ public class Range implements WritableComparable<Range> { * Copies a range */ public Range(Range range) { - this(range.start, range.stop, range.startKeyInclusive, range.stopKeyInclusive, range.infiniteStartKey, range.infiniteStopKey); + this(range.start, range.startKeyInclusive, range.infiniteStartKey, range.stop, range.stopKeyInclusive, range.infiniteStopKey); } + /** + * Creates a range from start to stop. + * + * @param start + * set this to null when negative infinity is needed + * @param stop + * set this to null when infinity is needed + * @param startKeyInclusive + * determines if the ranges includes the start key + * @param stopKeyInclusive + * determines if the range includes the end key + * @param infiniteStartKey + * true if start key is negative infinity (null) + * @param infiniteStopKey + * true if stop key is positive infinity (null) + * @throws IllegalArgumentException if stop is before start, or infiniteStartKey is true but start is not null, or infiniteStopKey is true but stop is not + * null + */ public Range(Key start, Key stop, boolean startKeyInclusive, boolean stopKeyInclusive, boolean infiniteStartKey, boolean infiniteStopKey) { + this(start, startKeyInclusive, infiniteStartKey, stop, stopKeyInclusive, infiniteStopKey); + if (!infiniteStartKey && !infiniteStopKey && beforeStartKey(stop)) { + throw new IllegalArgumentException("Start key must be less than end key in range (" + start + ", " + stop + ")"); + } + } + + /** + * Creates a range from start to stop. Unlike the public six-argument method, + * this one does not assure that stop is after start, which helps performance + * in cases where that assurance is already in place. + * + * @param start + * set this to null when negative infinity is needed + * @param startKeyInclusive + * determines if the ranges includes the start key + * @param infiniteStartKey + * true if start key is negative infinity (null) + * @param stop + * set this to null when infinity is needed + * @param stopKeyInclusive + * determines if the range includes the end key + * @param infiniteStopKey + * true if stop key is positive infinity (null) + * @throws IllegalArgumentException if infiniteStartKey is true but start is not null, or infiniteStopKey is true but stop is not null + */ + protected Range(Key start, boolean startKeyInclusive, boolean infiniteStartKey, Key stop, boolean stopKeyInclusive, boolean infiniteStopKey) { if (infiniteStartKey && start != null) throw new IllegalArgumentException(); @@ -189,8 +234,11 @@ public class Range implements WritableComparable<Range> { } public Range(TRange trange) { - this(trange.start == null ? null : new Key(trange.start), trange.stop == null ? null : new Key(trange.stop), trange.startKeyInclusive, - trange.stopKeyInclusive, trange.infiniteStartKey, trange.infiniteStopKey); + this(trange.start == null ? null : new Key(trange.start), trange.startKeyInclusive, trange.infiniteStartKey, + trange.stop == null ? null : new Key(trange.stop), trange.stopKeyInclusive, trange.infiniteStopKey); + if (!infiniteStartKey && !infiniteStopKey && beforeStartKey(stop)) { + throw new IllegalArgumentException("Start key must be less than end key in range (" + start + ", " + stop + ")"); + } } /** @@ -566,6 +614,10 @@ public class Range implements WritableComparable<Range> { startKeyInclusive = in.readBoolean(); stopKeyInclusive = in.readBoolean(); + + if (!infiniteStartKey && !infiniteStopKey && beforeStartKey(stop)) { + throw new InvalidObjectException("Start key must be less than end key in range (" + start + ", " + stop + ")"); + } } public void write(DataOutput out) throws IOException { http://git-wip-us.apache.org/repos/asf/accumulo/blob/cc68925e/src/core/src/test/java/org/apache/accumulo/core/data/RangeTest.java ---------------------------------------------------------------------- diff --git a/src/core/src/test/java/org/apache/accumulo/core/data/RangeTest.java b/src/core/src/test/java/org/apache/accumulo/core/data/RangeTest.java index a8d91b0..68d9731 100644 --- a/src/core/src/test/java/org/apache/accumulo/core/data/RangeTest.java +++ b/src/core/src/test/java/org/apache/accumulo/core/data/RangeTest.java @@ -16,12 +16,18 @@ */ package org.apache.accumulo.core.data; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.DataInputStream; +import java.io.DataOutputStream; +import java.io.InvalidObjectException; import java.util.Arrays; import java.util.HashSet; import java.util.List; import junit.framework.TestCase; +import org.apache.accumulo.core.data.thrift.TRange; import org.apache.hadoop.io.Text; public class RangeTest extends TestCase { @@ -761,4 +767,56 @@ public class RangeTest extends TestCase { assertNull(Range.followingPrefix(makeText((byte) 0xff, (byte) 0xff))); assertEquals(Range.followingPrefix(makeText((byte) 0x07, (byte) 0xff)), new Text(makeText((byte) 0x08))); } + + public void testReadFields() throws Exception { + Range r = nr("nuts", "soup"); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + DataOutputStream dos = new DataOutputStream(baos); + r.write(dos); + dos.close(); + ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray()); + DataInputStream dis = new DataInputStream(bais); + Range r2 = new Range(); + r2.readFields(dis); + dis.close(); + + assertEquals(r, r2); + } + + public void testReadFields_Check() throws Exception { + Range r = new Range(new Key(new Text("soup")), true, false, new Key(new Text("nuts")), true, false); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + DataOutputStream dos = new DataOutputStream(baos); + r.write(dos); + dos.close(); + ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray()); + DataInputStream dis = new DataInputStream(bais); + Range r2 = new Range(); + try { + r2.readFields(dis); + fail("readFields allowed invalid range"); + } catch (InvalidObjectException exc) { + /* good! */ + } finally { + dis.close(); + } + } + + public void testThrift() { + Range r = nr("nuts", "soup"); + TRange tr = r.toThrift(); + Range r2 = new Range(tr); + assertEquals(r, r2); + } + + public void testThrift_Check() { + Range r = new Range(new Key(new Text("soup")), true, false, new Key(new Text("nuts")), true, false); + TRange tr = r.toThrift(); + try { + Range r2 = new Range(tr); + fail("Thrift constructor allowed invalid range"); + } catch (IllegalArgumentException exc) { + /* good! */ + } + } }
