THRIFT-3568 THeader server crashes on readSlow

Client: C++
Patch: Nobuaki Sukegawa

This closes #807

3 files changed, 12 insertions, 35 deletions

diff --git a/lib/cpp/src/thrift/transport/TBufferTransports.h b/lib/cpp/src/thrift/transport/TBufferTransports.h
index 013c6e006..e690d0c6e 100644
--- a/lib/cpp/src/thrift/transport/TBufferTransports.h
+++ b/lib/cpp/src/thrift/transport/TBufferTransports.h
@@ -372,7 +372,7 @@ public:
    * TVirtualTransport provides a default implementation of readAll().
    * We want to use the TBufferBase version instead.
    */
-  uint32_t readAll(uint8_t* buf, uint32_t len) { return TBufferBase::readAll(buf, len); }
+  using TBufferBase::readAll;
 
   /**
    * Returns the origin of the underlying transport
@@ -396,7 +396,7 @@ protected:
    * Returns true if a frame was read successfully, or false on EOF.
    * (Raises a TTransportException if EOF occurs after a partial frame.)
    */
-  bool readFrame();
+  virtual bool readFrame();
 
   void initPointers() {
     setReadBuffer(NULL, 0);
diff --git a/lib/cpp/src/thrift/transport/THeaderTransport.cpp b/lib/cpp/src/thrift/transport/THeaderTransport.cpp
index 79bc5ead7..fd24fedb3 100644
--- a/lib/cpp/src/thrift/transport/THeaderTransport.cpp
+++ b/lib/cpp/src/thrift/transport/THeaderTransport.cpp
@@ -40,14 +40,7 @@ namespace transport {
 using namespace apache::thrift::protocol;
 using apache::thrift::protocol::TBinaryProtocol;
 
-uint32_t THeaderTransport::readAll(uint8_t* buf, uint32_t len) {
-  // We want to call TBufferBase's version here, because
-  // TFramedTransport would try and call its own readFrame function
-  return TBufferBase::readAll(buf, len);
-}
-
 uint32_t THeaderTransport::readSlow(uint8_t* buf, uint32_t len) {
-
   if (clientType == THRIFT_UNFRAMED_DEPRECATED) {
     return transport_->read(buf, len);
   }
@@ -70,7 +63,7 @@ void THeaderTransport::ensureReadBuffer(uint32_t sz) {
   }
 }
 
-bool THeaderTransport::readFrame(uint32_t minFrameSize) {
+bool THeaderTransport::readFrame() {
   // szN is network byte order of sz
   uint32_t szN;
   uint32_t sz;
@@ -99,6 +92,7 @@ bool THeaderTransport::readFrame(uint32_t minFrameSize) {
 
   sz = ntohl(szN);
 
+  uint32_t minFrameSize = 0;
   ensureReadBuffer(minFrameSize + 4);
 
   if ((sz & TBinaryProtocol::VERSION_MASK) == (uint32_t)TBinaryProtocol::VERSION_1) {
@@ -368,7 +362,7 @@ void THeaderTransport::resetProtocol() {
   clientType = THRIFT_HEADER_CLIENT_TYPE;
 
   // Read the header and decide which protocol to go with
-  readFrame(0);
+  readFrame();
 }
 
 uint32_t THeaderTransport::getWriteBytes() {
diff --git a/lib/cpp/src/thrift/transport/THeaderTransport.h b/lib/cpp/src/thrift/transport/THeaderTransport.h
index 94135ea5c..a1256326e 100644
--- a/lib/cpp/src/thrift/transport/THeaderTransport.h
+++ b/lib/cpp/src/thrift/transport/THeaderTransport.h
@@ -22,6 +22,7 @@
 
 #include <bitset>
 #include <vector>
+#include <stdexcept>
 #include <string>
 #include <map>
 
@@ -69,7 +70,7 @@ public:
 
   /// Use default buffer sizes.
   explicit THeaderTransport(const boost::shared_ptr<TTransport>& transport)
-    : transport_(transport),
+    : TVirtualTransport(transport),
       outTransport_(transport),
       protoId(T_COMPACT_PROTOCOL),
       clientType(THRIFT_HEADER_CLIENT_TYPE),
@@ -77,12 +78,13 @@ public:
       flags(0),
       tBufSize_(0),
       tBuf_(NULL) {
+    if (!transport_) throw std::invalid_argument("transport is empty");
     initBuffers();
   }
 
   THeaderTransport(const boost::shared_ptr<TTransport> inTransport,
                    const boost::shared_ptr<TTransport> outTransport)
-    : transport_(inTransport),
+    : TVirtualTransport(inTransport),
       outTransport_(outTransport),
       protoId(T_COMPACT_PROTOCOL),
       clientType(THRIFT_HEADER_CLIENT_TYPE),
@@ -90,34 +92,16 @@ public:
       flags(0),
       tBufSize_(0),
       tBuf_(NULL) {
+    if (!transport_) throw std::invalid_argument("inTransport is empty");
+    if (!outTransport_) throw std::invalid_argument("outTransport is empty");
     initBuffers();
   }
 
-  void open() { transport_->open(); }
-
-  bool isOpen() { return transport_->isOpen(); }
-
-  bool peek() { return (this->rBase_ < this->rBound_) || transport_->peek(); }
-
-  void close() {
-    flush();
-    transport_->close();
-  }
-
   virtual uint32_t readSlow(uint8_t* buf, uint32_t len);
-  virtual uint32_t readAll(uint8_t* buf, uint32_t len);
   virtual void flush();
 
   void resizeTransformBuffer(uint32_t additionalSize = 0);
 
-  boost::shared_ptr<TTransport> getUnderlyingTransport() { return transport_; }
-
-  /*
-   * TVirtualTransport provides a default implementation of readAll().
-   * We want to use the TBufferBase version instead.
-   */
-  using TBufferBase::readAll;
-
   uint16_t getProtocolId() const;
   void setProtocolId(uint16_t protoId) { this->protoId = protoId; }
 
@@ -191,7 +175,7 @@ protected:
    * Returns true if a frame was read successfully, or false on EOF.
    * (Raises a TTransportException if EOF occurs after a partial frame.)
    */
-  bool readFrame(uint32_t minFrameSize);
+  virtual bool readFrame();
 
   void ensureReadBuffer(uint32_t sz);
   uint32_t getWriteBytes();
@@ -201,7 +185,6 @@ protected:
     setWriteBuffer(wBuf_.get(), wBufSize_);
   }
 
-  boost::shared_ptr<TTransport> transport_;
   boost::shared_ptr<TTransport> outTransport_;
 
   // 0 and 16th bits must be 0 to differentiate from framed & unframed