[
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:
[email protected]
> 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)
