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-4620) TZlibTransport.cpp doesn't ensure that there is enough space for the zlib flush marker in the buffer.
Date Sun, 16 Sep 2018 10:52:00 GMT

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

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

jeking3 closed pull request #1591: THRIFT-4620: Ensure enough space for for zlib flush marker
URL: https://github.com/apache/thrift/pull/1591
 
 
   

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/cpp/src/thrift/transport/TZlibTransport.cpp b/lib/cpp/src/thrift/transport/TZlibTransport.cpp
index fb5cc5da16..e426dc390c 100644
--- a/lib/cpp/src/thrift/transport/TZlibTransport.cpp
+++ b/lib/cpp/src/thrift/transport/TZlibTransport.cpp
@@ -255,6 +255,15 @@ void TZlibTransport::flush() {
     throw TTransportException(TTransportException::BAD_ARGS, "flush() called after finish()");
   }
 
+  flushToZlib(uwbuf_, uwpos_, Z_BLOCK);
+  uwpos_ = 0;
+
+  if(wstream_->avail_out < 6){
+    transport_->write(cwbuf_, cwbuf_size_ - wstream_->avail_out);
+    wstream_->next_out = cwbuf_;
+    wstream_->avail_out = cwbuf_size_;
+  }
+
   flushToTransport(Z_FULL_FLUSH);
 }
 
@@ -285,7 +294,7 @@ void TZlibTransport::flushToZlib(const uint8_t* buf, int len, int flush)
{
   wstream_->avail_in = len;
 
   while (true) {
-    if (flush == Z_NO_FLUSH && wstream_->avail_in == 0) {
+    if ((flush == Z_NO_FLUSH || flush == Z_BLOCK) && wstream_->avail_in == 0)
{
       break;
     }
 
diff --git a/test/cpp/CMakeLists.txt b/test/cpp/CMakeLists.txt
index cdd63dbf0d..95d2991f8b 100755
--- a/test/cpp/CMakeLists.txt
+++ b/test/cpp/CMakeLists.txt
@@ -28,6 +28,9 @@ include_directories(SYSTEM "${OPENSSL_INCLUDE_DIR}")
 find_package(Libevent REQUIRED)  # Libevent comes with CMake support from upstream
 include_directories(SYSTEM ${LIBEVENT_INCLUDE_DIRS})
 
+find_package(ZLIB REQUIRED)
+include_directories(SYSTEM ${ZLIB_INCLUDE_DIRS})
+
 #Make sure gen-cpp files can be included
 include_directories("${CMAKE_CURRENT_BINARY_DIR}")
 include_directories("${CMAKE_CURRENT_BINARY_DIR}/gen-cpp")
diff --git a/test/cpp/src/TestClient.cpp b/test/cpp/src/TestClient.cpp
index 87bb0283ab..54b43dba85 100644
--- a/test/cpp/src/TestClient.cpp
+++ b/test/cpp/src/TestClient.cpp
@@ -31,6 +31,7 @@
 #include <thrift/transport/TTransportUtils.h>
 #include <thrift/transport/TSocket.h>
 #include <thrift/transport/TSSLSocket.h>
+#include <thrift/transport/TZlibTransport.h>
 #include <thrift/async/TEvhttpClientChannel.h>
 #include <thrift/server/TNonblockingServer.h> // <event.h>
 
@@ -154,6 +155,7 @@ int main(int argc, char** argv) {
   int port = 9090;
   int numTests = 1;
   bool ssl = false;
+  bool zlib = false;
   string transport_type = "buffered";
   string protocol_type = "binary";
   string domain_socket = "";
@@ -179,12 +181,14 @@ int main(int argc, char** argv) {
           " (no connection with filesystem pathnames)")
       ("transport",
           boost::program_options::value<string>(&transport_type)->default_value(transport_type),
-          "Transport: buffered, framed, http, evhttp")
+          "Transport: buffered, framed, http, evhttp, zlib")
       ("protocol",
           boost::program_options::value<string>(&protocol_type)->default_value(protocol_type),
           "Protocol: binary, compact, header, json, multi, multic, multih, multij")
       ("ssl",
           "Encrypted Transport using SSL")
+      ("zlib",
+          "Wrap Transport with Zlib")
       ("testloops,n",
           boost::program_options::value<int>(&numTests)->default_value(numTests),
           "Number of Tests")
@@ -220,6 +224,8 @@ int main(int argc, char** argv) {
       } else if (transport_type == "framed") {
       } else if (transport_type == "http") {
       } else if (transport_type == "evhttp") {
+      } else if (transport_type == "zlib") {
+        // crosstest will pass zlib as a transport and as a flag right now..
       } else {
         throw invalid_argument("Unknown transport type " + transport_type);
       }
@@ -235,6 +241,10 @@ int main(int argc, char** argv) {
     ssl = true;
   }
 
+  if (vm.count("zlib")) {
+    zlib = true;
+  }
+
   if (vm.count("abstract-namespace")) {
     abstract_namespace = true;
   }
@@ -278,14 +288,15 @@ int main(int argc, char** argv) {
   }
 
   if (transport_type.compare("http") == 0) {
-    stdcxx::shared_ptr<TTransport> httpSocket(new THttpClient(socket, host, "/service"));
-    transport = httpSocket;
+    transport = stdcxx::make_shared<THttpClient>(socket, host, "/service");
   } else if (transport_type.compare("framed") == 0) {
-    stdcxx::shared_ptr<TFramedTransport> framedSocket(new TFramedTransport(socket));
-    transport = framedSocket;
+    transport = stdcxx::make_shared<TFramedTransport>(socket);
   } else {
-    stdcxx::shared_ptr<TBufferedTransport> bufferedSocket(new TBufferedTransport(socket));
-    transport = bufferedSocket;
+    transport = stdcxx::make_shared<TBufferedTransport>(socket);
+  }
+
+  if (zlib) {
+    transport = stdcxx::make_shared<TZlibTransport>(transport);
   }
 
   if (protocol_type == "json" || protocol_type == "multij") {
diff --git a/test/cpp/src/TestServer.cpp b/test/cpp/src/TestServer.cpp
index 1c38124102..323f873547 100644
--- a/test/cpp/src/TestServer.cpp
+++ b/test/cpp/src/TestServer.cpp
@@ -39,6 +39,7 @@
 #include <thrift/transport/TSSLSocket.h>
 #include <thrift/transport/TServerSocket.h>
 #include <thrift/transport/TTransportUtils.h>
+#include <thrift/transport/TZlibTransport.h>
 
 #include "SecondService.h"
 #include "ThriftTest.h"
@@ -571,6 +572,7 @@ int main(int argc, char** argv) {
 #endif
   int port = 9090;
   bool ssl = false;
+  bool zlib = false;
   string transport_type = "buffered";
   string protocol_type = "binary";
   string server_type = "simple";
@@ -587,9 +589,10 @@ int main(int argc, char** argv) {
     ("domain-socket", po::value<string>(&domain_socket) ->default_value(domain_socket),
"Unix Domain Socket (e.g. /tmp/ThriftTest.thrift)")
     ("abstract-namespace", "Create the domain socket in the Abstract Namespace (no connection
with filesystem pathnames)")
     ("server-type", po::value<string>(&server_type)->default_value(server_type),
"type of server, \"simple\", \"thread-pool\", \"threaded\", or \"nonblocking\"")
-    ("transport", po::value<string>(&transport_type)->default_value(transport_type),
"transport: buffered, framed, http")
+    ("transport", po::value<string>(&transport_type)->default_value(transport_type),
"transport: buffered, framed, http, zlib")
     ("protocol", po::value<string>(&protocol_type)->default_value(protocol_type),
"protocol: binary, compact, header, json, multi, multic, multih, multij")
     ("ssl", "Encrypted Transport using SSL")
+    ("zlib", "Wrapped Transport using Zlib")
     ("processor-events", "processor-events")
     ("workers,n", po::value<size_t>(&workers)->default_value(workers), "Number
of thread pools workers. Only valid for thread-pool server type")
     ("string-limit", po::value<int>(&string_limit))
@@ -633,6 +636,8 @@ int main(int argc, char** argv) {
       if (transport_type == "buffered") {
       } else if (transport_type == "framed") {
       } else if (transport_type == "http") {
+      } else if (transport_type == "zlib") {
+        // crosstester will pass zlib as a flag and a transport right now...
       } else {
         throw invalid_argument("Unknown transport type " + transport_type);
       }
@@ -648,6 +653,10 @@ int main(int argc, char** argv) {
     ssl = true;
   }
 
+  if (vm.count("zlib")) {
+    zlib = true;
+  }
+
 #if defined(HAVE_SIGNAL_H) && defined(SIGPIPE)
   if (ssl) {
     signal(SIGPIPE, SIG_IGN); // for OpenSSL, otherwise we end abruptly
@@ -719,14 +728,16 @@ int main(int argc, char** argv) {
   stdcxx::shared_ptr<TTransportFactory> transportFactory;
 
   if (transport_type == "http" && server_type != "nonblocking") {
-    stdcxx::shared_ptr<TTransportFactory> httpTransportFactory(new THttpServerTransportFactory());
-    transportFactory = httpTransportFactory;
+    transportFactory = stdcxx::make_shared<THttpServerTransportFactory>();
   } else if (transport_type == "framed") {
-    stdcxx::shared_ptr<TTransportFactory> framedTransportFactory(new TFramedTransportFactory());
-    transportFactory = framedTransportFactory;
+    transportFactory = stdcxx::make_shared<TFramedTransportFactory>();
   } else {
-    stdcxx::shared_ptr<TTransportFactory> bufferedTransportFactory(new TBufferedTransportFactory());
-    transportFactory = bufferedTransportFactory;
+    transportFactory = stdcxx::make_shared<TBufferedTransportFactory>();
+  }
+
+  if (zlib) {
+    // hmm.. doesn't seem to be a way to make it wrap the others...
+    transportFactory = stdcxx::make_shared<TZlibTransportFactory>();
   }
 
   // Server Info
diff --git a/test/crossrunner/test.py b/test/crossrunner/test.py
index 633e926169..0e912843ab 100644
--- a/test/crossrunner/test.py
+++ b/test/crossrunner/test.py
@@ -66,11 +66,19 @@ def _socket_args(self, socket, port):
             'abstract': ['--abstract-namespace', '--domain-socket=%s' % domain_socket_path(port)],
         }.get(socket, None)
 
+    def _transport_args(self, transport):
+        return {
+            'zlib': ['--zlib'],
+        }.get(transport, None)
+
     def build_command(self, port):
         cmd = copy.copy(self._base_command)
         args = copy.copy(self._extra_args2)
         args.append('--protocol=' + self.protocol)
         args.append('--transport=' + self.transport)
+        transport_args = self._transport_args(self.transport)
+        if transport_args:
+            args += transport_args
         socket_args = self._socket_args(self.socket, port)
         if socket_args:
             args += socket_args
diff --git a/test/go/src/bin/testclient/main.go b/test/go/src/bin/testclient/main.go
index 20104f9e12..4357ee83f9 100644
--- a/test/go/src/bin/testclient/main.go
+++ b/test/go/src/bin/testclient/main.go
@@ -35,6 +35,7 @@ var domain_socket = flag.String("domain-socket", "", "Domain Socket (e.g.
/tmp/t
 var transport = flag.String("transport", "buffered", "Transport: buffered, framed, http,
zlib")
 var protocol = flag.String("protocol", "binary", "Protocol: binary, compact, json")
 var ssl = flag.Bool("ssl", false, "Encrypted Transport using SSL")
+var zlib = flag.Bool("zlib", false, "Wrapped Transport using Zlib")
 var testloops = flag.Int("testloops", 1, "Number of Tests")
 
 func main() {
diff --git a/test/go/src/bin/testserver/main.go b/test/go/src/bin/testserver/main.go
index 0bf833d1d8..ca2d967b62 100644
--- a/test/go/src/bin/testserver/main.go
+++ b/test/go/src/bin/testserver/main.go
@@ -34,6 +34,7 @@ var domain_socket = flag.String("domain-socket", "", "Domain Socket (e.g.
/tmp/T
 var transport = flag.String("transport", "buffered", "Transport: buffered, framed, http,
zlib")
 var protocol = flag.String("protocol", "binary", "Protocol: binary, compact, json")
 var ssl = flag.Bool("ssl", false, "Encrypted Transport using SSL")
+var zlib = flag.Bool("zlib", false, "Wrapped Transport using Zlib")
 var certPath = flag.String("certPath", "keys", "Directory that contains SSL certificates")
 
 func main() {
diff --git a/test/known_failures_Linux.json b/test/known_failures_Linux.json
index 9d6d54bcf3..24ce997fb5 100644
--- a/test/known_failures_Linux.json
+++ b/test/known_failures_Linux.json
@@ -311,6 +311,10 @@
   "go-java_compact_http-ip-ssl",
   "go-java_json_http-ip",
   "go-java_json_http-ip-ssl",
+  "go-py3_binary-accel_zlib-ip-ssl",
+  "go-py3_compact-accelc_zlib-ip-ssl",
+  "go-py_binary-accel_zlib-ip-ssl",
+  "go-py_compact-accelc_zlib-ip-ssl",
   "hs-csharp_binary_buffered-ip",
   "hs-csharp_binary_framed-ip",
   "hs-csharp_compact_buffered-ip",
@@ -377,8 +381,12 @@
   "perl-rs_multi_framed-ip",
   "py-cpp_accel-binary_http-ip",
   "py-cpp_accel-binary_http-ip-ssl",
+  "py-cpp_accel-binary_zlib-ip",
+  "py-cpp_accel-binary_zlib-ip-ssl",
   "py-cpp_accelc-compact_http-ip",
   "py-cpp_accelc-compact_http-ip-ssl",
+  "py-cpp_accelc-compact_zlib-ip",
+  "py-cpp_accelc-compact_zlib-ip-ssl",
   "py-cpp_binary_http-ip",
   "py-cpp_binary_http-ip-ssl",
   "py-cpp_compact_http-ip",
@@ -425,8 +433,12 @@
   "py-lua_json_http-ip",
   "py3-cpp_accel-binary_http-ip",
   "py3-cpp_accel-binary_http-ip-ssl",
+  "py3-cpp_accel-binary_zlib-ip",
+  "py3-cpp_accel-binary_zlib-ip-ssl",
   "py3-cpp_accelc-compact_http-ip",
   "py3-cpp_accelc-compact_http-ip-ssl",
+  "py3-cpp_accelc-compact_zlib-ip",
+  "py3-cpp_accelc-compact_zlib-ip-ssl",
   "py3-cpp_binary_http-ip",
   "py3-cpp_binary_http-ip-ssl",
   "py3-cpp_compact_http-ip",
@@ -477,4 +489,4 @@
   "rb-cpp_json_framed-domain",
   "rb-cpp_json_framed-ip",
   "rb-cpp_json_framed-ip-ssl"
-]
+]
\ No newline at end of file
diff --git a/test/tests.json b/test/tests.json
index 85a0c0797a..27e75cc218 100644
--- a/test/tests.json
+++ b/test/tests.json
@@ -105,7 +105,8 @@
     "transports": [
       "buffered",
       "framed",
-      "http"
+      "http",
+      "zlib"
     ],
     "sockets": [
       "ip",
@@ -262,7 +263,8 @@
     "transports": [
       "buffered",
       "framed",
-      "http"
+      "http",
+      "zlib"
     ],
     "sockets": [
       "ip",
@@ -309,7 +311,8 @@
     "transports": [
       "buffered",
       "framed",
-      "http"
+      "http",
+      "zlib"
     ],
     "sockets": [
       "ip",
@@ -353,7 +356,8 @@
     "transports": [
       "buffered",
       "http",
-      "framed"
+      "framed",
+      "zlib"
     ],
     "sockets": [
       "ip",


 

----------------------------------------------------------------
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


> TZlibTransport.cpp doesn't ensure that there is enough space for the zlib flush marker
in the buffer.
> -----------------------------------------------------------------------------------------------------
>
>                 Key: THRIFT-4620
>                 URL: https://issues.apache.org/jira/browse/THRIFT-4620
>             Project: Thrift
>          Issue Type: Bug
>          Components: C++ - Library
>    Affects Versions: 0.9
>            Reporter: Dominic Coyne
>            Priority: Major
>              Labels: c++, zlib
>
> I asked [this question|https://stackoverflow.com/questions/51784225/how-does-thrift-handle-zlib-flush-markers-being-split-over-multiple-messages]
on stack overflow related to a crash that I have been getting with Thrift.
> The problem occurs when using TZlibTransport.cpp. After writing to the buffer a few times,
we do a flush. If there isn't enough space in cwbuf_ , the Thrift flush marker is split across
two messages, which causes an error in the client, as a deflate stream can't start with a
partial flush marker, ff.
> Thrift should assure that there will be enough space in the buffer for the complete flush
marker, before a deflate.
>  



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

Mime
View raw message