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 <[email protected]> Authored: Fri Jun 5 15:03:30 2015 -0700 Committer: Nobuaki Sukegawa <[email protected]> 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)));
