thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yuxuan Wang (Jira)" <j...@apache.org>
Subject [jira] [Updated] (THRIFT-5183) [Go] Read beyond current frame in THeaderTransport might block the protocol code
Date Wed, 22 Apr 2020 16:14:00 GMT

     [ https://issues.apache.org/jira/browse/THRIFT-5183?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Yuxuan Wang updated THRIFT-5183:
--------------------------------
    Description: 
This is a bug introduced in my initial implementation of go THeader in https://github.com/apache/thrift/commit/4d46c1124450eeb77d2a6adc7ea5fab304bfeb4a.

In THeaderTransport.Read implementation, after finished reading the current frame, we try
to read the next frame to fill the rest of the reading buffer: https://github.com/apache/thrift/blob/df2f5d2cf321f070a356872eea13dd3f68891043/lib/go/thrift/header_transport.go#L502-L506

This is actually a wrong behavior. At the end of frame, the other end is highly likely awaiting
for the response, and very unlikely to send anything for the next frame. So this behavior
could potentially cause a blocking read and eventually lead to timeout.

Currently the two supported wrapped protocol under THeaderProtocol are TBinaryProtocol and
TCompactProtocol, I checked the code and neither of them will actually use a more than enough
buffer for the Read call to the transport, so this bug shouldn't be causing any real issues
yet. But it's still a bug worth fixing.

I already have a fix ready, will send out the PR after this ticket is created.

Ironically, this bug is actually very similar to the bug in TFrameTransport that I fixed in
that commit :(

  was:
This is a bug introduced in my initial implementation of go THeader in https://github.com/apache/thrift/commit/4d46c1124450eeb77d2a6adc7ea5fab304bfeb4a.

In THeaderTransport.Read implementation, after finished reading the current frame, we try
to read the next frame to fill the rest of the reading buffer: https://github.com/apache/thrift/blob/df2f5d2cf321f070a356872eea13dd3f68891043/lib/go/thrift/header_transport.go#L502-L506

This is actually a wrong behavior. At the end of frame, the other end is highly likely awaiting
for the response, and very unlikely to send anything for the next frame. So this behavior
could potentially cause a blocking read an eventually lead to timeout.

Currently the two supported wrapped protocol under THeaderProtocol are TBinaryProtocol and
TCompactProtocol, I checked the code and neither of them will actually use a more than enough
buffer for the Read call to the transport, so this bug shouldn't be causing any real issues
yet. But it's still a bug worth fixing.

I already have a fix ready, will send out the PR after this ticket is created.

Ironically, this bug is actually very similar to the bug in TFrameTransport that I fixed in
that commit :(


> [Go] Read beyond current frame in THeaderTransport might block the protocol code
> --------------------------------------------------------------------------------
>
>                 Key: THRIFT-5183
>                 URL: https://issues.apache.org/jira/browse/THRIFT-5183
>             Project: Thrift
>          Issue Type: Improvement
>          Components: Go - Library
>    Affects Versions: 0.13.0
>            Reporter: Yuxuan Wang
>            Priority: Major
>
> This is a bug introduced in my initial implementation of go THeader in https://github.com/apache/thrift/commit/4d46c1124450eeb77d2a6adc7ea5fab304bfeb4a.
> In THeaderTransport.Read implementation, after finished reading the current frame, we
try to read the next frame to fill the rest of the reading buffer: https://github.com/apache/thrift/blob/df2f5d2cf321f070a356872eea13dd3f68891043/lib/go/thrift/header_transport.go#L502-L506
> This is actually a wrong behavior. At the end of frame, the other end is highly likely
awaiting for the response, and very unlikely to send anything for the next frame. So this
behavior could potentially cause a blocking read and eventually lead to timeout.
> Currently the two supported wrapped protocol under THeaderProtocol are TBinaryProtocol
and TCompactProtocol, I checked the code and neither of them will actually use a more than
enough buffer for the Read call to the transport, so this bug shouldn't be causing any real
issues yet. But it's still a bug worth fixing.
> I already have a fix ready, will send out the PR after this ticket is created.
> Ironically, this bug is actually very similar to the bug in TFrameTransport that I fixed
in that commit :(



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Mime
View raw message