thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (THRIFT-4626) Communication crash when using binary/compact protocol and zlib transport
Date Sun, 02 Sep 2018 11:02:00 GMT

    [ https://issues.apache.org/jira/browse/THRIFT-4626?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16601177#comment-16601177
] 

ASF GitHub Bot commented on THRIFT-4626:
----------------------------------------

jeking3 closed pull request #1589: THRIFT-4626: Remove checking of remaining bytes in the
Go library.
URL: https://github.com/apache/thrift/pull/1589
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/lib/go/thrift/binary_protocol.go b/lib/go/thrift/binary_protocol.go
index de0f6a7a5c..1f90bf4351 100644
--- a/lib/go/thrift/binary_protocol.go
+++ b/lib/go/thrift/binary_protocol.go
@@ -448,9 +448,6 @@ func (p *TBinaryProtocol) ReadBinary() ([]byte, error) {
 	if size < 0 {
 		return nil, invalidDataLength
 	}
-	if uint64(size) > p.trans.RemainingBytes() {
-		return nil, invalidDataLength
-	}
 
 	isize := int(size)
 	buf := make([]byte, isize)
@@ -481,9 +478,6 @@ func (p *TBinaryProtocol) readStringBody(size int32) (value string, err
error) {
 	if size < 0 {
 		return "", nil
 	}
-	if uint64(size) > p.trans.RemainingBytes() {
-		return "", invalidDataLength
-	}
 
 	var (
 		buf bytes.Buffer
diff --git a/lib/go/thrift/compact_protocol.go b/lib/go/thrift/compact_protocol.go
index 66fbf5c335..1900d50c3b 100644
--- a/lib/go/thrift/compact_protocol.go
+++ b/lib/go/thrift/compact_protocol.go
@@ -562,9 +562,6 @@ func (p *TCompactProtocol) ReadString() (value string, err error) {
 	if length < 0 {
 		return "", invalidDataLength
 	}
-	if uint64(length) > p.trans.RemainingBytes() {
-		return "", invalidDataLength
-	}
 
 	if length == 0 {
 		return "", nil
@@ -591,9 +588,6 @@ func (p *TCompactProtocol) ReadBinary() (value []byte, err error) {
 	if length < 0 {
 		return nil, invalidDataLength
 	}
-	if uint64(length) > p.trans.RemainingBytes() {
-		return nil, invalidDataLength
-	}
 
 	buf := make([]byte, length)
 	_, e = io.ReadFull(p.trans, buf)
diff --git a/lib/go/thrift/compact_protocol_test.go b/lib/go/thrift/compact_protocol_test.go
index f940b4e15a..65f77f2c42 100644
--- a/lib/go/thrift/compact_protocol_test.go
+++ b/lib/go/thrift/compact_protocol_test.go
@@ -26,11 +26,18 @@ import (
 
 func TestReadWriteCompactProtocol(t *testing.T) {
 	ReadWriteProtocolTest(t, NewTCompactProtocolFactory())
+
 	transports := []TTransport{
 		NewTMemoryBuffer(),
 		NewStreamTransportRW(bytes.NewBuffer(make([]byte, 0, 16384))),
 		NewTFramedTransport(NewTMemoryBuffer()),
 	}
+
+	zlib0, _ := NewTZlibTransport(NewTMemoryBuffer(), 0)
+	zlib6, _ := NewTZlibTransport(NewTMemoryBuffer(), 6)
+	zlib9, _ := NewTZlibTransport(NewTFramedTransport(NewTMemoryBuffer()), 9)
+	transports = append(transports, zlib0, zlib6, zlib9)
+
 	for _, trans := range transports {
 		p := NewTCompactProtocol(trans)
 		ReadWriteBool(t, p, trans)
diff --git a/lib/go/thrift/protocol_test.go b/lib/go/thrift/protocol_test.go
index e9118da88f..944055c0b4 100644
--- a/lib/go/thrift/protocol_test.go
+++ b/lib/go/thrift/protocol_test.go
@@ -32,7 +32,6 @@ import (
 const PROTOCOL_BINARY_DATA_SIZE = 155
 
 var (
-	data           string // test data for writing
 	protocol_bdata []byte // test data for writing; same as data
 	BOOL_VALUES    []bool
 	BYTE_VALUES    []int8
@@ -48,7 +47,6 @@ func init() {
 	for i := 0; i < PROTOCOL_BINARY_DATA_SIZE; i++ {
 		protocol_bdata[i] = byte((i + 'a') % 255)
 	}
-	data = string(protocol_bdata)
 	BOOL_VALUES = []bool{false, true, false, false, true}
 	BYTE_VALUES = []int8{117, 0, 1, 32, 127, -128, -1}
 	INT16_VALUES = []int16{459, 0, 1, -1, -128, 127, 32767, -32768}
@@ -121,6 +119,9 @@ func ReadWriteProtocolTest(t *testing.T, protocolFactory TProtocolFactory)
{
 		NewTMemoryBufferTransportFactory(1024),
 		NewStreamTransportFactory(buf, buf, true),
 		NewTFramedTransportFactory(NewTMemoryBufferTransportFactory(1024)),
+		NewTZlibTransportFactoryWithFactory(0, NewTMemoryBufferTransportFactory(1024)),
+		NewTZlibTransportFactoryWithFactory(6, NewTMemoryBufferTransportFactory(1024)),
+		NewTZlibTransportFactoryWithFactory(9, NewTFramedTransportFactory(NewTMemoryBufferTransportFactory(1024))),
 		NewTHttpPostClientTransportFactory("http://" + addr.String()),
 	}
 	for _, tf := range transports {


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


> Communication crash when using binary/compact protocol and zlib transport
> -------------------------------------------------------------------------
>
>                 Key: THRIFT-4626
>                 URL: https://issues.apache.org/jira/browse/THRIFT-4626
>             Project: Thrift
>          Issue Type: Bug
>          Components: Go - Library
>    Affects Versions: 0.11.0
>         Environment: Ubuntu 18.04
> Go 1.10.1
>            Reporter: Vyacheslav Kulakov
>            Priority: Major
>             Fix For: 0.12.0
>
>
> Reading a request on the server side or reading a response on the client side always
fail with the "Invalid data length" error when using the binary/compact protocol and the zlib
transport, which wraps the framed transport.
> In my project, I use the following code on the server side (only for testing):
> {code:go}
> processor := flume.NewThriftSourceProtocolProcessor(protocol)
> serverTransport, _ := thrift.NewTServerSocketTimeout(address, timeout)
> protocolFactory := thrift.NewTBinaryProtocolFactoryDefault()
> transportFactory := thrift.NewTFramedTransportFactory(thrift.NewTTransportFactory())
> transportFactory = thrift.NewTZlibTransportFactoryWithFactory(level, transportFactory)
> server := thrift.NewTSimpleServer4(
> 	processor,
> 	serverTransport,
> 	transportFactory,
> 	protocolFactory,
> )
> {code}
> and following code on the client side:
> {code:go}
> factory := thrift.NewTBinaryProtocolFactoryDefault()
> transport := thrift.TTransport(thrift.NewTFramedTransport(socket))
> transport, err = thrift.NewTZlibTransport(transport, compressionLevel)
> if err != nil {
> 	return err
> }
> err = transport.Open()
> if err != nil {
> 	return err
> }
> client := flume.NewThriftSourceProtocolClient(
> 	thrift.NewTStandardClient(
> 		factory.GetProtocol(transport),
> 		factory.GetProtocol(transport),
> 	),
> )
> {code}
> When I send data from the client to the server I always get the "EOF" error on the client
and the "Invalid data length" error on the server. If I use the compact protocol the errors
stay at their places. 
> I investigated Go library code and found a reason of that errors: the protocol invoke
the RemainingBytes method on the zlib transport and it always return zero because all bytes
were already read from a frame and were stored in a decompressor buffer. But we can't access
to this buffer to obtain correct number of remaining bytes. So this combination of protocols
and transports can't work together at all.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message