diff options
| -rw-r--r-- | contrib/epee/include/net/abstract_tcp_server2.h | 17 | ||||
| -rw-r--r-- | contrib/epee/include/net/abstract_tcp_server2.inl | 65 | ||||
| -rw-r--r-- | contrib/epee/src/parserse_base_utils.cpp | 2 | ||||
| -rwxr-xr-x | contrib/gitian/dockrun.sh | 16 | ||||
| -rw-r--r-- | src/daemonizer/posix_fork.cpp | 91 | ||||
| -rw-r--r-- | src/net/i2p_address.cpp | 12 | ||||
| -rw-r--r-- | src/net/tor_address.cpp | 14 | ||||
| -rw-r--r-- | src/p2p/net_node.h | 1 | ||||
| -rw-r--r-- | src/p2p/net_node.inl | 48 | ||||
| -rw-r--r-- | src/wallet/wallet2.cpp | 1 | ||||
| -rw-r--r-- | src/wallet/wallet_rpc_server.cpp | 6 | ||||
| -rw-r--r-- | tests/unit_tests/epee_boosted_tcp_server.cpp | 11 | ||||
| -rw-r--r-- | tests/unit_tests/net.cpp | 28 |
13 files changed, 231 insertions, 81 deletions
diff --git a/contrib/epee/include/net/abstract_tcp_server2.h b/contrib/epee/include/net/abstract_tcp_server2.h index fe201f155..75a26cbbb 100644 --- a/contrib/epee/include/net/abstract_tcp_server2.h +++ b/contrib/epee/include/net/abstract_tcp_server2.h @@ -320,7 +320,7 @@ namespace net_utils bool speed_limit_is_enabled() const; ///< tells us should we be sleeping here (e.g. do not sleep on RPC connections) - bool cancel(); + bool cancel(bool wait_for_shutdown = false); private: //----------------- i_service_endpoint --------------------- @@ -378,8 +378,21 @@ namespace net_utils /// wait for service workers stop bool timed_wait_server_stop(uint64_t wait_mseconds); + /// Mark the server as stopping without closing connections or stopping the io_context. + bool mark_stop_signal_sent(); + + /// Close boosted_tcp_server-owned connections, including ones not yet registered with the protocol handler. + void close_server_connections(); + + /// Stop the server io_context. + void stop_io_context(); + /// Stop the server. - void send_stop_signal(std::function<void()> close_all_connections = [](){}); + /// + /// Warning: Do NOT call this if the io_context is shared for connections + /// managed outside the boosted_tcp_server. See p2p net_node shutdown for + /// the correct staged shutdown in that case. + void send_stop_signal(); bool is_stop_signal_sent() const noexcept { return m_stop_signal_sent; }; diff --git a/contrib/epee/include/net/abstract_tcp_server2.inl b/contrib/epee/include/net/abstract_tcp_server2.inl index 56d3d7de2..f8651fae9 100644 --- a/contrib/epee/include/net/abstract_tcp_server2.inl +++ b/contrib/epee/include/net/abstract_tcp_server2.inl @@ -1119,9 +1119,9 @@ namespace net_utils } template<typename T> - bool connection<T>::cancel() + bool connection<T>::cancel(const bool wait_for_shutdown) { - return close(false); + return close(wait_for_shutdown); } template<typename T> @@ -1140,7 +1140,9 @@ namespace net_utils bool connection<T>::close(const bool wait_for_shutdown) { std::lock_guard<std::mutex> guard(m_state.lock); - if (m_state.status != status_t::RUNNING) + if (m_state.status == status_t::TERMINATED || m_state.status == status_t::WASTED) + return true; + if (!wait_for_shutdown && m_state.status != status_t::RUNNING) return false; terminate_async(); @@ -1286,7 +1288,7 @@ namespace net_utils template<class t_protocol_handler> boosted_tcp_server<t_protocol_handler>::~boosted_tcp_server() { - this->send_stop_signal(); + send_stop_signal(); timed_wait_server_stop(10000); } //--------------------------------------------------------------------------------- @@ -1577,26 +1579,55 @@ namespace net_utils } //--------------------------------------------------------------------------------- template<class t_protocol_handler> - void boosted_tcp_server<t_protocol_handler>::send_stop_signal(std::function<void()> close_all_connections) + bool boosted_tcp_server<t_protocol_handler>::mark_stop_signal_sent() { - m_stop_signal_sent = true; + if (m_stop_signal_sent.exchange(true)) + { + MDEBUG("Stop signal already sent"); + return false; + } typename connection<t_protocol_handler>::shared_state *state = static_cast<typename connection<t_protocol_handler>::shared_state*>(m_state.get()); state->stop_signal_sent = true; - TRY_ENTRY(); - connections_mutex.lock(); - for (auto &c: connections_) + return true; + } + //--------------------------------------------------------------------------------- + template<class t_protocol_handler> + void boosted_tcp_server<t_protocol_handler>::close_server_connections() + { + decltype(connections_) connections; { - c->cancel(); + boost::unique_lock<boost::mutex> lock(connections_mutex); + connections.swap(connections_); } - connections_.clear(); - connections_mutex.unlock(); - // Since we shut down connections in the strand, we want to make sure to complete the shutdown sequence before - // stopping the io_context. We let the caller handle closing because the caller is the one keeping track of all - // connections (connections_ is only a subset of all connections). - close_all_connections(); + for (auto &c: connections) + { + c->cancel(true/*wait_for_shutdown*/); + } + } + //--------------------------------------------------------------------------------- + template<class t_protocol_handler> + void boosted_tcp_server<t_protocol_handler>::stop_io_context() + { + { + boost::unique_lock<boost::mutex> lock(connections_mutex); + if (!connections_.empty()) + { + MERROR("Stopping io_context with " << connections_.size() << " server-owned connections still open"); + } + } + MDEBUG("Stopping io_context"); io_context_.stop(); - MDEBUG("Done with send_stop_signal"); + } + //--------------------------------------------------------------------------------- + template<class t_protocol_handler> + void boosted_tcp_server<t_protocol_handler>::send_stop_signal() + { + TRY_ENTRY(); + if (!mark_stop_signal_sent()) + return; + close_server_connections(); + stop_io_context(); CATCH_ENTRY_L0("boosted_tcp_server<t_protocol_handler>::send_stop_signal()", void()); } //--------------------------------------------------------------------------------- diff --git a/contrib/epee/src/parserse_base_utils.cpp b/contrib/epee/src/parserse_base_utils.cpp index e154a75f8..923dde424 100644 --- a/contrib/epee/src/parserse_base_utils.cpp +++ b/contrib/epee/src/parserse_base_utils.cpp @@ -129,7 +129,7 @@ namespace misc_utils case '/': //Slash character val.push_back('/');break; case 'u': //Unicode code point - if (buf_end - it < 4) + if (buf_end - it < 5) { ASSERT_MES_AND_THROW("Invalid Unicode escape sequence"); } diff --git a/contrib/gitian/dockrun.sh b/contrib/gitian/dockrun.sh index 396db126b..63aa6eef6 100755 --- a/contrib/gitian/dockrun.sh +++ b/contrib/gitian/dockrun.sh @@ -9,12 +9,18 @@ VERSION=$1 DOCKER=`command -v docker` CACHER=`command -v apt-cacher-ng` -if [ -z "$DOCKER" -o -z "$CACHER" ]; then - echo "$0: you must first install docker.io and apt-cacher-ng" - echo " e.g. sudo apt-get install docker.io apt-cacher-ng" +if [ -z "$DOCKER" ]; then + echo "$0: you must first install docker.io" + echo " e.g. sudo apt-get install docker.io" exit 1 fi +# only use APT cacher if package is present +DOCKER_CACHE_LINE="" +if [ "$CACHER" ]; then + DOCKER_CACHE_LINE="RUN echo 'Acquire::http { Proxy \"http://172.17.0.1:3142\"; };' > /etc/apt/apt.conf.d/50cacher" +fi + GH_USER=${GH_USER-$USER} TAG=gitrun-bionic @@ -33,7 +39,7 @@ cat <<EOF > ${TAG}.Dockerfile FROM ubuntu:bionic ENV DEBIAN_FRONTEND=noninteractive -RUN echo 'Acquire::http { Proxy "http://172.17.0.1:3142"; };' > /etc/apt/apt.conf.d/50cacher +$DOCKER_CACHE_LINE RUN echo "$GID" >> /etc/group RUN apt-get update && apt-get --no-install-recommends -y install lsb-release ruby git make wget docker.io python3 curl @@ -66,7 +72,7 @@ cat <<EOF > ${TAG2}.Dockerfile FROM ubuntu:bionic ENV DEBIAN_FRONTEND=noninteractive -RUN echo 'Acquire::http { Proxy "http://172.17.0.1:3142"; };' > /etc/apt/apt.conf.d/50cacher +$DOCKER_CACHE_LINE RUN apt-get update && apt-get --no-install-recommends -y install build-essential git language-pack-en \ wget lsb-release curl gcc-7 g++-7 gcc g++ binutils-gold pkg-config autoconf libtool automake faketime \ bsdmainutils ca-certificates python cmake gperf diff --git a/src/daemonizer/posix_fork.cpp b/src/daemonizer/posix_fork.cpp index 16758215d..39e9f7b5f 100644 --- a/src/daemonizer/posix_fork.cpp +++ b/src/daemonizer/posix_fork.cpp @@ -5,13 +5,19 @@ // #include "daemonizer/posix_fork.h" +#include "misc_language.h" #include "misc_log_ex.h" +#include <cerrno> #include <cstdlib> +#include <cstring> #include <fcntl.h> +#include <fstream> #include <unistd.h> #include <stdexcept> #include <string> +#include <sys/stat.h> +#include <sys/types.h> #ifndef TMPDIR #define TMPDIR "/tmp" @@ -35,26 +41,64 @@ void fork(const std::string & pidfile) // processes. // Only in the final child process do we write the PID to the // file (and close it). - std::ofstream pidofs; + int pid_fd = -1; + auto close_pid_fd = [&pid_fd]() + { + if (pid_fd >= 0) + { + close(pid_fd); + pid_fd = -1; + } + }; + epee::misc_utils::auto_scope_leave_caller pid_fd_guard = + epee::misc_utils::create_scope_leave_handler(close_pid_fd); if (! pidfile.empty ()) { - int oldpid; - std::ifstream pidrifs; - pidrifs.open(pidfile, std::fstream::in); - if (! pidrifs.fail()) + struct stat st; + if (lstat(pidfile.c_str(), &st) == 0) { - // Read the PID and send signal 0 to see if the process exists. - if (pidrifs >> oldpid && oldpid > 1 && kill(oldpid, 0) == 0) + if (S_ISLNK(st.st_mode)) { - quit("PID file " + pidfile + " already exists and the PID therein is valid"); - } - pidrifs.close(); - } + quit("PID file path is a symlink, refusing: " + pidfile); + } + if (!S_ISREG(st.st_mode)) + { + quit("PID file path exists and is not a regular file: " + pidfile); + } - pidofs.open(pidfile, std::fstream::out | std::fstream::trunc); - if (pidofs.fail()) + int oldpid = 0; + std::ifstream pidrifs; + pidrifs.open(pidfile, std::fstream::in); + if (!pidrifs.fail()) + { + // Read the PID and send signal 0 to see if the process exists. + errno = 0; + if (pidrifs >> oldpid && oldpid > 1 && (kill(oldpid, 0) == 0 || errno == EPERM)) + { + quit("PID file " + pidfile + " already exists and the PID therein is valid"); + } + pidrifs.close(); + } + + if (unlink(pidfile.c_str()) != 0) + { + quit("Failed to remove stale PID file: " + pidfile + ": " + std::strerror(errno)); + } + } + else if (errno != ENOENT) { - quit("Failed to open specified PID file for writing"); + quit("Failed to inspect PID file path: " + pidfile + ": " + std::strerror(errno)); + } + +#ifdef O_NOFOLLOW + const int flags = O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW; +#else + const int flags = O_WRONLY | O_CREAT | O_EXCL; +#endif + pid_fd = open(pidfile.c_str(), flags, 0644); + if (pid_fd < 0) + { + quit("Failed to create PID file: " + pidfile + ": " + std::strerror(errno)); } } // Fork the process and have the parent exit. If the process was started @@ -65,7 +109,7 @@ void fork(const std::string & pidfile) if (pid > 0) { // We're in the parent process and need to exit. - pidofs.close(); + close_pid_fd(); // When the exit() function is used, the program terminates without // invoking local variables' destructors. Only global variables are // destroyed. @@ -86,7 +130,7 @@ void fork(const std::string & pidfile) { if (pid > 0) { - pidofs.close(); + close_pid_fd(); exit(0); } else @@ -95,11 +139,18 @@ void fork(const std::string & pidfile) } } - if (! pidofs.fail()) + if (pid_fd >= 0) { - int pid = ::getpid(); - pidofs << pid << std::endl; - pidofs.close(); + const std::string pid = std::to_string(::getpid()) + "\n"; + const ssize_t written = write(pid_fd, pid.data(), pid.size()); + if (written < 0) + { + quit("Failed to write PID file: " + pidfile + ": " + std::strerror(errno)); + } + if (static_cast<size_t>(written) != pid.size()) + { + quit("Failed to write complete PID file: " + pidfile); + } } // Close the standard streams. This decouples the daemon from the terminal diff --git a/src/net/i2p_address.cpp b/src/net/i2p_address.cpp index e793048c0..e24e7da01 100644 --- a/src/net/i2p_address.cpp +++ b/src/net/i2p_address.cpp @@ -117,11 +117,15 @@ namespace net bool i2p_address::_load(epee::serialization::portable_storage& src, epee::serialization::section* hparent) { i2p_serialized in{}; - if (in._load(src, hparent) && in.host.size() < sizeof(host_) && (in.host == unknown_host || !host_check(in.host).has_error())) + if (in._load(src, hparent) && in.host.size() < sizeof(host_)) { - std::memcpy(host_, in.host.data(), in.host.size()); - std::memset(host_ + in.host.size(), 0, sizeof(host_) - in.host.size()); - return true; + net::canonicalize_host(in.host); + if (in.host == unknown_host || !host_check(in.host).has_error()) + { + std::memcpy(host_, in.host.data(), in.host.size()); + std::memset(host_ + in.host.size(), 0, sizeof(host_) - in.host.size()); + return true; + } } static_assert(sizeof(unknown_host) <= sizeof(host_), "bad buffer size"); std::memcpy(host_, unknown_host, sizeof(unknown_host)); // include null terminator diff --git a/src/net/tor_address.cpp b/src/net/tor_address.cpp index 35bd8e9a2..25f9fde66 100644 --- a/src/net/tor_address.cpp +++ b/src/net/tor_address.cpp @@ -129,12 +129,16 @@ namespace net bool tor_address::_load(epee::serialization::portable_storage& src, epee::serialization::section* hparent) { tor_serialized in{}; - if (in._load(src, hparent) && in.host.size() < sizeof(host_) && (in.host == unknown_host || !host_check(in.host).has_error())) + if (in._load(src, hparent) && in.host.size() < sizeof(host_)) { - std::memcpy(host_, in.host.data(), in.host.size()); - std::memset(host_ + in.host.size(), 0, sizeof(host_) - in.host.size()); - port_ = in.port; - return true; + net::canonicalize_host(in.host); + if (in.host == unknown_host || !host_check(in.host).has_error()) + { + std::memcpy(host_, in.host.data(), in.host.size()); + std::memset(host_ + in.host.size(), 0, sizeof(host_) - in.host.size()); + port_ = in.port; + return true; + } } static_assert(sizeof(unknown_host) <= sizeof(host_), "bad buffer size"); std::memcpy(host_, unknown_host, sizeof(unknown_host)); // include null terminator diff --git a/src/p2p/net_node.h b/src/p2p/net_node.h index 8e3312f29..d7cbaa567 100644 --- a/src/p2p/net_node.h +++ b/src/p2p/net_node.h @@ -451,6 +451,7 @@ namespace nodetool bool m_use_ipv6; bool m_require_ipv4; std::atomic<bool> is_closing; + std::atomic<bool> m_stop_signal_sent_once{false}; std::unique_ptr<boost::thread> mPeersLoggerThread; //critical_section m_connections_lock; //connections_indexed_container m_connections; diff --git a/src/p2p/net_node.inl b/src/p2p/net_node.inl index d1f7e7cdd..91a54f43e 100644 --- a/src/p2p/net_node.inl +++ b/src/p2p/net_node.inl @@ -1107,28 +1107,44 @@ namespace nodetool template<class t_payload_net_handler> bool node_server<t_payload_net_handler>::send_stop_signal() { + if (m_stop_signal_sent_once.exchange(true)) + { + MDEBUG("[node] Stop signal already sent"); + return true; + } MDEBUG("[node] stopping server payload handler"); m_payload_handler.stop(); - MDEBUG("[node] sending stop signal"); + + MDEBUG("[node] marking net servers as stopping"); for (auto& zone : m_network_zones) { - const auto close_all_connections = [&, this]() + zone.second.m_net_server.mark_stop_signal_sent(); + } + + MDEBUG("[node] closing connections"); + for (auto& zone : m_network_zones) + { + zone.second.m_net_server.close_server_connections(); + + std::list<boost::uuids::uuid> connection_ids; + zone.second.m_net_server.get_config_object().foreach_connection([&](const p2p_connection_context& cntxt) { - std::list<boost::uuids::uuid> connection_ids; - zone.second.m_net_server.get_config_object().foreach_connection([&](const p2p_connection_context& cntxt) { - connection_ids.push_back(cntxt.m_connection_id); - return true; - }); - for (const auto &connection_id: connection_ids) - { - MDEBUG("Closing connection " << connection_id); - // We need to wait for every connection's shutdown sequence to complete before stopping the io_context. - zone.second.m_net_server.get_config_object().close(connection_id, true/*wait_for_shutdown*/); - MDEBUG("Closed connection " << connection_id); - } - }; + connection_ids.push_back(cntxt.m_connection_id); + return true; + }); + for (const auto &connection_id: connection_ids) + { + MDEBUG("Closing connection " << connection_id); + // All zone connections must finish shutting down before any shared io_context is stopped. + zone.second.m_net_server.get_config_object().close(connection_id, true/*wait_for_shutdown*/); + MDEBUG("Closed connection " << connection_id); + } + } - zone.second.m_net_server.send_stop_signal(close_all_connections); + MDEBUG("[node] stopping net server io_contexts"); + for (auto& zone : m_network_zones) + { + zone.second.m_net_server.stop_io_context(); } MDEBUG("[node] Stop signal sent"); return true; diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index 4c8d54f61..61f49481e 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -9116,7 +9116,6 @@ void wallet2::light_wallet_get_outs(std::vector<std::vector<tools::wallet2::get_ { const boost::lock_guard<boost::recursive_mutex> lock{m_daemon_rpc_mutex}; bool r = epee::net_utils::invoke_http_json("/get_random_outs", oreq, ores, *m_http_client, rpc_timeout, "POST"); - m_daemon_rpc_mutex.unlock(); THROW_WALLET_EXCEPTION_IF(!r, error::no_connection_to_daemon, "get_random_outs"); THROW_WALLET_EXCEPTION_IF(ores.amount_outs.empty() , error::wallet_internal_error, "No outputs received from light wallet node. Error: " + ores.Error); } diff --git a/src/wallet/wallet_rpc_server.cpp b/src/wallet/wallet_rpc_server.cpp index 634bdae76..6a3e92b0b 100644 --- a/src/wallet/wallet_rpc_server.cpp +++ b/src/wallet/wallet_rpc_server.cpp @@ -3336,14 +3336,14 @@ namespace tools } entry.m_address = info.address; entry.m_is_subaddress = info.is_subaddress; - if (info.has_payment_id) - entry.m_payment_id = info.payment_id; + entry.m_has_payment_id = info.has_payment_id; + entry.m_payment_id = info.has_payment_id ? info.payment_id : crypto::null_hash8; } if (req.set_description) entry.m_description = req.description; - if (!m_wallet->set_address_book_row(req.index, entry.m_address, req.set_address && entry.m_has_payment_id ? &entry.m_payment_id : NULL, entry.m_description, entry.m_is_subaddress)) + if (!m_wallet->set_address_book_row(req.index, entry.m_address, entry.m_has_payment_id ? &entry.m_payment_id : NULL, entry.m_description, entry.m_is_subaddress)) { er.code = WALLET_RPC_ERROR_CODE_UNKNOWN_ERROR; er.message = "Failed to edit address book entry"; diff --git a/tests/unit_tests/epee_boosted_tcp_server.cpp b/tests/unit_tests/epee_boosted_tcp_server.cpp index 6052f7a50..586cc25a5 100644 --- a/tests/unit_tests/epee_boosted_tcp_server.cpp +++ b/tests/unit_tests/epee_boosted_tcp_server.cpp @@ -801,14 +801,11 @@ TEST(boosted_tcp_server, shutdown) server.get_config_object().handshake_received.wait(); } - // Now stop the server, providing the callback necessary to wait for all connections to shutdown - const auto close_all_connections = [&]() - { - server.get_config_object().close(context.m_connection_id, true/*wait_for_shutdown*/); - }; - MINFO("Stopping the server"); - server.send_stop_signal(close_all_connections); + server.mark_stop_signal_sent(); + server.close_server_connections(); + server.get_config_object().close(context.m_connection_id, true/*wait_for_shutdown*/); + server.stop_io_context(); running_server.join(); MINFO("Waiting for handshake to cancel"); diff --git a/tests/unit_tests/net.cpp b/tests/unit_tests/net.cpp index 9633f50e5..291f1e6ab 100644 --- a/tests/unit_tests/net.cpp +++ b/tests/unit_tests/net.cpp @@ -343,6 +343,20 @@ TEST(tor_address, epee_serializev_v3) EXPECT_STREQ(v3_onion, command.tor.host_str()); EXPECT_EQ(10u, command.tor.port()); + // make sure tor_address::_load canonicalizes incoming hosts + { + epee::serialization::portable_storage stg{}; + stg.load_from_binary(epee::to_span(buffer)); + + EXPECT_TRUE(stg.set_value("host", std::string{v3_onion_upper}, stg.open_section("tor", nullptr, false))); + EXPECT_TRUE(command.load(stg)); + } + + EXPECT_FALSE(command.tor.is_unknown()); + EXPECT_NE(net::tor_address{}, command.tor); + EXPECT_STREQ(v3_onion, command.tor.host_str()); + EXPECT_EQ(10u, command.tor.port()); + // make sure that exceeding max buffer doesn't destroy tor_address::_load { epee::serialization::portable_storage stg{}; @@ -751,6 +765,20 @@ TEST(i2p_address, epee_serializev_b32) EXPECT_STREQ(b32_i2p, command.i2p.host_str()); EXPECT_EQ(1u, command.i2p.port()); + // make sure i2p_address::_load canonicalizes incoming hosts + { + epee::serialization::portable_storage stg{}; + stg.load_from_binary(epee::to_span(buffer)); + + EXPECT_TRUE(stg.set_value("host", std::string{b32_i2p_upper}, stg.open_section("i2p", nullptr, false))); + EXPECT_TRUE(command.load(stg)); + } + + EXPECT_FALSE(command.i2p.is_unknown()); + EXPECT_NE(net::i2p_address{}, command.i2p); + EXPECT_STREQ(b32_i2p, command.i2p.host_str()); + EXPECT_EQ(1u, command.i2p.port()); + // make sure that exceeding max buffer doesn't destroy i2p_address::_load { epee::serialization::portable_storage stg{}; |