[
https://issues.apache.org/jira/browse/THRIFT-6181?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jens Geyer updated THRIFT-6181:
-------------------------------
Description:
TAsyncMethodCall sizes the response frame from the four bytes the peer sends
ahead of it, with no check of any kind
(lib/java/src/main/java/org/apache/thrift/async/TAsyncMethodCall.java,
doReadingResponseSize):
{code}
if (sizeBuffer.remaining() == 0) {
state = State.READING_RESPONSE_BODY;
frameBuffer =
ByteBuffer.allocate(TFramedTransport.decodeFrameSize(sizeBufferArray));
}
{code}
decodeFrameSize returns a signed int, so the peer chooses any value from
-2147483648 to 2147483647, and ByteBuffer.allocate is handed it directly. A
negative one raises IllegalArgumentException, which is not a
TTransportException and reaches the callback as-is; a large positive one is
allocated, and ByteBuffer.allocate zeroes the array, so the memory is resident
rather than reserved.
This is the odd one out among the three places lib/java reads a frame size.
TFramedTransport.readFrame and TFastFramedTransport.readFrame both refuse a
negative size and one over getConfiguration().getMaxFrameSize().
AbstractNonblockingServer.FrameBuffer refuses a size <= 0, one over
trans_.getMaxFrameSize(), and additionally defers a frame that would push the
server past MAX_READ_BUFFER_BYTES. Only the async client takes the number as it
comes.
The fix applies the two checks the framed transports already apply, with the
same messages, so that the two ways of reading a framed response agree.
Compatibility, worth a release note: maxFrameSize defaults to 16384000, so an
async client now refuses a response frame larger than that where it previously
allocated it. Callers who expect larger responses raise it with
TNonblockingTransport.setMaxFrameSize, and the bound follows.
Tests are in
lib/java/src/test/java/org/apache/thrift/async/TestTAsyncMethodCallFrameSize.java.
They drive a real TAsyncMethodCall against a socket that answers with a frame
size and no body, and assert the buffer that was allocated rather than only
that the call failed -- a response body that never arrives ends the call either
way. Two of the three fail before the change: "allocated 65536 for a frame that
is over the maximum", and "a negative size reached ByteBuffer.allocate:
java.lang.IllegalArgumentException: capacity < 0: (-1 < 0)". The third reads a
frame of exactly the maximum end to end and passes either way.
was:
TAsyncMethodCall sizes the response frame from the four bytes the peer sends
ahead of it, with no check of any kind
(lib/java/src/main/java/org/apache/thrift/async/TAsyncMethodCall.java,
doReadingResponseSize):
if (sizeBuffer.remaining() == 0) {
state = State.READING_RESPONSE_BODY;
frameBuffer =
ByteBuffer.allocate(TFramedTransport.decodeFrameSize(sizeBufferArray));
}
decodeFrameSize returns a signed int, so the peer chooses any value from
-2147483648 to 2147483647, and ByteBuffer.allocate is handed it directly. A
negative one raises IllegalArgumentException, which is not a
TTransportException and reaches the callback as-is; a large positive one is
allocated, and ByteBuffer.allocate zeroes the array, so the memory is resident
rather than reserved.
This is the odd one out among the three places lib/java reads a frame size.
TFramedTransport.readFrame and TFastFramedTransport.readFrame both refuse a
negative size and one over getConfiguration().getMaxFrameSize().
AbstractNonblockingServer.FrameBuffer refuses a size <= 0, one over
trans_.getMaxFrameSize(), and additionally defers a frame that would push the
server past MAX_READ_BUFFER_BYTES. Only the async client takes the number as it
comes.
The fix applies the two checks the framed transports already apply, with the
same messages, so that the two ways of reading a framed response agree.
Compatibility, worth a release note: maxFrameSize defaults to 16384000, so an
async client now refuses a response frame larger than that where it previously
allocated it. Callers who expect larger responses raise it with
TNonblockingTransport.setMaxFrameSize, and the bound follows.
Tests are in
lib/java/src/test/java/org/apache/thrift/async/TestTAsyncMethodCallFrameSize.java.
They drive a real TAsyncMethodCall against a socket that answers with a frame
size and no body, and assert the buffer that was allocated rather than only
that the call failed -- a response body that never arrives ends the call either
way. Two of the three fail before the change: "allocated 65536 for a frame that
is over the maximum", and "a negative size reached ByteBuffer.allocate:
java.lang.IllegalArgumentException: capacity < 0: (-1 < 0)". The third reads a
frame of exactly the maximum end to end and passes either way.
> Bound the response frame size in the Java async client
> ------------------------------------------------------
>
> Key: THRIFT-6181
> URL: https://issues.apache.org/jira/browse/THRIFT-6181
> Project: Thrift
> Issue Type: Bug
> Components: Java - Library
> Reporter: Jens Geyer
> Priority: Major
> Fix For: 0.25.0
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> TAsyncMethodCall sizes the response frame from the four bytes the peer sends
> ahead of it, with no check of any kind
> (lib/java/src/main/java/org/apache/thrift/async/TAsyncMethodCall.java,
> doReadingResponseSize):
> {code}
> if (sizeBuffer.remaining() == 0) {
> state = State.READING_RESPONSE_BODY;
> frameBuffer =
> ByteBuffer.allocate(TFramedTransport.decodeFrameSize(sizeBufferArray));
> }
> {code}
> decodeFrameSize returns a signed int, so the peer chooses any value from
> -2147483648 to 2147483647, and ByteBuffer.allocate is handed it directly. A
> negative one raises IllegalArgumentException, which is not a
> TTransportException and reaches the callback as-is; a large positive one is
> allocated, and ByteBuffer.allocate zeroes the array, so the memory is
> resident rather than reserved.
> This is the odd one out among the three places lib/java reads a frame size.
> TFramedTransport.readFrame and TFastFramedTransport.readFrame both refuse a
> negative size and one over getConfiguration().getMaxFrameSize().
> AbstractNonblockingServer.FrameBuffer refuses a size <= 0, one over
> trans_.getMaxFrameSize(), and additionally defers a frame that would push the
> server past MAX_READ_BUFFER_BYTES. Only the async client takes the number as
> it comes.
> The fix applies the two checks the framed transports already apply, with the
> same messages, so that the two ways of reading a framed response agree.
> Compatibility, worth a release note: maxFrameSize defaults to 16384000, so an
> async client now refuses a response frame larger than that where it
> previously allocated it. Callers who expect larger responses raise it with
> TNonblockingTransport.setMaxFrameSize, and the bound follows.
> Tests are in
> lib/java/src/test/java/org/apache/thrift/async/TestTAsyncMethodCallFrameSize.java.
> They drive a real TAsyncMethodCall against a socket that answers with a
> frame size and no body, and assert the buffer that was allocated rather than
> only that the call failed -- a response body that never arrives ends the call
> either way. Two of the three fail before the change: "allocated 65536 for a
> frame that is over the maximum", and "a negative size reached
> ByteBuffer.allocate: java.lang.IllegalArgumentException: capacity < 0: (-1 <
> 0)". The third reads a frame of exactly the maximum end to end and passes
> either way.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
