[
https://issues.apache.org/jira/browse/THRIFT-4326?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16160213#comment-16160213
]
ASF GitHub Bot commented on THRIFT-4326:
----------------------------------------
GitHub user benweint opened a pull request:
https://github.com/apache/thrift/pull/1352
THRIFT-4326 Allow safer reuse of BufferedTransport instances in Ruby
Addresses [THRIFT-4326](https://issues.apache.org/jira/browse/THRIFT-4326).
## The problem
In the case where a single `Thrift::BufferedTransport` instance is re-used
across multiple service calls, certain kinds of malformed responses from one
service call can 'leak' into subsequent calls and cause them to fail with a
`Thrift::ProtocolException`.
The most easily reproducible example is when a service returns a
well-formed Thrift response for the first service call, but with N extra bytes
of garbage tacked onto the end.
In such a case, the initial service call will be handled just fine (at
least when using the compact protocol), however, the next N service calls that
go through the same `Thrift::BufferedTransport` instance will fail with a
`Thrift::ProtocolException`. This happens because the `BufferedTransport`
doesn't re-set the `@rbuf` instance variable until the read buffer is fully
exhausted, so each of the N subsequent service calls will attempt to read one
byte identifying the protocol from the remaining buffer, and will get some
bogus value from the garbage bytes at the end of the response from the initial
service call.
This can also happen if reading from `@rbuf` is interrupted part-way
through, while `@index` is still pointing to the middle of the read buffer
(e.g. due to a Ruby timeout exception).
## Proposed solution
Re-setting the `@rbuf` instance variable to an empty byte buffer upon every
call to `#flush` addresses this problem, and is conceptually similar to what
happens in `HttpClientTransport#flush` (where `@inbuf` and `@outbuf` are both
always reset).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/benweint/thrift THRIFT-4326
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1352.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1352
----
commit 79936408af223c5df3fe8a52ec65873ccc32a255
Author: Ben Weintraub <b...@newrelic.com>
Date: 2017-09-10T04:17:42Z
THRIFT-4326 Ensure rbuf in BufferedTransport reset upon flush to allow reuse
----
> Ruby BufferedTransport not safe for reuse after reading corrupted input
> -----------------------------------------------------------------------
>
> Key: THRIFT-4326
> URL: https://issues.apache.org/jira/browse/THRIFT-4326
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Library
> Affects Versions: 0.10.0
> Environment: Originally observed with Thrift 0.9.3 on Linux with Ruby
> 2.3.4, but have also reproduced on Mac OS X with Thrift 0.10.0.
> Reporter: Ben Weintraub
>
> We've experimented with the Ruby {{BufferedTransport}} class as a wrapper
> around the {{HttpClientTransport}} class, and found that we were getting
> clusters sporadic {{Thrift::ProtocolException}} errors in Ruby client
> processes after network issues caused corruption of some Thrift response
> bodies.
> Using a bare {{HttpClientTransport}} makes these issues disappear.
> For a given service, we retain a long-lived protocol instance
> ({{CompactProtocol}} in our case), which in turn holds a reference to a
> long-lived {{BufferedTransport}} instance.
> The problem seems to stem from the case where the Thrift client is
> interrupted (e.g. by a Ruby timeout exception) before consuming to the end of
> the {{@rbuf}} instance variable in {{BufferedTransport}}, leaving {{@index}}
> pointing to the middle of the read buffer, and meaning that when the
> transport is re-used upon the next service call, the {{BufferedTransport}}
> continues reading where it left off in the old buffer, rather than calling
> through to the wrapped {{HttpClientTransport}} to read the new response
> obtained from the last call to {{#flush}}.
> Now I know {{Timeout}} is fundamentally unsafe in Ruby and can lead to all
> kinds of issues like this, but I've also found that this same issue can be
> triggered by another fairly plausible scenario: if the Thrift service returns
> a well-formed Thrift response but with N extra bytes of garbage tacked onto
> the end, then the next N following service calls through the same
> {{BufferedTransport}} instance will fail with a
> {{Thrift::ProtocolException}}, as the {{BufferedTransport}} will continue
> attempting to read the left-over bytes in {{@rbuf}}.
> The naive solution seems like it would be to just reset {{@rbuf}} from
> {{#flush}}.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
