Skip to content

Commit

Permalink
Merge pull request #2360 from mavlink/pr-backport-socket
Browse files Browse the repository at this point in the history
[BACKPORT v2.12] core: Correctly close sockets
  • Loading branch information
JonasVautherin authored Jul 25, 2024
2 parents 93b91d6 + 73b42d3 commit fda7259
Show file tree
Hide file tree
Showing 7 changed files with 114 additions and 50 deletions.
1 change: 1 addition & 0 deletions src/mavsdk/core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
53 changes: 53 additions & 0 deletions src/mavsdk/core/socket_holder.cpp
Original file line number Diff line number Diff line change
@@ -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
37 changes: 37 additions & 0 deletions src/mavsdk/core/socket_holder.h
Original file line number Diff line number Diff line change
@@ -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
31 changes: 10 additions & 21 deletions src/mavsdk/core/tcp_connection.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include <arpa/inet.h>
#include <errno.h>
#include <netdb.h>
#include <unistd.h> // for close()
#endif

#include <cassert>
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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();
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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.
Expand Down
4 changes: 3 additions & 1 deletion src/mavsdk/core/tcp_connection.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#pragma once

#include "socket_holder.h"

#include <atomic>
#include <mutex>
#include <memory>
Expand Down Expand Up @@ -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;
Expand Down
34 changes: 7 additions & 27 deletions src/mavsdk/core/udp_connection.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
#include <sys/socket.h>
#include <arpa/inet.h>
#include <errno.h>
#include <unistd.h> // for close()
#endif

#include <algorithm>
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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();
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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;
Expand Down
4 changes: 3 additions & 1 deletion src/mavsdk/core/udp_connection.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
#include <atomic>
#include <vector>
#include <cstdint>

#include "connection.h"
#include "socket_holder.h"

namespace mavsdk {

Expand Down Expand Up @@ -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};
};
Expand Down

0 comments on commit fda7259

Please sign in to comment.