From 91cfb9901ebd6d584b0055bff5f91c372875a276 Mon Sep 17 00:00:00 2001 From: Jens Geyer Date: Sat, 17 May 2014 01:07:28 +0200 Subject: [PATCH] THRIFT-2500 sending random data crashes thrift(golang) service Client: Go Patch: Aleksey Pesternikov This closes #117 commit 1bb25c4a48845e112847ca8293402f0294d8f597 Author: Aleksey Pesternikov Date: 2014-05-02T21:40:59Z recover from panic in processor commit 8d1427a2c3c183d499442dc1f0437292e6641ac3 Author: Aleksey Pesternikov Date: 2014-05-02T21:41:52Z some sanity checks in binary protocol commit 666cc87a51f86ca5940225c36716bbad467c6e73 Author: Aleksey Pesternikov Date: 2014-05-02T21:53:59Z some sanity checks in compact protocol --- lib/go/thrift/binary_protocol.go | 33 ++++++++++++++++++++++++++---- lib/go/thrift/compact_protocol.go | 20 ++++++++++++++++-- lib/go/thrift/simple_server.go | 11 ++++++++-- test/go/src/common/mock_handler.go | 2 +- 4 files changed, 57 insertions(+), 9 deletions(-) diff --git a/lib/go/thrift/binary_protocol.go b/lib/go/thrift/binary_protocol.go index abbe0bc65..09f94d4c7 100644 --- a/lib/go/thrift/binary_protocol.go +++ b/lib/go/thrift/binary_protocol.go @@ -21,6 +21,7 @@ package thrift import ( "encoding/binary" + "errors" "fmt" "io" "math" @@ -301,6 +302,8 @@ func (p *TBinaryProtocol) ReadFieldEnd() error { return nil } +var invalidDataLength = NewTProtocolExceptionWithType(INVALID_DATA, errors.New("Invalid data length")) + func (p *TBinaryProtocol) ReadMapBegin() (kType, vType TType, size int, err error) { k, e := p.ReadByte() if e != nil { @@ -315,11 +318,15 @@ func (p *TBinaryProtocol) ReadMapBegin() (kType, vType TType, size int, err erro } vType = TType(v) size32, e := p.ReadI32() - size = int(size32) if e != nil { err = NewTProtocolException(e) return } + if size32 < 0 { + err = invalidDataLength + return + } + size = int(size32) return kType, vType, size, nil } @@ -335,12 +342,17 @@ func (p *TBinaryProtocol) ReadListBegin() (elemType TType, size int, err error) } elemType = TType(b) size32, e := p.ReadI32() - size = int(size32) if e != nil { err = NewTProtocolException(e) return } - return elemType, size, nil + if size32 < 0 { + err = invalidDataLength + return + } + size = int(size32) + + return } func (p *TBinaryProtocol) ReadListEnd() error { @@ -355,11 +367,15 @@ func (p *TBinaryProtocol) ReadSetBegin() (elemType TType, size int, err error) { } elemType = TType(b) size32, e := p.ReadI32() - size = int(size32) if e != nil { err = NewTProtocolException(e) return } + if size32 < 0 { + err = invalidDataLength + return + } + size = int(size32) return elemType, size, nil } @@ -413,6 +429,11 @@ func (p *TBinaryProtocol) ReadString() (value string, err error) { if e != nil { return "", e } + if size < 0 { + err = invalidDataLength + return + } + return p.readStringBody(int(size)) } @@ -421,6 +442,10 @@ func (p *TBinaryProtocol) ReadBinary() ([]byte, error) { if e != nil { return nil, e } + if size < 0 { + return nil, invalidDataLength + } + isize := int(size) buf := make([]byte, isize) _, err := io.ReadFull(p.trans, buf) diff --git a/lib/go/thrift/compact_protocol.go b/lib/go/thrift/compact_protocol.go index 14bf62d20..c275cf438 100644 --- a/lib/go/thrift/compact_protocol.go +++ b/lib/go/thrift/compact_protocol.go @@ -421,11 +421,16 @@ func (p *TCompactProtocol) ReadFieldEnd() error { return nil } // "correct" types. func (p *TCompactProtocol) ReadMapBegin() (keyType TType, valueType TType, size int, err error) { size32, e := p.readVarint32() - size = int(size32) if e != nil { err = NewTProtocolException(e) return } + if size32 < 0 { + err = invalidDataLength + return + } + size = int(size32) + keyAndValueType := byte(STOP) if size != 0 { keyAndValueType, err = p.ReadByte() @@ -456,6 +461,10 @@ func (p *TCompactProtocol) ReadListBegin() (elemType TType, size int, err error) err = NewTProtocolException(e) return } + if size2 < 0 { + err = invalidDataLength + return + } size = int(size2) } elemType, e := p.getTType(tCompactType(size_and_type)) @@ -541,6 +550,10 @@ func (p *TCompactProtocol) ReadString() (value string, err error) { if e != nil { return "", NewTProtocolException(e) } + if length < 0 { + return "", invalidDataLength + } + if length == 0 { return "", nil } @@ -561,7 +574,10 @@ func (p *TCompactProtocol) ReadBinary() (value []byte, err error) { return nil, NewTProtocolException(e) } if length == 0 { - return nil, nil //nil == empty slice + return []byte{}, nil + } + if length < 0 { + return nil, invalidDataLength } buf := make([]byte, length) diff --git a/lib/go/thrift/simple_server.go b/lib/go/thrift/simple_server.go index eb8eb9538..9a2721561 100644 --- a/lib/go/thrift/simple_server.go +++ b/lib/go/thrift/simple_server.go @@ -21,6 +21,7 @@ package thrift import ( "log" + "runtime/debug" ) // Simple, non-concurrent server for testing. @@ -131,7 +132,7 @@ func (p *TSimpleServer) AcceptLoop() error { } if client != nil { go func() { - if err := p.processRequest(client); err != nil { + if err := p.processRequests(client); err != nil { log.Println("error processing request:", err) } }() @@ -154,12 +155,17 @@ func (p *TSimpleServer) Stop() error { return nil } -func (p *TSimpleServer) processRequest(client TTransport) error { +func (p *TSimpleServer) processRequests(client TTransport) error { processor := p.processorFactory.GetProcessor(client) inputTransport := p.inputTransportFactory.GetTransport(client) outputTransport := p.outputTransportFactory.GetTransport(client) inputProtocol := p.inputProtocolFactory.GetProtocol(inputTransport) outputProtocol := p.outputProtocolFactory.GetProtocol(outputTransport) + defer func() { + if e := recover(); e != nil { + log.Printf("panic in processor: %s: %s", e, debug.Stack()) + } + }() if inputTransport != nil { defer inputTransport.Close() } @@ -171,6 +177,7 @@ func (p *TSimpleServer) processRequest(client TTransport) error { if err, ok := err.(TTransportException); ok && err.TypeId() == END_OF_FILE { return nil } else if err != nil { + log.Printf("error processing request: %s", err) return err } if !ok { diff --git a/test/go/src/common/mock_handler.go b/test/go/src/common/mock_handler.go index 0aed38b20..d736ed965 100644 --- a/test/go/src/common/mock_handler.go +++ b/test/go/src/common/mock_handler.go @@ -23,8 +23,8 @@ package common import ( - thrifttest "gen/thrifttest" gomock "code.google.com/p/gomock/gomock" + thrifttest "gen/thrifttest" ) // Mock of ThriftTest interface