From 73b42d37c9489025bca248c2af72ab7f18eaf8ac Mon Sep 17 00:00:00 2001 From: fibonacci-matrix <176021117+fibonacci-matrix@users.noreply.github.com> Date: Thu, 25 Jul 2024 10:39:39 +0300 Subject: [PATCH] core: Correctly close sockets (#2357) * core: Correctly close sockets * Update socket_holder.cpp - Fix newline style * Update socket_holder.cpp * Update src/mavsdk/core/tcp_client_connection.cpp Co-authored-by: Jonas Vautherin * Update socket_holder.cpp - fix code style * Update tcp_client_connection.cpp - fix code style * Update tcp_server_connection.cpp - fix code style * Update socket_holder.h - fix code style * Update src/mavsdk/core/socket_holder.h Co-authored-by: Julian Oes * Update socket_holder.h - use 64 bit descriptor type on Win64 * Update socket_holder.cpp - use 64 bit descriptor type on Win64 * Update socket_holder.cpp - minor improving of if logic * Remove default move constructor It is currently not in use, and the default implementation is not suitable because it does not change _fd to INVALID for the object being copied from --------- Co-authored-by: Jonas Vautherin Co-authored-by: Julian Oes --- src/mavsdk/core/CMakeLists.txt | 1 + src/mavsdk/core/socket_holder.cpp | 53 ++++++++++++++++++++++++++++++ src/mavsdk/core/socket_holder.h | 37 +++++++++++++++++++++ src/mavsdk/core/tcp_connection.cpp | 31 ++++++----------- src/mavsdk/core/tcp_connection.h | 4 ++- src/mavsdk/core/udp_connection.cpp | 34 ++++--------------- src/mavsdk/core/udp_connection.h | 4 ++- 7 files changed, 114 insertions(+), 50 deletions(-) create mode 100644 src/mavsdk/core/socket_holder.cpp create mode 100644 src/mavsdk/core/socket_holder.h diff --git a/src/mavsdk/core/CMakeLists.txt b/src/mavsdk/core/CMakeLists.txt index 5b6a41c2b5..a9a56799b5 100644 --- a/src/mavsdk/core/CMakeLists.txt +++ b/src/mavsdk/core/CMakeLists.txt @@ -51,6 +51,7 @@ target_sources(mavsdk server_component_impl.cpp server_plugin_impl_base.cpp tcp_connection.cpp + socket_holder.cpp timeout_handler.cpp udp_connection.cpp log.cpp diff --git a/src/mavsdk/core/socket_holder.cpp b/src/mavsdk/core/socket_holder.cpp new file mode 100644 index 0000000000..cccc56efc0 --- /dev/null +++ b/src/mavsdk/core/socket_holder.cpp @@ -0,0 +1,53 @@ +#include "socket_holder.h" + +#ifndef WINDOWS +#include +#include +#endif + +namespace mavsdk { + +SocketHolder::SocketHolder(DescriptorType fd) noexcept : _fd{fd} {} + +SocketHolder::~SocketHolder() noexcept +{ + close(); +} + +void SocketHolder::reset(DescriptorType fd) noexcept +{ + if (_fd != fd) { + close(); + _fd = fd; + } +} + +void SocketHolder::close() noexcept +{ + if (!empty()) { +#if defined(WINDOWS) + shutdown(_fd, SD_BOTH); + closesocket(_fd); + WSACleanup(); +#else + // This should interrupt a recv/recvfrom call. + shutdown(_fd, SHUT_RDWR); + + // But on Mac, closing is also needed to stop blocking recv/recvfrom. + ::close(_fd); +#endif + _fd = invalid_socket_fd; + } +} + +bool SocketHolder::empty() const noexcept +{ + return _fd == invalid_socket_fd; +} + +SocketHolder::DescriptorType SocketHolder::get() const noexcept +{ + return _fd; +} + +} // namespace mavsdk diff --git a/src/mavsdk/core/socket_holder.h b/src/mavsdk/core/socket_holder.h new file mode 100644 index 0000000000..cf0dd0991c --- /dev/null +++ b/src/mavsdk/core/socket_holder.h @@ -0,0 +1,37 @@ +#pragma once + +#if defined(WINDOWS) +#include +#endif + +namespace mavsdk { + +class SocketHolder { +public: +#if defined(WINDOWS) + using DescriptorType = SOCKET; + static constexpr DescriptorType invalid_socket_fd = INVALID_SOCKET; +#else + using DescriptorType = int; + static constexpr DescriptorType invalid_socket_fd = -1; +#endif + + SocketHolder() noexcept = default; + explicit SocketHolder(DescriptorType socket_fd) noexcept; + + ~SocketHolder() noexcept; + + void reset(DescriptorType fd) noexcept; + void close() noexcept; + + bool empty() const noexcept; + DescriptorType get() const noexcept; + +private: + SocketHolder(const SocketHolder&) = delete; + SocketHolder& operator=(const SocketHolder&) = delete; + + DescriptorType _fd = invalid_socket_fd; +}; + +} // namespace mavsdk diff --git a/src/mavsdk/core/tcp_connection.cpp b/src/mavsdk/core/tcp_connection.cpp index d645b31ef6..fdd058b4a9 100644 --- a/src/mavsdk/core/tcp_connection.cpp +++ b/src/mavsdk/core/tcp_connection.cpp @@ -11,7 +11,6 @@ #include #include #include -#include // for close() #endif #include @@ -70,9 +69,9 @@ ConnectionResult TcpConnection::setup_port() } #endif - _socket_fd = socket(AF_INET, SOCK_STREAM, 0); + _socket_fd.reset(socket(AF_INET, SOCK_STREAM, 0)); - if (_socket_fd < 0) { + if (_socket_fd.empty()) { LogErr() << "socket error" << GET_ERROR(errno); _is_ok = false; return ConnectionResult::SocketError; @@ -92,8 +91,10 @@ ConnectionResult TcpConnection::setup_port() memcpy(&remote_addr.sin_addr, hp->h_addr, hp->h_length); - if (connect(_socket_fd, reinterpret_cast(&remote_addr), sizeof(struct sockaddr_in)) < - 0) { + if (connect( + _socket_fd.get(), + reinterpret_cast(&remote_addr), + sizeof(struct sockaddr_in)) < 0) { LogErr() << "connect error: " << GET_ERROR(errno); _is_ok = false; return ConnectionResult::SocketConnectionError; @@ -112,19 +113,7 @@ ConnectionResult TcpConnection::stop() { _should_exit = true; -#ifndef WINDOWS - // This should interrupt a recv/recvfrom call. - shutdown(_socket_fd, SHUT_RDWR); - - // But on Mac, closing is also needed to stop blocking recv/recvfrom. - close(_socket_fd); -#else - shutdown(_socket_fd, SD_BOTH); - - closesocket(_socket_fd); - - WSACleanup(); -#endif + _socket_fd.close(); if (_recv_thread) { _recv_thread->join(); @@ -174,7 +163,7 @@ bool TcpConnection::send_message(const mavlink_message_t& message) #endif const auto send_len = sendto( - _socket_fd, + _socket_fd.get(), reinterpret_cast(buffer), buffer_len, flags, @@ -201,7 +190,7 @@ void TcpConnection::receive() setup_port(); } - const auto recv_len = recv(_socket_fd, buffer, sizeof(buffer), 0); + const auto recv_len = recv(_socket_fd.get(), buffer, sizeof(buffer), 0); if (recv_len == 0) { // This can happen when shutdown is called on the socket, @@ -211,7 +200,7 @@ void TcpConnection::receive() } if (recv_len < 0) { - // This happens on desctruction when close(_socket_fd) is called, + // This happens on destruction when close(_socket_fd.get()) is called, // therefore be quiet. // LogErr() << "recvfrom error: " << GET_ERROR(errno); // Something went wrong, we should try to re-connect in next iteration. diff --git a/src/mavsdk/core/tcp_connection.h b/src/mavsdk/core/tcp_connection.h index 785905f8a0..42ab97ae0b 100644 --- a/src/mavsdk/core/tcp_connection.h +++ b/src/mavsdk/core/tcp_connection.h @@ -1,5 +1,7 @@ #pragma once +#include "socket_holder.h" + #include #include #include @@ -43,7 +45,7 @@ class TcpConnection : public Connection { int _remote_port_number; std::mutex _mutex = {}; - int _socket_fd = -1; + SocketHolder _socket_fd; std::unique_ptr _recv_thread{}; std::atomic_bool _should_exit; diff --git a/src/mavsdk/core/udp_connection.cpp b/src/mavsdk/core/udp_connection.cpp index c1c65328b4..84c0deadb5 100644 --- a/src/mavsdk/core/udp_connection.cpp +++ b/src/mavsdk/core/udp_connection.cpp @@ -13,7 +13,6 @@ #include #include #include -#include // for close() #endif #include @@ -69,9 +68,9 @@ ConnectionResult UdpConnection::setup_port() } #endif - _socket_fd = socket(AF_INET, SOCK_DGRAM, 0); + _socket_fd.reset(socket(AF_INET, SOCK_DGRAM, 0)); - if (_socket_fd < 0) { + if (_socket_fd.empty()) { LogErr() << "socket error" << GET_ERROR(errno); return ConnectionResult::SocketError; } @@ -81,7 +80,7 @@ ConnectionResult UdpConnection::setup_port() inet_pton(AF_INET, _local_ip.c_str(), &(addr.sin_addr)); addr.sin_port = htons(_local_port_number); - if (bind(_socket_fd, reinterpret_cast(&addr), sizeof(addr)) != 0) { + if (bind(_socket_fd.get(), reinterpret_cast(&addr), sizeof(addr)) != 0) { LogErr() << "bind error: " << GET_ERROR(errno); return ConnectionResult::BindError; } @@ -98,32 +97,13 @@ ConnectionResult UdpConnection::stop() { _should_exit = true; -#if !defined(WINDOWS) - // This should interrupt a recv/recvfrom call. - shutdown(_socket_fd, SHUT_RDWR); - -#if defined(APPLE) - // But on Mac, closing is also needed to stop blocking recv/recvfrom. - close(_socket_fd); -#endif -#else - shutdown(_socket_fd, SD_BOTH); - - closesocket(_socket_fd); - - WSACleanup(); -#endif + _socket_fd.close(); if (_recv_thread) { _recv_thread->join(); _recv_thread.reset(); } -#if !defined(WINDOWS) & !defined(APPLE) - // On Linux we can close later to avoid thread sanitizer from complaining. - close(_socket_fd); -#endif - // We need to stop this after stopping the receive thread, otherwise // it can happen that we interfere with the parsing of a message. stop_mavlink_receiver(); @@ -157,7 +137,7 @@ bool UdpConnection::send_message(const mavlink_message_t& message) uint16_t buffer_len = mavlink_msg_to_send_buffer(buffer, &message); const auto send_len = sendto( - _socket_fd, + _socket_fd.get(), reinterpret_cast(buffer), buffer_len, 0, @@ -212,7 +192,7 @@ void UdpConnection::receive() struct sockaddr_in src_addr = {}; socklen_t src_addr_len = sizeof(src_addr); const auto recv_len = recvfrom( - _socket_fd, + _socket_fd.get(), buffer, sizeof(buffer), 0, @@ -226,7 +206,7 @@ void UdpConnection::receive() } if (recv_len < 0) { - // This happens on destruction when close(_socket_fd) is called, + // This happens on destruction when _socket_fd.close() is called, // therefore be quiet. // LogErr() << "recvfrom error: " << GET_ERROR(errno); continue; diff --git a/src/mavsdk/core/udp_connection.h b/src/mavsdk/core/udp_connection.h index afe821e059..ea5f568657 100644 --- a/src/mavsdk/core/udp_connection.h +++ b/src/mavsdk/core/udp_connection.h @@ -7,7 +7,9 @@ #include #include #include + #include "connection.h" +#include "socket_holder.h" namespace mavsdk { @@ -54,7 +56,7 @@ class UdpConnection : public Connection { }; std::vector _remotes{}; - int _socket_fd{-1}; + SocketHolder _socket_fd; std::unique_ptr _recv_thread{}; std::atomic_bool _should_exit{false}; };