This is an automated email from the ASF dual-hosted git repository.
dcelasun pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/thrift.git
The following commit(s) were added to refs/heads/master by this push:
new e98ba9c THRIFT-5183: Don't try to read next frame in
THeaderTransport.Read
e98ba9c is described below
commit e98ba9cb841153fcbe2185ca44c77dbbc745ceb3
Author: Yuxuan 'fishy' Wang <[email protected]>
AuthorDate: Thu Apr 23 23:39:04 2020 -0700
THRIFT-5183: Don't try to read next frame in THeaderTransport.Read
Trying to read the next frame will likely cause the Read call blocking
indefinitely and eventually lead to timeout. See the JIRA ticket for
more context.
Client: go
---
lib/go/thrift/header_transport.go | 15 +++++++---
lib/go/thrift/header_transport_test.go | 51 ++++++++++++++++++++++++++++++++++
2 files changed, 62 insertions(+), 4 deletions(-)
diff --git a/lib/go/thrift/header_transport.go
b/lib/go/thrift/header_transport.go
index 5343ccb..103741e 100644
--- a/lib/go/thrift/header_transport.go
+++ b/lib/go/thrift/header_transport.go
@@ -499,10 +499,17 @@ func (t *THeaderTransport) Read(p []byte) (read int, err
error) {
if err != nil {
return
}
- if read < len(p) {
- var nextRead int
- nextRead, err = t.Read(p[read:])
- read += nextRead
+ if read == 0 {
+ // Try to read the next frame when we hit EOF
+ // (end of frame) immediately.
+ // When we got here, it means the last read
+ // finished the previous frame, but didn't
+ // do endOfFrame handling yet.
+ // We have to read the next frame here,
+ // as otherwise we would return 0 and nil,
+ // which is a case not handled well by most
+ // protocol implementations.
+ return t.Read(p)
}
}
return
diff --git a/lib/go/thrift/header_transport_test.go
b/lib/go/thrift/header_transport_test.go
index e304768..e3ae41b 100644
--- a/lib/go/thrift/header_transport_test.go
+++ b/lib/go/thrift/header_transport_test.go
@@ -23,7 +23,9 @@ import (
"context"
"io"
"io/ioutil"
+ "strings"
"testing"
+ "testing/quick"
)
func TestTHeaderHeadersReadWrite(t *testing.T) {
@@ -128,3 +130,52 @@ func TestTHeaderTransportNoDoubleWrapping(t *testing.T) {
t.Errorf("NewTHeaderTransport double wrapped THeaderTransport")
}
}
+
+func TestTHeaderTransportNoReadBeyondFrame(t *testing.T) {
+ trans := NewTMemoryBuffer()
+ writeContent := func(writer TTransport, content string) error {
+ if _, err := io.Copy(writer, strings.NewReader(content)); err
!= nil {
+ return err
+ }
+ if err := writer.Flush(context.Background()); err != nil {
+ return err
+ }
+ return nil
+ }
+ f := func(content string) bool {
+ defer trans.Reset()
+ if len(content) == 0 {
+ return true
+ }
+
+ reader := NewTHeaderTransport(trans)
+ writer := NewTHeaderTransport(trans)
+ // Write content twice
+ if err := writeContent(writer, content); err != nil {
+ t.Error(err)
+ }
+ if err := writeContent(writer, content); err != nil {
+ t.Error(err)
+ }
+ // buf is big enough to read both content out,
+ // but it shouldn't read beyond the first one in a single Read
call.
+ buf := make([]byte, len(content)*3)
+ read, err := reader.Read(buf)
+ if err != nil {
+ t.Error(err)
+ }
+ if read == 0 || read > len(content) {
+ t.Errorf(
+ "Expected read in no more than %d:%q, got
%d:%q",
+ len(content),
+ content,
+ read,
+ buf[:read],
+ )
+ }
+ return !t.Failed()
+ }
+ if err := quick.Check(f, nil); err != nil {
+ t.Error(err)
+ }
+}
