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! */
+    }
+  }
 }

Reply via email to