From 0a0cd345520382bd726fef50c62864715b03f164 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 7 May 2014 09:09:13 +0200 Subject: [PATCH 1/4] rpc: pass errors from async_accept According to the [boost::asio documentation](http://www.boost.org/doc/libs/1_55_0/doc/html/boost_asio/reference/basic_socket_acceptor/async_accept/overload2.html), the function signature of the handler must be: void handler( const boost::system::error_code& error // Result of operation. ); We were binding *all* the arguments, instead of all but the error, resulting in nullary function that never got the error. Fix this by adding an input argument substitution. --- src/rpcserver.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/rpcserver.cpp b/src/rpcserver.cpp index ac40ea7cf..44f329dd1 100644 --- a/src/rpcserver.cpp +++ b/src/rpcserver.cpp @@ -466,7 +466,7 @@ static void RPCListen(boost::shared_ptr< basic_socket_acceptor Date: Wed, 7 May 2014 09:24:44 +0200 Subject: [PATCH 2/4] rpc: Make sure conn object is always cleaned up Make sure conn object always gets cleaned up by using a `boost::shared_ptr`. This makes valgrind happy - before this commit, one connection object always leaked at shutdown, as well as can avoid other leaks, when for example an exception happens. Also add an explicit Close() to the !ClientAllowed path to make it similar to the normal path (I'm not sure whether it is needed, but it can't hurt). --- src/rpcserver.cpp | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/rpcserver.cpp b/src/rpcserver.cpp index 44f329dd1..442bf5c9c 100644 --- a/src/rpcserver.cpp +++ b/src/rpcserver.cpp @@ -444,7 +444,7 @@ template static void RPCAcceptHandler(boost::shared_ptr< basic_socket_acceptor > acceptor, ssl::context& context, bool fUseSSL, - AcceptedConnection* conn, + boost::shared_ptr< AcceptedConnection > conn, const boost::system::error_code& error); /** @@ -456,7 +456,7 @@ static void RPCListen(boost::shared_ptr< basic_socket_acceptor* conn = new AcceptedConnectionImpl(acceptor->get_io_service(), context, fUseSSL); + boost::shared_ptr< AcceptedConnectionImpl > conn(new AcceptedConnectionImpl(acceptor->get_io_service(), context, fUseSSL)); acceptor->async_accept( conn->sslStream.lowest_layer(), @@ -477,23 +477,20 @@ template static void RPCAcceptHandler(boost::shared_ptr< basic_socket_acceptor > acceptor, ssl::context& context, const bool fUseSSL, - AcceptedConnection* conn, + boost::shared_ptr< AcceptedConnection > conn, const boost::system::error_code& error) { // Immediately start accepting new connections, except when we're cancelled or our socket is closed. if (error != asio::error::operation_aborted && acceptor->is_open()) RPCListen(acceptor, context, fUseSSL); - AcceptedConnectionImpl* tcp_conn = dynamic_cast< AcceptedConnectionImpl* >(conn); + AcceptedConnectionImpl* tcp_conn = dynamic_cast< AcceptedConnectionImpl* >(conn.get()); - // TODO: Actually handle errors if (error) { - delete conn; // TODO: Actually handle errors LogPrintf("%s: Error: %s\n", __func__, error.message()); } - // Restrict callers by IP. It is important to // do this before starting client thread, to filter out // certain DoS and misbehaving clients. @@ -502,12 +499,11 @@ static void RPCAcceptHandler(boost::shared_ptr< basic_socket_acceptorstream() << HTTPReply(HTTP_FORBIDDEN, "", false) << std::flush; - delete conn; + conn->close(); } else { - ServiceConnection(conn); + ServiceConnection(conn.get()); conn->close(); - delete conn; } } From 381b25dfde013efae4e0360f82cd34e27662e743 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 7 May 2014 11:47:16 +0200 Subject: [PATCH 3/4] doc: remove mention of `-rpctimeout` from man page That option hasn't existed for a long time. --- contrib/debian/manpages/bitcoin.conf.5 | 3 --- 1 file changed, 3 deletions(-) diff --git a/contrib/debian/manpages/bitcoin.conf.5 b/contrib/debian/manpages/bitcoin.conf.5 index eef213149..7438b4b66 100644 --- a/contrib/debian/manpages/bitcoin.conf.5 +++ b/contrib/debian/manpages/bitcoin.conf.5 @@ -37,9 +37,6 @@ You must set *rpcuser* to secure the JSON-RPC api. \fBrpcpassword=\fR\fI'password'\fR You must set *rpcpassword* to secure the JSON-RPC api. .TP -\fBrpctimeout=\fR\fI'30'\fR -How many seconds *bitcoin* will wait for a complete RPC HTTP request, after the HTTP connection is established. -.TP \fBrpcallowip=\fR\fI'192.168.1.*'\fR By default, only RPC connections from localhost are allowed. Specify as many *rpcallowip=* settings as you like to allow connections from other hosts (and you may use * as a wildcard character). .TP From cef44941e798c33f7334bf90a2dd531f9bada8c3 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 9 May 2014 10:01:50 +0200 Subject: [PATCH 4/4] rpc: keep track of acceptors, and cancel them in StopRPCThreads Fixes #4156. The problem is that the boost::asio::io_service destructor waits for the acceptors to finish (on windows, and boost 1.55). Fix this by keeping track of the acceptors and cancelling them before stopping the event loops. --- src/rpcserver.cpp | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/rpcserver.cpp b/src/rpcserver.cpp index 442bf5c9c..d4a229b09 100644 --- a/src/rpcserver.cpp +++ b/src/rpcserver.cpp @@ -39,6 +39,7 @@ static ssl::context* rpc_ssl_context = NULL; static boost::thread_group* rpc_worker_group = NULL; static boost::asio::io_service::work *rpc_dummy_work = NULL; static std::vector rpc_allow_subnets; //!< List of subnets to allow RPC connections from +static std::vector< boost::shared_ptr > rpc_acceptors; void RPCTypeCheck(const Array& params, const list& typesExpected, @@ -593,12 +594,13 @@ void StartRPCThreads() asio::ip::address bindAddress = loopback ? asio::ip::address_v6::loopback() : asio::ip::address_v6::any(); ip::tcp::endpoint endpoint(bindAddress, GetArg("-rpcport", Params().RPCPort())); boost::system::error_code v6_only_error; - boost::shared_ptr acceptor(new ip::tcp::acceptor(*rpc_io_service)); bool fListening = false; std::string strerr; try { + boost::shared_ptr acceptor(new ip::tcp::acceptor(*rpc_io_service)); + rpc_acceptors.push_back(acceptor); acceptor->open(endpoint.protocol()); acceptor->set_option(boost::asio::ip::tcp::acceptor::reuse_address(true)); @@ -616,7 +618,6 @@ void StartRPCThreads() { strerr = strprintf(_("An error occurred while setting up the RPC port %u for listening on IPv6, falling back to IPv4: %s"), endpoint.port(), e.what()); } - try { // If dual IPv6/IPv4 failed (or we're opening loopback interfaces only), open IPv4 separately if (!fListening || loopback || v6_only_error) @@ -624,7 +625,8 @@ void StartRPCThreads() bindAddress = loopback ? asio::ip::address_v4::loopback() : asio::ip::address_v4::any(); endpoint.address(bindAddress); - acceptor.reset(new ip::tcp::acceptor(*rpc_io_service)); + boost::shared_ptr acceptor(new ip::tcp::acceptor(*rpc_io_service)); + rpc_acceptors.push_back(acceptor); acceptor->open(endpoint.protocol()); acceptor->set_option(boost::asio::ip::tcp::acceptor::reuse_address(true)); acceptor->bind(endpoint); @@ -668,7 +670,16 @@ void StopRPCThreads() { if (rpc_io_service == NULL) return; + // First, cancel all timers and acceptors + // This is not done automatically by ->stop(), and in some cases the destructor of + // asio::io_service can hang if this is skipped. + BOOST_FOREACH(const boost::shared_ptr &acceptor, rpc_acceptors) + acceptor->cancel(); + rpc_acceptors.clear(); + BOOST_FOREACH(const PAIRTYPE(std::string, boost::shared_ptr) &timer, deadlineTimers) + timer.second->cancel(); deadlineTimers.clear(); + rpc_io_service->stop(); if (rpc_worker_group != NULL) rpc_worker_group->join_all();