You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@thrift.apache.org by ns...@apache.org on 2015/11/04 15:24:51 UTC
thrift git commit: THRIFT-3182 TFramedTransport is in an invalid
state after frame size exception Client: Java Patch: Marshall Scorcio
Repository: thrift
Updated Branches:
refs/heads/master f0f607ffa -> fe5330955
THRIFT-3182 TFramedTransport is in an invalid state after frame size exception
Client: Java
Patch: Marshall Scorcio
This closes #512
Project: http://git-wip-us.apache.org/repos/asf/thrift/repo
Commit: http://git-wip-us.apache.org/repos/asf/thrift/commit/fe533095
Tree: http://git-wip-us.apache.org/repos/asf/thrift/tree/fe533095
Diff: http://git-wip-us.apache.org/repos/asf/thrift/diff/fe533095
Branch: refs/heads/master
Commit: fe5330955f6e52c63ed76819e4b36b9f263a9218
Parents: f0f607f
Author: Marshall Scorcio <ms...@swiftype.com>
Authored: Fri Jun 5 15:03:30 2015 -0700
Committer: Nobuaki Sukegawa <ns...@apache.org>
Committed: Wed Nov 4 23:22:53 2015 +0900
----------------------------------------------------------------------
.../thrift/transport/TFastFramedTransport.java | 13 ++++---
.../thrift/transport/TFramedTransport.java | 9 +++--
.../thrift/transport/TTransportException.java | 1 +
.../thrift/transport/ReadCountingTransport.java | 24 ++++++++++---
.../transport/TestTFastFramedTransport.java | 5 +++
.../thrift/transport/TestTFramedTransport.java | 38 ++++++++++++++++++++
6 files changed, 77 insertions(+), 13 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/thrift/blob/fe533095/lib/java/src/org/apache/thrift/transport/TFastFramedTransport.java
----------------------------------------------------------------------
diff --git a/lib/java/src/org/apache/thrift/transport/TFastFramedTransport.java b/lib/java/src/org/apache/thrift/transport/TFastFramedTransport.java
index e32b7db..0398ca7 100644
--- a/lib/java/src/org/apache/thrift/transport/TFastFramedTransport.java
+++ b/lib/java/src/org/apache/thrift/transport/TFastFramedTransport.java
@@ -19,12 +19,12 @@
package org.apache.thrift.transport;
/**
- * This transport is wire compatible with {@link TFramedTransport}, but makes
+ * This transport is wire compatible with {@link TFramedTransport}, but makes
* use of reusable, expanding read and write buffers in order to avoid
* allocating new byte[]s all the time. Since the buffers only expand, you
* should probably only use this transport if your messages are not too variably
* large, unless the persistent memory cost is not an issue.
- *
+ *
* This implementation is NOT threadsafe.
*/
public class TFastFramedTransport extends TTransport {
@@ -91,7 +91,7 @@ public class TFastFramedTransport extends TTransport {
}
/**
- *
+ *
* @param underlying Transport that real reads and writes will go through to.
* @param initialBufferCapacity The initial size of the read and write buffers.
* In practice, it's not critical to set this unless you know in advance that
@@ -141,11 +141,14 @@ public class TFastFramedTransport extends TTransport {
int size = TFramedTransport.decodeFrameSize(i32buf);
if (size < 0) {
- throw new TTransportException("Read a negative frame size (" + size + ")!");
+ close();
+ throw new TTransportException(TTransportException.CORRUPTED_DATA, "Read a negative frame size (" + size + ")!");
}
if (size > maxLength) {
- throw new TTransportException("Frame size (" + size + ") larger than max length (" + maxLength + ")!");
+ close();
+ throw new TTransportException(TTransportException.CORRUPTED_DATA,
+ "Frame size (" + size + ") larger than max length (" + maxLength + ")!");
}
readBuffer.fill(underlying, size);
http://git-wip-us.apache.org/repos/asf/thrift/blob/fe533095/lib/java/src/org/apache/thrift/transport/TFramedTransport.java
----------------------------------------------------------------------
diff --git a/lib/java/src/org/apache/thrift/transport/TFramedTransport.java b/lib/java/src/org/apache/thrift/transport/TFramedTransport.java
index c948aa4..f7d220c 100644
--- a/lib/java/src/org/apache/thrift/transport/TFramedTransport.java
+++ b/lib/java/src/org/apache/thrift/transport/TFramedTransport.java
@@ -130,11 +130,14 @@ public class TFramedTransport extends TTransport {
int size = decodeFrameSize(i32buf);
if (size < 0) {
- throw new TTransportException("Read a negative frame size (" + size + ")!");
+ close();
+ throw new TTransportException(TTransportException.CORRUPTED_DATA, "Read a negative frame size (" + size + ")!");
}
if (size > maxLength_) {
- throw new TTransportException("Frame size (" + size + ") larger than max length (" + maxLength_ + ")!");
+ close();
+ throw new TTransportException(TTransportException.CORRUPTED_DATA,
+ "Frame size (" + size + ") larger than max length (" + maxLength_ + ")!");
}
byte[] buff = new byte[size];
@@ -166,7 +169,7 @@ public class TFramedTransport extends TTransport {
}
public static final int decodeFrameSize(final byte[] buf) {
- return
+ return
((buf[0] & 0xff) << 24) |
((buf[1] & 0xff) << 16) |
((buf[2] & 0xff) << 8) |
http://git-wip-us.apache.org/repos/asf/thrift/blob/fe533095/lib/java/src/org/apache/thrift/transport/TTransportException.java
----------------------------------------------------------------------
diff --git a/lib/java/src/org/apache/thrift/transport/TTransportException.java b/lib/java/src/org/apache/thrift/transport/TTransportException.java
index d08f3b0..b886bc2 100644
--- a/lib/java/src/org/apache/thrift/transport/TTransportException.java
+++ b/lib/java/src/org/apache/thrift/transport/TTransportException.java
@@ -34,6 +34,7 @@ public class TTransportException extends TException {
public static final int ALREADY_OPEN = 2;
public static final int TIMED_OUT = 3;
public static final int END_OF_FILE = 4;
+ public static final int CORRUPTED_DATA = 5;
protected int type_ = UNKNOWN;
http://git-wip-us.apache.org/repos/asf/thrift/blob/fe533095/lib/java/test/org/apache/thrift/transport/ReadCountingTransport.java
----------------------------------------------------------------------
diff --git a/lib/java/test/org/apache/thrift/transport/ReadCountingTransport.java b/lib/java/test/org/apache/thrift/transport/ReadCountingTransport.java
index 8474188..3c749f9 100644
--- a/lib/java/test/org/apache/thrift/transport/ReadCountingTransport.java
+++ b/lib/java/test/org/apache/thrift/transport/ReadCountingTransport.java
@@ -22,26 +22,40 @@ package org.apache.thrift.transport;
public class ReadCountingTransport extends TTransport {
public int readCount = 0;
private TTransport trans;
+ private boolean open = true;
public ReadCountingTransport(TTransport underlying) {
trans = underlying;
}
@Override
- public void close() {}
+ public void close() {
+ open = false;
+ }
@Override
- public boolean isOpen() {return true;}
+ public boolean isOpen() {
+ return open;
+ }
@Override
- public void open() throws TTransportException {}
+ public void open() throws TTransportException {
+ open = true;
+ }
@Override
public int read(byte[] buf, int off, int len) throws TTransportException {
+ if (!isOpen()) {
+ throw new TTransportException(TTransportException.NOT_OPEN, "Transport is closed");
+ }
readCount++;
return trans.read(buf, off, len);
}
@Override
- public void write(byte[] buf, int off, int len) throws TTransportException {}
-}
\ No newline at end of file
+ public void write(byte[] buf, int off, int len) throws TTransportException {
+ if (!isOpen()) {
+ throw new TTransportException(TTransportException.NOT_OPEN, "Transport is closed");
+ }
+ }
+}
http://git-wip-us.apache.org/repos/asf/thrift/blob/fe533095/lib/java/test/org/apache/thrift/transport/TestTFastFramedTransport.java
----------------------------------------------------------------------
diff --git a/lib/java/test/org/apache/thrift/transport/TestTFastFramedTransport.java b/lib/java/test/org/apache/thrift/transport/TestTFastFramedTransport.java
index e024049..11fbdf4 100644
--- a/lib/java/test/org/apache/thrift/transport/TestTFastFramedTransport.java
+++ b/lib/java/test/org/apache/thrift/transport/TestTFastFramedTransport.java
@@ -23,4 +23,9 @@ public class TestTFastFramedTransport extends TestTFramedTransport {
protected TTransport getTransport(TTransport underlying) {
return new TFastFramedTransport(underlying, 50, 10 * 1024 * 1024);
}
+
+ @Override
+ protected TTransport getTransport(TTransport underlying, int maxLength) {
+ return new TFastFramedTransport(underlying, 50, maxLength);
+ }
}
http://git-wip-us.apache.org/repos/asf/thrift/blob/fe533095/lib/java/test/org/apache/thrift/transport/TestTFramedTransport.java
----------------------------------------------------------------------
diff --git a/lib/java/test/org/apache/thrift/transport/TestTFramedTransport.java b/lib/java/test/org/apache/thrift/transport/TestTFramedTransport.java
index 78f58ec..6cebd3c 100644
--- a/lib/java/test/org/apache/thrift/transport/TestTFramedTransport.java
+++ b/lib/java/test/org/apache/thrift/transport/TestTFramedTransport.java
@@ -34,6 +34,10 @@ public class TestTFramedTransport extends TestCase {
return new TFramedTransport(underlying);
}
+ protected TTransport getTransport(TTransport underlying, int maxLength) {
+ return new TFramedTransport(underlying, maxLength);
+ }
+
public static byte[] byteSequence(int start, int end) {
byte[] result = new byte[end-start+1];
for (int i = 0; i <= (end-start); i++) {
@@ -75,6 +79,40 @@ public class TestTFramedTransport extends TestCase {
assertEquals(4, countTrans.readCount);
}
+ public void testInvalidFrameSize() throws IOException, TTransportException {
+ int maxLength = 128;
+
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ DataOutputStream dos = new DataOutputStream(baos);
+ dos.writeInt(130);
+ dos.write(byteSequence(0, 129));
+
+ TMemoryBuffer membuf = new TMemoryBuffer(0);
+ membuf.write(baos.toByteArray());
+
+ ReadCountingTransport countTrans = new ReadCountingTransport(membuf);
+ TTransport trans = getTransport(countTrans, maxLength);
+
+ byte[] readBuf = new byte[10];
+ try {
+ trans.read(readBuf, 0, 4);
+ fail("Expected a TTransportException");
+ } catch (TTransportException e) {
+ // We expect this exception because the frame we're trying to read is larger than our max frame length
+ assertEquals(TTransportException.CORRUPTED_DATA, e.getType());
+ }
+
+ assertFalse(trans.isOpen());
+
+ try {
+ trans.read(readBuf, 0, 4);
+ fail("Expected a TTransportException");
+ } catch (TTransportException e) {
+ // This time we get an exception indicating the connection was closed
+ assertEquals(TTransportException.NOT_OPEN, e.getType());
+ }
+ }
+
public void testWrite() throws TTransportException, IOException {
ByteArrayOutputStream baos = new ByteArrayOutputStream();
WriteCountingTransport countingTrans = new WriteCountingTransport(new TIOStreamTransport(new BufferedOutputStream(baos)));