Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-5183

Read beyond current frame in THeaderTransport might block the protocol code

VotersWatch issueWatchersLinkCloneUpdate Comment AuthorReplace String in CommentUpdate Comment VisibilityDelete Comments
    XMLWordPrintableJSON

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.13.0
    • Fix Version/s: None
    • Component/s: Go - Library
    • Labels:
      None

      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

        Attachments

          Activity

            People

            • Assignee:
              fishywang Yuxuan Wang
              Reporter:
              fishywang Yuxuan Wang

              Dates

              • Created:
                Updated:
                Resolved:

                Issue deployment