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

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

* 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 <[email protected]>
Co-authored-by: Julian Oes <[email protected]>

Author: 3 people

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

src/mavsdk/core/socket_holder.cpp

@@ -0,0 +1,53 @@
+#include "socket_holder.h"
+
+#ifndef WINDOWS
+#include <sys/socket.h>
+#include <unistd.h>
+#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

src/mavsdk/core/socket_holder.h

@@ -0,0 +1,37 @@
+#pragma once
+
+#if defined(WINDOWS)
+#include <winsock2.h>
+#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

src/mavsdk/core/tcp_connection.cpp

@@ -11,7 +11,6 @@
 #include <arpa/inet.h>
 #include <errno.h>
 #include <netdb.h>
-#include <unistd.h> // for close()
 #endif
 
 #include <cassert>
@@ -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<sockaddr*>(&remote_addr), sizeof(struct sockaddr_in)) <
-        0) {
+    if (connect(
+            _socket_fd.get(),
+            reinterpret_cast<sockaddr*>(&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<char*>(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.

src/mavsdk/core/tcp_connection.h

@@ -1,5 +1,7 @@
 #pragma once
 
+#include "socket_holder.h"
+
 #include <atomic>
 #include <mutex>
 #include <memory>
@@ -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<std::thread> _recv_thread{};
     std::atomic_bool _should_exit;

src/mavsdk/core/udp_connection.cpp

@@ -13,7 +13,6 @@
 #include <sys/socket.h>
 #include <arpa/inet.h>
 #include <errno.h>
-#include <unistd.h> // for close()
 #endif
 
 #include <algorithm>
@@ -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<sockaddr*>(&addr), sizeof(addr)) != 0) {
+    if (bind(_socket_fd.get(), reinterpret_cast<sockaddr*>(&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<char*>(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;

src/mavsdk/core/udp_connection.h

@@ -7,7 +7,9 @@
 #include <atomic>
 #include <vector>
 #include <cstdint>
+
 #include "connection.h"
+#include "socket_holder.h"
 
 namespace mavsdk {
 
@@ -54,7 +56,7 @@ class UdpConnection : public Connection {
     };
     std::vector<Remote> _remotes{};
 
-    int _socket_fd{-1};
+    SocketHolder _socket_fd;
     std::unique_ptr<std::thread> _recv_thread{};
     std::atomic_bool _should_exit{false};
 };