Merge pull request #2360 from mavlink/pr-backport-socket

[BACKPORT v2.12] core: Correctly close sockets

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};
 };