From 7b84d0f9819e79b170325f1183325916fe545d7c Mon Sep 17 00:00:00 2001 From: Julian Oes Date: Thu, 16 Jul 2026 10:40:32 +1200 Subject: [PATCH 1/9] core: backport FTP client/server bugfixes from main Backports the following fixes from main to v3: - 10c11967b core: fix out-of-bounds read in MavlinkFtpServer::_data_as_string - 93a625b73 core: check ofstream (not ifstream) after seekp in FTP _work_write - 9235e523e core: fix FTP server deadlock joining burst thread while holding _mutex - 48b498719 core: make FTP server target ids atomic - cbac9c089 core: reject FTP burst offset past file size to avoid huge allocation - a565b661d fix: reject FTP responses with unexpected seq_number (#2850) --- src/mavsdk/core/mavlink_ftp_client.cpp | 46 ++++++++++++++++++++++---- src/mavsdk/core/mavlink_ftp_client.h | 2 +- src/mavsdk/core/mavlink_ftp_server.cpp | 43 +++++++++++++++++++----- src/mavsdk/core/mavlink_ftp_server.h | 14 +++++--- 4 files changed, 86 insertions(+), 19 deletions(-) diff --git a/src/mavsdk/core/mavlink_ftp_client.cpp b/src/mavsdk/core/mavlink_ftp_client.cpp index b5d8adbafc..ed5135020a 100644 --- a/src/mavsdk/core/mavlink_ftp_client.cpp +++ b/src/mavsdk/core/mavlink_ftp_client.cpp @@ -150,11 +150,32 @@ void MavlinkFtpClient::process_mavlink_ftp_message(const mavlink_message_t& msg) << ", req: " << (int)payload->req_opcode; return; } - if (work->last_received_seq_number != 0 && - work->last_received_seq_number == payload->seq_number) { - // We have already seen this ack/nak. - LogWarn() << "Already seen"; - return; + // For non-burst transfers, strictly check the expected seq_number. + // work->payload.seq_number is the seq_number of the most-recently sent + // request; the server echoes back seq_number + 1. Accepting any other + // value would allow stale/duplicate UDP datagrams from a previous request + // to corrupt the transfer (e.g. writing a full 239-byte chunk when only + // the final 49 bytes remain). + // + // For burst transfers we cannot use a strict check: the server sends + // multiple packets per CMD_BURST_READ_FILE request, each with an + // incrementing seq_number, and packets may arrive out of order or with + // gaps that are filled by a subsequent re-request. For burst we fall back + // to the original duplicate-rejection approach. + const bool is_burst = std::holds_alternative(work->item); + if (!is_burst) { + const auto expected_seq = static_cast(work->payload.seq_number + 1); + if (payload->seq_number != expected_seq) { + LogWarn() << "Unexpected seq: got " << (int)payload->seq_number << ", expected " + << (int)expected_seq; + return; + } + } else { + if (work->last_received_seq_number != 0 && + work->last_received_seq_number == payload->seq_number) { + LogWarn() << "Already seen seq: " << (int)payload->seq_number; + return; + } } std::visit( @@ -378,7 +399,10 @@ void MavlinkFtpClient::process_mavlink_ftp_message(const mavlink_message_t& msg) }}, work->item); - work->last_received_seq_number = payload->seq_number; + // Track the last received seq for burst duplicate detection. + if (is_burst) { + work->last_received_seq_number = payload->seq_number; + } } bool MavlinkFtpClient::download_start(Work& work, DownloadItem& item) @@ -538,6 +562,16 @@ bool MavlinkFtpClient::download_burst_continue( return false; } + if (payload->offset > item.file_size) { + // The server should never point us past the end of the file. Reject rather + // than allocating/zero-filling a gap of up to ~4 GB from a bad offset. + LogWarn() << "Got payload offset " << (uint32_t)payload->offset + << " past file size " << item.file_size; + item.callback(ClientResult::ProtocolError, {}); + download_burst_end(work); + return false; + } + // we missed a part item.missing_data.emplace_back(DownloadBurstItem::MissingData{ item.current_offset, payload->offset - item.current_offset}); diff --git a/src/mavsdk/core/mavlink_ftp_client.h b/src/mavsdk/core/mavlink_ftp_client.h index 0c3b0e7d28..bfc6898f01 100644 --- a/src/mavsdk/core/mavlink_ftp_client.h +++ b/src/mavsdk/core/mavlink_ftp_client.h @@ -229,7 +229,7 @@ class MavlinkFtpClient { unsigned retries{RETRIES}; bool started{false}; Opcode last_opcode{}; - uint16_t last_received_seq_number{0}; + uint16_t last_received_seq_number{0}; // used for burst duplicate detection uint8_t target_compid{}; Work(Item new_item, uint8_t target_compid_) : item(std::move(new_item)), diff --git a/src/mavsdk/core/mavlink_ftp_server.cpp b/src/mavsdk/core/mavlink_ftp_server.cpp index 6f7d627c35..b7bb02f554 100644 --- a/src/mavsdk/core/mavlink_ftp_server.cpp +++ b/src/mavsdk/core/mavlink_ftp_server.cpp @@ -223,17 +223,32 @@ void MavlinkFtpServer::_send_mavlink_ftp_message(const PayloadHeader& payload) std::string MavlinkFtpServer::_data_as_string(const PayloadHeader& payload, size_t entry) { + // Only ever scan within the bytes the sender claims are valid. payload.size is + // validated to be <= max_data_length before we get here, but clamp defensively so + // we can never read past the fixed data[] buffer regardless of the input. + const size_t data_length = std::min(static_cast(payload.size), size_t{max_data_length}); + size_t start = 0; size_t end = 0; - std::string result; for (int i = entry; i >= 0; --i) { start = end; + if (start >= data_length) { + // The requested entry is beyond the available data. + return {}; + } end += - strnlen(reinterpret_cast(&payload.data[start]), max_data_length - start) + - 1; + strnlen(reinterpret_cast(&payload.data[start]), data_length - start) + 1; + } + + // The trailing field may not be null-terminated within the valid data, in which case + // strnlen pushed end one past data_length. Clamp it back so the memcpy stays in bounds. + end = std::min(end, data_length); + if (end <= start) { + return {}; } + std::string result; result.resize(end - start); std::memcpy(result.data(), &payload.data[start], end - start); @@ -789,18 +804,30 @@ void MavlinkFtpServer::_work_burst(const PayloadHeader& payload) // Schedule sending out burst messages. _session_info.burst_thread = std::thread([this]() { - while (!_session_info.burst_stop) - if (_send_burst_packet()) + while (!_session_info.burst_stop) { + // Try to grab the lock rather than blocking on it. If another thread holds + // _mutex to stop and join us, blocking here would deadlock; instead we fall + // back to observing burst_stop (atomic) and exit. + std::unique_lock burst_lock(_mutex, std::try_to_lock); + if (!burst_lock.owns_lock()) { + std::this_thread::yield(); + continue; + } + if (_session_info.burst_stop) { break; + } + if (_send_burst_packet()) { + break; + } + } }); // Don't send response as that's done in the call every burst call above. } -// Returns true if sending is complete +// Requires _mutex to be held. Returns true if sending is complete. bool MavlinkFtpServer::_send_burst_packet() { - std::lock_guard lock(_mutex); if (!_session_info.ifstream.is_open()) { return false; } @@ -867,7 +894,7 @@ void MavlinkFtpServer::_work_write(const PayloadHeader& payload) } _session_info.ofstream.seekp(payload.offset); - if (_session_info.ifstream.fail()) { + if (_session_info.ofstream.fail()) { response.opcode = Opcode::RSP_NAK; response.size = 1; response.data[0] = ServerResult::ERR_FAIL; diff --git a/src/mavsdk/core/mavlink_ftp_server.h b/src/mavsdk/core/mavlink_ftp_server.h index f9e1ad309f..efc81126f9 100644 --- a/src/mavsdk/core/mavlink_ftp_server.h +++ b/src/mavsdk/core/mavlink_ftp_server.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -134,7 +135,7 @@ class MavlinkFtpServer { void _work_rename(const PayloadHeader& payload); void _work_calc_file_CRC32(const PayloadHeader& payload); - bool _send_burst_packet(); + bool _send_burst_packet(); // Requires _mutex to be held. void _make_burst_packet(PayloadHeader& packet); std::mutex _mutex{}; @@ -144,13 +145,18 @@ class MavlinkFtpServer { uint8_t burst_chunk_size{0}; std::ifstream ifstream; std::ofstream ofstream; - bool burst_stop{false}; + // Atomic so the burst worker can observe a stop request without holding + // _mutex, which lets a thread that holds _mutex join the worker without + // deadlocking (the worker would otherwise block trying to re-acquire _mutex). + std::atomic burst_stop{false}; std::thread burst_thread; } _session_info{}; uint8_t _network_id = 0; - uint8_t _target_system_id = 0; - uint8_t _target_component_id = 0; + // Written from the message-processing thread and read from the burst thread + // (via _send_mavlink_ftp_message), so keep these atomic. + std::atomic _target_system_id{0}; + std::atomic _target_component_id{0}; std::string _root_dir{}; std::mutex _tmp_files_mutex{}; From 05603963c70adb397185a7df6f5c3ca9de2ef667 Mon Sep 17 00:00:00 2001 From: Julian Oes Date: Thu, 16 Jul 2026 10:40:33 +1200 Subject: [PATCH 2/9] core: backport parsing/safety bugfixes from main Backports the following fixes from main to v3: - bd632eba1 core: fix UAF when unregister_all_blocking misses _register_later_table (#2876) - 4fea94bb6 core: avoid std::stoi terminate() when parsing metadata uri component id - 5c5eea8b2 core: check fopen result before using it in download_file_to_path - 1bfd36358 core: fix out-of-bounds read on non-terminated PARAM_EXT_SET custom value - 2e1c78ca1 core: guard against NULL getpwuid() in get_cache_directory - c97fa917a fix to properly remove the trailing null byte (#2899) - 46b3c9ade core: reject trailing garbage when parsing port and baudrate - 6d68e3d73 core: check inet_ntop result in resolve_hostname_to_ip --- src/mavsdk/core/cli_arg.cpp | 7 +++++-- src/mavsdk/core/curl_wrapper.cpp | 7 +++++++ src/mavsdk/core/fs_utils.cpp | 12 ++++++------ src/mavsdk/core/hostname_to_ip.cpp | 13 +++++++------ src/mavsdk/core/mavlink_component_metadata.cpp | 12 +++++++++++- src/mavsdk/core/mavlink_message_handler.cpp | 16 ++++++++++++++++ src/mavsdk/core/param_value.cpp | 2 +- 7 files changed, 53 insertions(+), 16 deletions(-) diff --git a/src/mavsdk/core/cli_arg.cpp b/src/mavsdk/core/cli_arg.cpp index d49b418517..1f6c5e396d 100644 --- a/src/mavsdk/core/cli_arg.cpp +++ b/src/mavsdk/core/cli_arg.cpp @@ -230,7 +230,9 @@ std::optional CliArg::port_from_str(std::string_view str) int value; auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(), value); - if (static_cast(ec) || value < 1 || value > 65535) { + // from_chars stops at the first non-digit and reports success, so also require + // that the whole string was consumed to reject inputs like "8080abc". + if (static_cast(ec) || ptr != str.data() + str.size() || value < 1 || value > 65535) { return {}; } return {value}; @@ -284,7 +286,8 @@ bool CliArg::parse_serial(const std::string_view rest, bool flow_control_enabled auto [ptr, ec] = std::from_chars(baudrate_str.data(), baudrate_str.data() + baudrate_str.size(), value); - if (static_cast(ec)) { + // Reject trailing garbage (e.g. "57600xyz"); from_chars would otherwise accept it. + if (static_cast(ec) || ptr != baudrate_str.data() + baudrate_str.size()) { return {}; } diff --git a/src/mavsdk/core/curl_wrapper.cpp b/src/mavsdk/core/curl_wrapper.cpp index 8c99cbaba3..53d20ade65 100644 --- a/src/mavsdk/core/curl_wrapper.cpp +++ b/src/mavsdk/core/curl_wrapper.cpp @@ -103,6 +103,13 @@ bool CurlWrapper::download_file_to_path( progress.progress_callback = progress_callback; fp = fopen(path.c_str(), "wb"); + if (fp == nullptr) { + LogErr() << "Error: cannot open file for writing: " << path; + if (nullptr != progress_callback) { + progress_callback(0, HttpStatus::Error, CURLcode::CURLE_WRITE_ERROR); + } + return false; + } curl_easy_setopt(curl.get(), CURLOPT_CONNECTTIMEOUT, 5L); curl_easy_setopt(curl.get(), CURLOPT_XFERINFOFUNCTION, download_progress_update); curl_easy_setopt(curl.get(), CURLOPT_PROGRESSDATA, &progress); diff --git a/src/mavsdk/core/fs_utils.cpp b/src/mavsdk/core/fs_utils.cpp index 9c8f8f49fd..10e0e70af3 100644 --- a/src/mavsdk/core/fs_utils.cpp +++ b/src/mavsdk/core/fs_utils.cpp @@ -70,17 +70,17 @@ std::optional get_cache_directory() // Read /proc/self/cmdline std::ifstream cmdline("/proc/self/cmdline"); std::string line; - if (std::getline(cmdline, line)) { - // line might have a trailing \0 - if (line.length() > 0 && *line.end() == 0) { - line.pop_back(); - } + // /proc/self/cmdline stores the package name followed by a NUL byte, which we want to exclude + if (std::getline(cmdline, line, '\0')) { return "/data/data/" + line + "/mavsdk_cache"; } #elif defined(APPLE) || defined(LINUX) const char* homedir; if ((homedir = getenv("HOME")) == NULL) { - homedir = getpwuid(getuid())->pw_dir; + // getpwuid() can return NULL if there is no passwd entry for the uid (common in + // minimal containers), so guard against dereferencing it. + const struct passwd* pw = getpwuid(getuid()); + homedir = (pw != nullptr) ? pw->pw_dir : nullptr; } if (!homedir) { return std::nullopt; diff --git a/src/mavsdk/core/hostname_to_ip.cpp b/src/mavsdk/core/hostname_to_ip.cpp index d008211a65..f45b07e1e5 100644 --- a/src/mavsdk/core/hostname_to_ip.cpp +++ b/src/mavsdk/core/hostname_to_ip.cpp @@ -41,20 +41,21 @@ std::optional resolve_hostname_to_ip(const std::string& hostname) return {}; } - std::string ipAddress; + std::optional ipAddress; for (addrinfo* ptr = result; ptr != nullptr; ptr = ptr->ai_next) { sockaddr_in* sockaddrIpv4 = reinterpret_cast(ptr->ai_addr); - char ipStr[INET_ADDRSTRLEN]; - inet_ntop(AF_INET, &(sockaddrIpv4->sin_addr), ipStr, INET_ADDRSTRLEN); - ipAddress = ipStr; - break; // Take the first result + char ipStr[INET_ADDRSTRLEN] = {}; + if (inet_ntop(AF_INET, &(sockaddrIpv4->sin_addr), ipStr, INET_ADDRSTRLEN) != nullptr) { + ipAddress = ipStr; + break; // Take the first result + } } freeaddrinfo(result); #if defined(WINDOWS) WSACleanup(); #endif - return {ipAddress}; + return ipAddress; } } // namespace mavsdk diff --git a/src/mavsdk/core/mavlink_component_metadata.cpp b/src/mavsdk/core/mavlink_component_metadata.cpp index b69aa7116b..f62a1ce125 100644 --- a/src/mavsdk/core/mavlink_component_metadata.cpp +++ b/src/mavsdk/core/mavlink_component_metadata.cpp @@ -5,6 +5,7 @@ #include "unused.h" #include "system_impl.h" +#include #include #include #include @@ -136,7 +137,16 @@ bool MavlinkComponentMetadata::uri_is_mavlinkftp( std::regex compid_regex(R"(^(?:\[\;comp\=(\d+)\])(.*))"); std::smatch match; if (std::regex_search(download_path, match, compid_regex)) { - target_compid = std::stoi(match[1]); + // Parse the component id without throwing (MAVSDK builds with -fno-exceptions, + // so std::stoi would terminate() on an out-of-range value from the wire). + const std::string compid_str = match[1].str(); + char* end = nullptr; + const long parsed = std::strtol(compid_str.c_str(), &end, 10); + if (end == compid_str.c_str() || *end != '\0' || parsed < 0 || parsed > 255) { + LogWarn() << "Ignoring invalid component id in metadata uri: " << compid_str; + return false; + } + target_compid = static_cast(parsed); download_path = match[2]; } return true; diff --git a/src/mavsdk/core/mavlink_message_handler.cpp b/src/mavsdk/core/mavlink_message_handler.cpp index c89621d385..3a63b78e8b 100644 --- a/src/mavsdk/core/mavlink_message_handler.cpp +++ b/src/mavsdk/core/mavlink_message_handler.cpp @@ -58,6 +58,22 @@ void MavlinkMessageHandler::unregister_all_blocking(const void* cookie) { // Blocking version for use in destructors - waits for any in-flight callbacks to complete. // WARNING: Do NOT call this from within a message handler callback - it will deadlock. + + // Also purge any deferred registrations for this cookie from _register_later_table. + // If register_one() was called while process_message() held _mutex, the entries land in + // _register_later_table instead of _table. Without this step, check_register_later() + // would later promote those stale entries into _table and fire callbacks on a + // destroyed object (heap-use-after-free). + { + std::lock_guard register_later_lock(_register_later_mutex); + _register_later_table.erase( + std::remove_if( + _register_later_table.begin(), + _register_later_table.end(), + [&](const auto& entry) { return entry.cookie == cookie; }), + _register_later_table.end()); + } + std::lock_guard lock(_mutex); _table.erase( std::remove_if( diff --git a/src/mavsdk/core/param_value.cpp b/src/mavsdk/core/param_value.cpp index ebe3c42ca0..6f98d4eebd 100644 --- a/src/mavsdk/core/param_value.cpp +++ b/src/mavsdk/core/param_value.cpp @@ -182,7 +182,7 @@ bool ParamValue::set_from_mavlink_param_ext_set(const mavlink_param_ext_set_t& m _value = temp; } break; case MAV_PARAM_EXT_TYPE_CUSTOM: { - std::size_t len = std::min(std::size_t(128), strlen(mavlink_ext_set.param_value)); + std::size_t len = strnlen(mavlink_ext_set.param_value, 128); _value = std::string(mavlink_ext_set.param_value, mavlink_ext_set.param_value + len); } break; default: From 980237d2f6e4ad433a7158a13deb20c70061cd13 Mon Sep 17 00:00:00 2001 From: Julian Oes Date: Thu, 16 Jul 2026 10:40:34 +1200 Subject: [PATCH 3/9] core: backport request-message/mission/libmav bugfixes from main Backports the following fixes from main to v3: - 62c60ac77 core: fix message matching in MavlinkRequestMessage::handle_any_message - 020777a05 core: fix deleting of callback on rapid requests of messages (#2894) - c863cdfa2 core: cancel pending timeouts in MavlinkRequestMessage destructor - 89e663dc0 core: lock _system_ids when inserting from the libmav receive path - fa747afe core: ignore duplicate/out-of-order mission items on the server - 873722970 core: reindex non-extended parameter view contiguously - 21cba79b4 core: enlarge libmav accumulation buffer to avoid dropping messages --- src/mavsdk/core/connection.cpp | 3 +- src/mavsdk/core/libmav_receiver.h | 13 +++-- .../core/mavlink_mission_transfer_server.cpp | 7 +++ src/mavsdk/core/mavlink_parameter_cache.cpp | 9 ++++ src/mavsdk/core/mavlink_request_message.cpp | 52 ++++++++++++------- src/mavsdk/core/mavlink_request_message.h | 8 ++- 6 files changed, 68 insertions(+), 24 deletions(-) diff --git a/src/mavsdk/core/connection.cpp b/src/mavsdk/core/connection.cpp index 8d7e12bcb5..0807e2c68e 100644 --- a/src/mavsdk/core/connection.cpp +++ b/src/mavsdk/core/connection.cpp @@ -78,7 +78,8 @@ void Connection::receive_libmav_message( const Mavsdk::MavlinkMessage& message, Connection* connection) { // Register system ID when receiving a message from a new system. - if (_system_ids.find(message.system_id) == _system_ids.end()) { + { + std::lock_guard lock(_system_ids_mutex); _system_ids.insert(message.system_id); } diff --git a/src/mavsdk/core/libmav_receiver.h b/src/mavsdk/core/libmav_receiver.h index 0f43db0dbb..819355b8cb 100644 --- a/src/mavsdk/core/libmav_receiver.h +++ b/src/mavsdk/core/libmav_receiver.h @@ -59,9 +59,16 @@ class LibmavReceiver { Mavsdk::MavlinkMessage _last_message; std::optional _last_libmav_message; // Separate libmav message for integration - // Accumulation buffer for serial connections where messages can span multiple reads - // Buffer size is max message size (280 bytes: 255 payload + 10 header + 2 CRC + 13 signature) - static constexpr size_t ACCUMULATION_BUFFER_SIZE = mav::MessageDefinition::MAX_MESSAGE_SIZE; + // Accumulation buffer for connections where messages can span multiple reads. + // + // This bound is only a safety valve against unbounded growth; the parser normally + // drains the buffer (it consumes complete messages and skips garbage before the magic + // byte), so at most one partial message is ever pending. It must therefore be at least + // as large as a single read (the 2048-byte connection receive buffers) plus one full + // message, otherwise set_new_datagram() would drop the front of a large read *before* + // parsing and silently lose the messages in it. + static constexpr size_t ACCUMULATION_BUFFER_SIZE = + 2048 + mav::MessageDefinition::MAX_MESSAGE_SIZE; std::vector _accumulation_buffer; bool _debugging = false; diff --git a/src/mavsdk/core/mavlink_mission_transfer_server.cpp b/src/mavsdk/core/mavlink_mission_transfer_server.cpp index 12df22ef2a..e99990146f 100644 --- a/src/mavsdk/core/mavlink_mission_transfer_server.cpp +++ b/src/mavsdk/core/mavlink_mission_transfer_server.cpp @@ -279,6 +279,13 @@ void MavlinkMissionTransferServer::ReceiveIncomingMission::process_mission_item_ mavlink_mission_item_int_t item_int; mavlink_msg_mission_item_int_decode(&message, &item_int); + // Ignore duplicate or out-of-order items. Without this, a peer streaming items + // (e.g. all with seq 0) would grow _items without bound while refreshing the + // timeout, so the transfer would never time out. + if (_next_sequence != item_int.seq) { + return; + } + _items.push_back(ItemInt{ item_int.seq, item_int.frame, diff --git a/src/mavsdk/core/mavlink_parameter_cache.cpp b/src/mavsdk/core/mavlink_parameter_cache.cpp index d45617906b..dd3874fd2c 100644 --- a/src/mavsdk/core/mavlink_parameter_cache.cpp +++ b/src/mavsdk/core/mavlink_parameter_cache.cpp @@ -56,6 +56,15 @@ MavlinkParameterCache::all_parameters(bool including_extended) const std::back_inserter(params_without_extended), [](auto& entry) { return !entry.value.needs_extended(); }); + // The non-extended protocol only advertises these parameters, so their indices + // must be contiguous 0..N-1 to stay consistent with count(false). Otherwise, when + // extended-only params are interspersed, the stored (full-set) index no longer + // matches the position in this filtered view: param_by_index() would return the + // wrong param (or trip its assert) and broadcasts would send an index >= count. + for (uint16_t i = 0; i < params_without_extended.size(); ++i) { + params_without_extended[i].index = i; + } + return params_without_extended; } } diff --git a/src/mavsdk/core/mavlink_request_message.cpp b/src/mavsdk/core/mavlink_request_message.cpp index bb36dcc78b..c1639014d6 100644 --- a/src/mavsdk/core/mavlink_request_message.cpp +++ b/src/mavsdk/core/mavlink_request_message.cpp @@ -2,6 +2,7 @@ #include "log.h" #include "mavlink_request_message.h" #include "system_impl.h" +#include #include #include @@ -25,6 +26,21 @@ MavlinkRequestMessage::MavlinkRequestMessage( } } +MavlinkRequestMessage::~MavlinkRequestMessage() +{ + // The message handler holds callbacks capturing 'this', so make sure none of + // them can fire after we are gone. + _message_handler.unregister_all_blocking(this); + + // In-flight requests schedule timeouts that also capture 'this'. Cancel any that + // are still pending, otherwise TimeoutHandler could fire handle_timeout on us + // after destruction (use-after-free). + std::lock_guard lock(_mutex); + for (const auto& item : _work_items) { + _timeout_handler.remove(item.timeout_cookie); + } +} + void MavlinkRequestMessage::request( uint32_t message_id, uint8_t target_component, @@ -33,12 +49,6 @@ void MavlinkRequestMessage::request( { std::unique_lock lock(_mutex); - // Cleanup previous requests. - for (const auto id : _deferred_message_cleanup) { - _message_handler.unregister_one(id, this); - } - _deferred_message_cleanup.clear(); - // Respond with 'Busy' if already in progress. for (auto& item : _work_items) { if (item.message_id == message_id && item.param2 == param2 && @@ -57,11 +67,18 @@ void MavlinkRequestMessage::request( // Otherwise, schedule it. _work_items.emplace_back(WorkItem{message_id, target_component, callback, param2}); - // Register for message - _message_handler.register_one( - message_id, - [this](const mavlink_message_t& message) { handle_any_message(message); }, - this); + // Register a handler for this message id if we don't have one yet. We keep + // it registered for our lifetime; handle_any_message is a no-op when no work + // item is waiting, and this avoids the races of registering and + // unregistering per request. + if (std::find(_registered_message_ids.begin(), _registered_message_ids.end(), message_id) == + _registered_message_ids.end()) { + _registered_message_ids.push_back(message_id); + _message_handler.register_one( + message_id, + [this](const mavlink_message_t& message) { handle_any_message(message); }, + this); + } // And send off command send_request(_work_items.back()); @@ -202,9 +219,10 @@ void MavlinkRequestMessage::handle_any_message(const mavlink_message_t& message) std::unique_lock lock(_mutex); for (auto it = _work_items.begin(); it != _work_items.end(); ++it) { - // Check if we're waiting for this message. + // Check if we're waiting for this message. Skip unless both the message id and + // the component it came from match what this work item requested. // TODO: check if params are correct. - if (it->message_id != message.msgid && it->target_component == message.compid) { + if (it->message_id != message.msgid || it->target_component != message.compid) { continue; } @@ -214,10 +232,9 @@ void MavlinkRequestMessage::handle_any_message(const mavlink_message_t& message) // of the command later, we ignore it, and we fake the result to be successful // anyway. auto temp_callback = it->callback; - // We can now get rid of the entry now. + // We can now get rid of the entry now. We keep the message handler + // registered for next time. _work_items.erase(it); - // We have to clean up later outside this callback, otherwise we lock ourselves out. - _deferred_message_cleanup.push_back(message.msgid); lock.unlock(); if (temp_callback) { @@ -262,7 +279,6 @@ void MavlinkRequestMessage::handle_command_result( if (++it->retries > RETRIES) { // We have already retried, let's give up. auto temp_callback = it->callback; - _message_handler.unregister_one(it->message_id, this); _work_items.erase(it); lock.unlock(); if (temp_callback) { @@ -285,7 +301,6 @@ void MavlinkRequestMessage::handle_command_result( // It looks like this did not work, and we can report the error // No need to try again. auto temp_callback = it->callback; - _message_handler.unregister_one(it->message_id, this); _work_items.erase(it); lock.unlock(); if (temp_callback) { @@ -314,7 +329,6 @@ void MavlinkRequestMessage::handle_timeout(uint32_t message_id, uint8_t target_c if (++it->retries > RETRIES) { // We have already retried, let's give up. auto temp_callback = it->callback; - _message_handler.unregister_one(it->message_id, this); _work_items.erase(it); lock.unlock(); if (temp_callback) { diff --git a/src/mavsdk/core/mavlink_request_message.h b/src/mavsdk/core/mavlink_request_message.h index d755d76ccd..4b116eb491 100644 --- a/src/mavsdk/core/mavlink_request_message.h +++ b/src/mavsdk/core/mavlink_request_message.h @@ -4,6 +4,7 @@ #include "mavlink_message_handler.h" #include "timeout_handler.h" #include "mavlink_include.h" +#include #include #include #include @@ -20,6 +21,7 @@ class MavlinkRequestMessage { MavlinkCommandSender& command_sender, MavlinkMessageHandler& message_handler, TimeoutHandler& timeout_handler); + ~MavlinkRequestMessage(); MavlinkRequestMessage() = delete; using MavlinkRequestMessageCallback = @@ -58,7 +60,11 @@ class MavlinkRequestMessage { std::mutex _mutex{}; std::vector _work_items{}; - std::vector _deferred_message_cleanup{}; + // Message ids we have registered a handler for. We register lazily on first + // request and keep the handler until destruction, so there is never an + // unregister racing a register. This stays tiny (a handful of message ids), + // so a flat vector is faster than a node-based set. + std::vector _registered_message_ids{}; bool _debugging{false}; }; From 278899a7027b81325f7fed4a0f221a5ef57ab3f1 Mon Sep 17 00:00:00 2001 From: Julian Oes Date: Thu, 16 Jul 2026 10:40:57 +1200 Subject: [PATCH 4/9] core: backport parameter-server bugfixes from main Backports the following fixes from main to v3: - 845f382a4 core: send PARAM_ERROR for missing/wrong-type non-extended PARAM_SET - dfac8d4c8 param_server: allow updating an already-provided param after lockdown (#2916) - 673cecdb0 core: notify param value changes on every used protocol (#2915) - 3e70e8923 core: don't invoke param subscription callbacks under _all_params_mutex Note: main uses asio::post to defer the value-change broadcast/callbacks; on v3 this is re-expressed with the existing _work_queue and by invoking subscription callbacks after releasing _all_params_mutex. --- src/mavsdk/core/mavlink_parameter_server.cpp | 86 ++++++++++++++++--- src/mavsdk/core/mavlink_parameter_server.h | 13 ++- .../param_server/param_server_impl.cpp | 10 +-- 3 files changed, 86 insertions(+), 23 deletions(-) diff --git a/src/mavsdk/core/mavlink_parameter_server.cpp b/src/mavsdk/core/mavlink_parameter_server.cpp index 0d53982df3..9e2257e595 100644 --- a/src/mavsdk/core/mavlink_parameter_server.cpp +++ b/src/mavsdk/core/mavlink_parameter_server.cpp @@ -83,6 +83,15 @@ MavlinkParameterServer::provide_server_param(const std::string& name, const Para } } std::lock_guard lock(_all_params_mutex); + + // Updating the value of an already-provided parameter is always allowed. Adding a new + // parameter is only possible until the set has been locked down, i.e. until a client has + // enumerated the parameters and thereby fixed the indices and count. + const bool already_exists = _param_cache.param_by_id(name, true).has_value(); + if (!already_exists && _params_locked_down) { + return Result::ParamProvidedTooLate; + } + // first we try to add it as a new parameter switch (_param_cache.add_new_param(name, param_value)) { case MavlinkParameterCache::AddNewParamResult::Ok: @@ -92,15 +101,24 @@ MavlinkParameterServer::provide_server_param(const std::string& name, const Para switch (_param_cache.update_existing_param(name, param_value)) { case MavlinkParameterCache::UpdateExistingParamResult::Ok: find_and_call_subscriptions_value_changed(name, param_value); - { - auto new_work = std::make_shared( - name, - param_value, - WorkItemValue{ - std::numeric_limits::max(), - std::numeric_limits::max(), - _last_extended}); - _work_queue.push_back(new_work); + // Notify clients of the changed value on the protocol(s) that can carry it and + // that a client is actually using. The extended protocol can represent any + // parameter; the non-extended protocol only those that don't need_extended. + if (!_seen_extended && !_seen_non_extended) { + // No request seen yet, so we can't tell which protocol a client uses. + // Fall back to notifying on every protocol the parameter supports to avoid + // silently dropping the change. + enqueue_value_broadcast(name, param_value, true); + if (!param_value.needs_extended()) { + enqueue_value_broadcast(name, param_value, false); + } + } else { + if (_seen_extended) { + enqueue_value_broadcast(name, param_value, true); + } + if (_seen_non_extended && !param_value.needs_extended()) { + enqueue_value_broadcast(name, param_value, false); + } } return Result::OkExistsAlready; case MavlinkParameterCache::UpdateExistingParamResult::MissingParam: @@ -203,8 +221,13 @@ void MavlinkParameterServer::process_param_set_internally( uint8_t error_code = 0; bool send_error = false; + bool notify_value_changed = false; + std::string changed_param_id; + ParamValue changed_param_value; + { std::lock_guard lock(_all_params_mutex); + mark_protocol_seen(extended); // for checking if the update actually changed the value const auto opt_before_update = _param_cache.param_by_id(param_id, extended); const auto result = _param_cache.update_existing_param(param_id, value_to_set); @@ -223,7 +246,7 @@ void MavlinkParameterServer::process_param_set_internally( error_code = 1; // MAV_PARAM_ERROR_DOES_NOT_EXIST send_error = true; } - return; + break; } case MavlinkParameterCache::UpdateExistingParamResult::WrongType: { // Non-extended: send PARAM_ERROR with TYPE_MISMATCH @@ -245,7 +268,7 @@ void MavlinkParameterServer::process_param_set_internally( error_code = 7; // MAV_PARAM_ERROR_TYPE_MISMATCH send_error = true; } - return; + break; } case MavlinkParameterCache::UpdateExistingParamResult::Ok: { LogWarn() << "Update existing params!"; @@ -257,8 +280,12 @@ void MavlinkParameterServer::process_param_set_internally( LogDebug() << "Update had no effect: " << updated_parameter.value; } else { LogDebug() << "Updated param to :" << updated_parameter.value; - find_and_call_subscriptions_value_changed( - updated_parameter.id, updated_parameter.value); + // Don't call the (user) subscription callbacks while holding + // _all_params_mutex: a callback re-entering a server API that takes the + // same mutex would self-deadlock. Defer until the lock is released. + notify_value_changed = true; + changed_param_id = updated_parameter.id; + changed_param_value = updated_parameter.value; } if (extended) { auto new_work = std::make_shared( @@ -277,6 +304,11 @@ void MavlinkParameterServer::process_param_set_internally( } } + // Call the (user) subscription callbacks outside the lock if needed. + if (notify_value_changed) { + find_and_call_subscriptions_value_changed(changed_param_id, changed_param_value); + } + // Send PARAM_ERROR outside the lock if needed if (send_error) { send_param_error(error_param_id, error_param_index, error_code); @@ -399,6 +431,7 @@ void MavlinkParameterServer::internal_process_param_request_read_by_id( { { std::lock_guard lock(_all_params_mutex); + mark_protocol_seen(extended); const auto param_opt = _param_cache.param_by_id(id, extended); if (!param_opt.has_value()) { @@ -413,7 +446,6 @@ void MavlinkParameterServer::internal_process_param_request_read_by_id( param.id, param.value, WorkItemValue{param.index, param_count, extended}); _work_queue.push_back(new_work); - _last_extended = extended; return; } } @@ -429,6 +461,7 @@ void MavlinkParameterServer::internal_process_param_request_read_by_index( { { std::lock_guard lock(_all_params_mutex); + mark_protocol_seen(extended); const auto param_opt = _param_cache.param_by_index(index, extended); if (!param_opt.has_value()) { @@ -482,6 +515,7 @@ void MavlinkParameterServer::process_param_ext_request_list(const mavlink_messag void MavlinkParameterServer::broadcast_all_parameters(const bool extended) { std::lock_guard lock(_all_params_mutex); + mark_protocol_seen(extended); // Param used with index, we should no longer change the index _params_locked_down = true; @@ -618,6 +652,28 @@ void MavlinkParameterServer::send_param_error( } } +void MavlinkParameterServer::mark_protocol_seen(bool extended) +{ + if (extended) { + _seen_extended = true; + } else { + _seen_non_extended = true; + } +} + +void MavlinkParameterServer::enqueue_value_broadcast( + const std::string& name, const ParamValue& param_value, bool extended) +{ + auto new_work = std::make_shared( + name, + param_value, + WorkItemValue{ + std::numeric_limits::max(), + std::numeric_limits::max(), + extended}); + _work_queue.push_back(new_work); +} + std::ostream& operator<<(std::ostream& str, const MavlinkParameterServer::Result& result) { switch (result) { @@ -633,6 +689,8 @@ std::ostream& operator<<(std::ostream& str, const MavlinkParameterServer::Result return str << "NotFound"; case MavlinkParameterServer::Result::ParamValueTooLong: return str << ":ParamValueTooLong"; + case MavlinkParameterServer::Result::ParamProvidedTooLate: + return str << "ParamProvidedTooLate"; default: return str << "UnknownError"; } diff --git a/src/mavsdk/core/mavlink_parameter_server.h b/src/mavsdk/core/mavlink_parameter_server.h index cf09dea452..22fd8a9b37 100644 --- a/src/mavsdk/core/mavlink_parameter_server.h +++ b/src/mavsdk/core/mavlink_parameter_server.h @@ -41,6 +41,7 @@ class MavlinkParameterServer : public MavlinkParameterSubscription { ParamValueTooLong, TooManyParams, ParamNotFound, + ParamProvidedTooLate, Unknown, ValueOutOfRange, PermissionDenied, @@ -93,6 +94,14 @@ class MavlinkParameterServer : public MavlinkParameterSubscription { void send_param_error(const std::string& param_id, int16_t param_index, uint8_t error_code); + // Remember which parameter protocol(s) clients have actually used, so that a spontaneous + // value-change notification is only sent on protocols someone is listening on. + // Must be called with _all_params_mutex held. + void mark_protocol_seen(bool extended); + // Enqueue a spontaneous PARAM_VALUE / PARAM_EXT_VALUE broadcast for a changed value. + void + enqueue_value_broadcast(const std::string& name, const ParamValue& param_value, bool extended); + static std::variant extract_request_read_param_identifier(int16_t param_index, const char* param_id); @@ -130,7 +139,9 @@ class MavlinkParameterServer : public MavlinkParameterSubscription { bool _extended_protocol = true; bool _parameter_debugging = false; - bool _last_extended = true; + // Sticky flags tracking which protocol(s) clients have used (guarded by _all_params_mutex). + bool _seen_extended = false; + bool _seen_non_extended = false; bool _params_locked_down = false; }; diff --git a/src/mavsdk/plugins/param_server/param_server_impl.cpp b/src/mavsdk/plugins/param_server/param_server_impl.cpp index 8c3351d77a..cec91e6f9e 100644 --- a/src/mavsdk/plugins/param_server/param_server_impl.cpp +++ b/src/mavsdk/plugins/param_server/param_server_impl.cpp @@ -64,10 +64,6 @@ ParamServer::Result ParamServerImpl::provide_param_int(std::string name, int32_t return ParamServer::Result::ParamNameTooLong; } - if (_server_component_impl->mavlink_parameter_server().params_locked_down()) { - return ParamServer::Result::ParamProvidedTooLate; - } - const auto ret = _server_component_impl->mavlink_parameter_server().provide_server_param_int(name, value); if (ret == MavlinkParameterServer::Result::Ok) { @@ -97,10 +93,6 @@ ParamServer::Result ParamServerImpl::provide_param_float(std::string name, float return ParamServer::Result::ParamNameTooLong; } - if (_server_component_impl->mavlink_parameter_server().params_locked_down()) { - return ParamServer::Result::ParamProvidedTooLate; - } - const auto ret = _server_component_impl->mavlink_parameter_server().provide_server_param_float(name, value); if (ret == MavlinkParameterServer::Result::Ok) { @@ -216,6 +208,8 @@ ParamServerImpl::result_from_mavlink_parameter_server_result(MavlinkParameterSer return ParamServer::Result::WrongType; case MavlinkParameterServer::Result::ParamValueTooLong: return ParamServer::Result::ParamValueTooLong; + case MavlinkParameterServer::Result::ParamProvidedTooLate: + return ParamServer::Result::ParamProvidedTooLate; default: LogErr() << "Unknown param error"; return ParamServer::Result::Unknown; From 6754f031cf7ff7e85466d742ad8b5a1c0171b10f Mon Sep 17 00:00:00 2001 From: Julian Oes Date: Thu, 16 Jul 2026 10:40:58 +1200 Subject: [PATCH 5/9] core: backport handler-list/configuration race fixes from main Backports the following fixes from main to v3: - cffa8f6dc core: fix erase-remove idiom in unregister_statustext_handler - 67d463101 core: synchronize statustext and param-changed handler lists - 6c643aaae core: guard MavsdkImpl::_configuration with a dedicated mutex - 79534d261 core: capture callback watchdog fields by value to avoid data race Note: main's asio-executor confinement does not apply to v3; the underlying races are fixed here with dedicated std::mutex members instead. --- src/mavsdk/core/mavsdk_impl.cpp | 62 ++++++++++++++++++++++++--------- src/mavsdk/core/mavsdk_impl.h | 5 +++ src/mavsdk/core/system_impl.cpp | 37 ++++++++++++++++---- src/mavsdk/core/system_impl.h | 2 ++ 4 files changed, 82 insertions(+), 24 deletions(-) diff --git a/src/mavsdk/core/mavsdk_impl.cpp b/src/mavsdk/core/mavsdk_impl.cpp index fd65b5ee10..b8a381b50c 100644 --- a/src/mavsdk/core/mavsdk_impl.cpp +++ b/src/mavsdk/core/mavsdk_impl.cpp @@ -210,7 +210,7 @@ std::shared_ptr MavsdkImpl::server_component(unsigned instance) { std::lock_guard lock(_mutex); - auto component_type = _configuration.get_component_type(); + auto component_type = get_component_type(); switch (component_type) { case ComponentType::Autopilot: case ComponentType::GroundStation: @@ -508,8 +508,8 @@ void MavsdkImpl::process_message(mavlink_message_t& message, Connection* connect // examples and integration tests) to connect to QGroundControl by accident // instead of PX4 because the check `has_autopilot()` is not used. - if (_configuration.get_component_type() == ComponentType::GroundStation && - message.sysid == 255 && message.compid == MAV_COMP_ID_MISSIONPLANNER) { + if (get_component_type() == ComponentType::GroundStation && message.sysid == 255 && + message.compid == MAV_COMP_ID_MISSIONPLANNER) { if (_message_logging_on) { LogDebug() << "Ignoring messages from QGC as we are also a ground station"; } @@ -604,8 +604,8 @@ void MavsdkImpl::process_libmav_message( } // Filter out QGroundControl messages similar to regular mavlink processing - if (_configuration.get_component_type() == ComponentType::GroundStation && - message.system_id == 255 && message.component_id == MAV_COMP_ID_MISSIONPLANNER) { + if (get_component_type() == ComponentType::GroundStation && message.system_id == 255 && + message.component_id == MAV_COMP_ID_MISSIONPLANNER) { if (_message_logging_on) { LogDebug() << "Ignoring libmav messages from QGC as we are also a ground station"; } @@ -1037,10 +1037,16 @@ void MavsdkImpl::remove_connection(Mavsdk::ConnectionHandle handle) Mavsdk::Configuration MavsdkImpl::get_configuration() const { - std::lock_guard configuration_lock(_mutex); + std::lock_guard configuration_lock(_configuration_mutex); return _configuration; } +ComponentType MavsdkImpl::get_component_type() const +{ + std::lock_guard configuration_lock(_configuration_mutex); + return _configuration.get_component_type(); +} + void MavsdkImpl::set_configuration(Mavsdk::Configuration new_configuration) { std::lock_guard server_components_lock(_server_components_mutex); @@ -1050,16 +1056,23 @@ void MavsdkImpl::set_configuration(Mavsdk::Configuration new_configuration) _default_server_component = server_component_by_id_with_lock( new_configuration.get_component_id(), new_configuration.get_mav_type()); - if (new_configuration.get_always_send_heartbeats() && - !_configuration.get_always_send_heartbeats()) { + const bool was_always_sending_heartbeats = [this] { + std::lock_guard configuration_lock(_configuration_mutex); + return _configuration.get_always_send_heartbeats(); + }(); + + if (new_configuration.get_always_send_heartbeats() && !was_always_sending_heartbeats) { start_sending_heartbeats(); } else if ( - !new_configuration.get_always_send_heartbeats() && - _configuration.get_always_send_heartbeats() && !is_any_system_connected()) { + !new_configuration.get_always_send_heartbeats() && was_always_sending_heartbeats && + !is_any_system_connected()) { stop_sending_heartbeats(); } - _configuration = new_configuration; + { + std::lock_guard configuration_lock(_configuration_mutex); + _configuration = new_configuration; + } // We cache these values as atomic to avoid having to lock any mutex for them. _our_system_id = new_configuration.get_system_id(); _our_component_id = new_configuration.get_component_id(); @@ -1077,16 +1090,19 @@ uint8_t MavsdkImpl::get_own_component_id() const uint8_t MavsdkImpl::get_mav_type() const { + std::lock_guard configuration_lock(_configuration_mutex); return _configuration.get_mav_type(); } Autopilot MavsdkImpl::get_autopilot() const { + std::lock_guard configuration_lock(_configuration_mutex); return _configuration.get_autopilot(); } uint8_t MavsdkImpl::get_mav_autopilot() const { + std::lock_guard configuration_lock(_configuration_mutex); switch (_configuration.get_autopilot()) { case Autopilot::Px4: return MAV_AUTOPILOT_PX4; @@ -1100,12 +1116,18 @@ uint8_t MavsdkImpl::get_mav_autopilot() const CompatibilityMode MavsdkImpl::get_compatibility_mode() const { + std::lock_guard configuration_lock(_configuration_mutex); return _configuration.get_compatibility_mode(); } Autopilot MavsdkImpl::effective_autopilot(Autopilot detected) const { - switch (_configuration.get_compatibility_mode()) { + CompatibilityMode compatibility_mode; + { + std::lock_guard configuration_lock(_configuration_mutex); + compatibility_mode = _configuration.get_compatibility_mode(); + } + switch (compatibility_mode) { case CompatibilityMode::Auto: return detected; case CompatibilityMode::Pure: @@ -1342,12 +1364,14 @@ void MavsdkImpl::process_user_callbacks_thread() } const double timeout_s = 1.0; + // Capture the fields we need by value: this watchdog runs on the io thread and + // can fire while (or just after) callback.func() runs here, so referencing the + // loop-local 'callback' would race with its destruction at the next iteration. auto cookie = timeout_handler.add( - [&]() { + [this, timeout_s, filename = callback.filename, linenumber = callback.linenumber]() { if (_callback_debugging) { - LogWarn() << "Callback called from " << callback.filename << ":" - << callback.linenumber << " took more than " << timeout_s - << " second to run."; + LogWarn() << "Callback called from " << filename << ":" << linenumber + << " took more than " << timeout_s << " second to run."; fflush(stdout); fflush(stderr); abort(); @@ -1394,7 +1418,11 @@ void MavsdkImpl::start_sending_heartbeats() void MavsdkImpl::stop_sending_heartbeats() { - if (!_configuration.get_always_send_heartbeats()) { + const bool always_send_heartbeats = [this] { + std::lock_guard configuration_lock(_configuration_mutex); + return _configuration.get_always_send_heartbeats(); + }(); + if (!always_send_heartbeats) { std::lock_guard lock(_heartbeat_mutex); call_every_handler.remove(_heartbeat_send_cookie); } diff --git a/src/mavsdk/core/mavsdk_impl.h b/src/mavsdk/core/mavsdk_impl.h index 1e53063e35..95080b3b1c 100644 --- a/src/mavsdk/core/mavsdk_impl.h +++ b/src/mavsdk/core/mavsdk_impl.h @@ -70,6 +70,7 @@ class MavsdkImpl { void set_configuration(Mavsdk::Configuration new_configuration); Mavsdk::Configuration get_configuration() const; + ComponentType get_component_type() const; bool send_message(mavlink_message_t& message); uint8_t get_own_system_id() const; @@ -239,6 +240,10 @@ class MavsdkImpl { CallbackList<> _new_system_callbacks{}; + // Leaf mutex guarding only _configuration. It is never held while acquiring + // another mutex, so it cannot participate in a lock-order inversion with + // _mutex / _server_components_mutex. + mutable std::mutex _configuration_mutex{}; Mavsdk::Configuration _configuration{ComponentType::GroundStation}; std::atomic _our_system_id{0}; std::atomic _our_component_id{0}; diff --git a/src/mavsdk/core/system_impl.cpp b/src/mavsdk/core/system_impl.cpp index cce9b2f869..016e4121c5 100644 --- a/src/mavsdk/core/system_impl.cpp +++ b/src/mavsdk/core/system_impl.cpp @@ -296,15 +296,19 @@ void SystemImpl::remove_call_every(CallEveryHandler::Cookie cookie) void SystemImpl::register_statustext_handler( std::function callback, void* cookie) { + std::lock_guard lock(_statustext_handler_callbacks_mutex); _statustext_handler_callbacks.push_back(StatustextCallback{std::move(callback), cookie}); } void SystemImpl::unregister_statustext_handler(void* cookie) { - _statustext_handler_callbacks.erase(std::remove_if( - _statustext_handler_callbacks.begin(), - _statustext_handler_callbacks.end(), - [&](const auto& entry) { return entry.cookie == cookie; })); + std::lock_guard lock(_statustext_handler_callbacks_mutex); + _statustext_handler_callbacks.erase( + std::remove_if( + _statustext_handler_callbacks.begin(), + _statustext_handler_callbacks.end(), + [&](const auto& entry) { return entry.cookie == cookie; }), + _statustext_handler_callbacks.end()); } void SystemImpl::process_heartbeat(const mavlink_message_t& message) @@ -360,7 +364,14 @@ void SystemImpl::process_statustext(const mavlink_message_t& message) << MavlinkStatustextHandler::severity_str(maybe_result.value().severity) << ": " << maybe_result.value().text; - for (const auto& entry : _statustext_handler_callbacks) { + // Copy the callbacks out under the lock and invoke them afterwards, so a + // callback that (un)registers a handler can't deadlock or invalidate the vector. + std::vector callbacks_copy; + { + std::lock_guard lock(_statustext_handler_callbacks_mutex); + callbacks_copy = _statustext_handler_callbacks; + } + for (const auto& entry : callbacks_copy) { entry.callback(maybe_result.value()); } } @@ -1359,8 +1370,18 @@ void SystemImpl::call_user_callback_located( void SystemImpl::param_changed(const std::string& name) { - for (auto& callback : _param_changed_callbacks) { - callback.second(name); + // Copy the callbacks out under the lock and invoke them afterwards, so a callback + // that (un)registers a handler can't deadlock or invalidate the map. + std::vector callbacks_copy; + { + std::lock_guard lock(_param_changed_callbacks_mutex); + callbacks_copy.reserve(_param_changed_callbacks.size()); + for (const auto& callback : _param_changed_callbacks) { + callbacks_copy.push_back(callback.second); + } + } + for (const auto& callback : callbacks_copy) { + callback(name); } } @@ -1377,11 +1398,13 @@ void SystemImpl::register_param_changed_handler( return; } + std::lock_guard lock(_param_changed_callbacks_mutex); _param_changed_callbacks[cookie] = callback; } void SystemImpl::unregister_param_changed_handler(const void* cookie) { + std::lock_guard lock(_param_changed_callbacks_mutex); auto it = _param_changed_callbacks.find(cookie); if (it == _param_changed_callbacks.end()) { LogWarn() << "param_changed_handler for cookie not found"; diff --git a/src/mavsdk/core/system_impl.h b/src/mavsdk/core/system_impl.h index f2f16bfb87..e48e686a0a 100644 --- a/src/mavsdk/core/system_impl.h +++ b/src/mavsdk/core/system_impl.h @@ -396,6 +396,7 @@ class SystemImpl { void* cookie; }; std::vector _statustext_handler_callbacks; + std::mutex _statustext_handler_callbacks_mutex{}; std::atomic _armed{false}; std::atomic _hitl_enabled{false}; @@ -440,6 +441,7 @@ class SystemImpl { std::unordered_set _components{}; std::unordered_map _param_changed_callbacks{}; + std::mutex _param_changed_callbacks_mutex{}; MAV_TYPE _vehicle_type{MAV_TYPE::MAV_TYPE_GENERIC}; bool _vehicle_type_set{false}; From ce8a492615c955b2259c9c976ec613909708bec8 Mon Sep 17 00:00:00 2001 From: Julian Oes Date: Thu, 16 Jul 2026 10:40:59 +1200 Subject: [PATCH 6/9] plugins: backport camera/mavlink_direct/log_streaming bugfixes from main Backports the following fixes from main to v3: - 546cd8e44 camera_server: fix deadlock between send_capture_status and send_tracking_status_with_interval (#2893) - c37178574 Join sending_tracking_status thread before destruction (#2912) - 35686f126 mavlink_direct: fix setting target_component (#2919) - 115511644 fix: clear MavlinkDirect callbacks on deinit to avoid teardown race (#2869) - fd43ce5f9 log_streaming: fix backend teardown race - 85e7dc671 fix(mavlink_passthrough): export operator<< for Result with MAVSDK_PUBLIC (#2881) --- .../camera_server/camera_server_impl.cpp | 92 +++++++++++-------- .../log_streaming_backend_ardupilot.cpp | 5 +- .../log_streaming_backend_px4.cpp | 5 +- .../log_streaming/log_streaming_impl.cpp | 38 +++++--- .../mavlink_direct/mavlink_direct_impl.cpp | 10 +- .../mavlink_passthrough/mavlink_passthrough.h | 3 +- 6 files changed, 99 insertions(+), 54 deletions(-) diff --git a/src/mavsdk/plugins/camera_server/camera_server_impl.cpp b/src/mavsdk/plugins/camera_server/camera_server_impl.cpp index 1e6e3e64df..fb783a7e02 100644 --- a/src/mavsdk/plugins/camera_server/camera_server_impl.cpp +++ b/src/mavsdk/plugins/camera_server/camera_server_impl.cpp @@ -16,6 +16,7 @@ CameraServerImpl::CameraServerImpl(std::shared_ptr server_compo CameraServerImpl::~CameraServerImpl() { + stop_sending_tracking_status(); _server_component_impl->unregister_plugin(this); } @@ -1369,35 +1370,45 @@ std::optional CameraServerImpl::process_camera_capture_st void CameraServerImpl::send_capture_status() { - std::lock_guard lg{_mutex}; - uint8_t image_status{}; - if (_capture_status.image_status == - CameraServer::CaptureStatus::ImageStatus::CaptureInProgress || - _capture_status.image_status == - CameraServer::CaptureStatus::ImageStatus::IntervalInProgress) { - image_status |= StatusFlags::IN_PROGRESS; - } + uint8_t video_status{}; + uint32_t recording_time_ms{}; + float available_capacity{}; + float image_capture_timer_interval_s{}; + int32_t image_capture_count{}; - if (_capture_status.image_status == CameraServer::CaptureStatus::ImageStatus::IntervalIdle || - _capture_status.image_status == - CameraServer::CaptureStatus::ImageStatus::IntervalInProgress || - _is_image_capture_interval_set) { - image_status |= StatusFlags::INTERVAL_SET; - } + { + std::lock_guard lg{_mutex}; - uint8_t video_status = 0; - if (_capture_status.video_status == CameraServer::CaptureStatus::VideoStatus::Idle) { - video_status = 0; - } else if ( - _capture_status.video_status == - CameraServer::CaptureStatus::VideoStatus::CaptureInProgress) { - video_status = 1; - } + if (_capture_status.image_status == + CameraServer::CaptureStatus::ImageStatus::CaptureInProgress || + _capture_status.image_status == + CameraServer::CaptureStatus::ImageStatus::IntervalInProgress) { + image_status |= StatusFlags::IN_PROGRESS; + } - const uint32_t recording_time_ms = - static_cast(static_cast(_capture_status.recording_time_s) * 1e3); - const float available_capacity = _capture_status.available_capacity_mib; + if (_capture_status.image_status == + CameraServer::CaptureStatus::ImageStatus::IntervalIdle || + _capture_status.image_status == + CameraServer::CaptureStatus::ImageStatus::IntervalInProgress || + _is_image_capture_interval_set) { + image_status |= StatusFlags::INTERVAL_SET; + } + + if (_capture_status.video_status == CameraServer::CaptureStatus::VideoStatus::Idle) { + video_status = 0; + } else if ( + _capture_status.video_status == + CameraServer::CaptureStatus::VideoStatus::CaptureInProgress) { + video_status = 1; + } + + recording_time_ms = + static_cast(static_cast(_capture_status.recording_time_s) * 1e3); + available_capacity = _capture_status.available_capacity_mib; + image_capture_timer_interval_s = _image_capture_timer_interval_s; + image_capture_count = _image_capture_count; + } _server_component_impl->queue_message([&](MavlinkAddress mavlink_address, uint8_t channel) { mavlink_message_t message{}; @@ -1409,10 +1420,10 @@ void CameraServerImpl::send_capture_status() static_cast(_server_component_impl->get_time().elapsed_s() * 1e3), image_status, video_status, - _image_capture_timer_interval_s, + image_capture_timer_interval_s, recording_time_ms, available_capacity, - _image_capture_count, + image_capture_count, 0); return message; }); @@ -2140,12 +2151,21 @@ void CameraServerImpl::send_tracking_status_with_interval(uint32_t interval_us) return; } } + TrackingMode tracking_mode{}; + CameraServer::TrackPoint tracked_point{}; + CameraServer::TrackRectangle tracked_rectangle{}; + { + std::lock_guard lg{_mutex}; + tracking_mode = _tracking_mode; + tracked_point = _tracked_point; + tracked_rectangle = _tracked_rectangle; + } + _server_component_impl->queue_message([&](MavlinkAddress mavlink_address, uint8_t channel) { mavlink_message_t message; - std::lock_guard lg{_mutex}; // The message is filled based on current tracking mode - switch (_tracking_mode) { + switch (tracking_mode) { default: // Fallthrough case TrackingMode::NONE: @@ -2177,9 +2197,9 @@ void CameraServerImpl::send_tracking_status_with_interval(uint32_t interval_us) CAMERA_TRACKING_STATUS_FLAGS_ACTIVE, CAMERA_TRACKING_MODE_POINT, CAMERA_TRACKING_TARGET_DATA_IN_STATUS, - _tracked_point.point_x, - _tracked_point.point_y, - _tracked_point.radius, + tracked_point.point_x, + tracked_point.point_y, + tracked_point.radius, 0.0f, 0.0f, 0.0f, @@ -2200,10 +2220,10 @@ void CameraServerImpl::send_tracking_status_with_interval(uint32_t interval_us) 0.0f, 0.0f, 0.0f, - _tracked_rectangle.top_left_corner_x, - _tracked_rectangle.top_left_corner_y, - _tracked_rectangle.bottom_right_corner_x, - _tracked_rectangle.bottom_right_corner_y, + tracked_rectangle.top_left_corner_x, + tracked_rectangle.top_left_corner_y, + tracked_rectangle.bottom_right_corner_x, + tracked_rectangle.bottom_right_corner_y, 0); break; } diff --git a/src/mavsdk/plugins/log_streaming/log_streaming_backend_ardupilot.cpp b/src/mavsdk/plugins/log_streaming/log_streaming_backend_ardupilot.cpp index eaa4e79152..85ac4f2cf8 100644 --- a/src/mavsdk/plugins/log_streaming/log_streaming_backend_ardupilot.cpp +++ b/src/mavsdk/plugins/log_streaming/log_streaming_backend_ardupilot.cpp @@ -36,7 +36,10 @@ void LogStreamingBackendArdupilot::deinit() } if (_system_impl) { - _system_impl->unregister_all_mavlink_message_handlers(this); + // Blocking, so that no message callback can fire into this backend anymore once + // deinit() returns -- the backend is destroyed right afterwards. This is safe from + // both the user thread (plugin destructor) and the io thread (disconnect). + _system_impl->unregister_all_mavlink_message_handlers_blocking(this); } } diff --git a/src/mavsdk/plugins/log_streaming/log_streaming_backend_px4.cpp b/src/mavsdk/plugins/log_streaming/log_streaming_backend_px4.cpp index 4fc149ae5d..5de3470a48 100644 --- a/src/mavsdk/plugins/log_streaming/log_streaming_backend_px4.cpp +++ b/src/mavsdk/plugins/log_streaming/log_streaming_backend_px4.cpp @@ -38,7 +38,10 @@ void LogStreamingBackendPx4::deinit() } if (_system_impl) { - _system_impl->unregister_all_mavlink_message_handlers(this); + // Blocking, so that no message callback can fire into this backend anymore once + // deinit() returns -- the backend is destroyed right afterwards. This is safe from + // both the user thread (plugin destructor) and the io thread (disconnect). + _system_impl->unregister_all_mavlink_message_handlers_blocking(this); } } diff --git a/src/mavsdk/plugins/log_streaming/log_streaming_impl.cpp b/src/mavsdk/plugins/log_streaming/log_streaming_impl.cpp index 4d93b95325..3b4e2b4e7c 100644 --- a/src/mavsdk/plugins/log_streaming/log_streaming_impl.cpp +++ b/src/mavsdk/plugins/log_streaming/log_streaming_impl.cpp @@ -40,18 +40,25 @@ void LogStreamingImpl::init() void LogStreamingImpl::deinit() { - std::lock_guard lock(_mutex); + std::unique_ptr backend; + { + std::lock_guard lock(_mutex); + + // Cancel any pending autopilot type polling + if (_check_autopilot_cookie) { + _system_impl->remove_call_every(_check_autopilot_cookie); + _check_autopilot_cookie = {}; + } + _start_callback = nullptr; - // Cancel any pending autopilot type polling - if (_check_autopilot_cookie) { - _system_impl->remove_call_every(_check_autopilot_cookie); - _check_autopilot_cookie = {}; + backend = std::move(_backend); } - _start_callback = nullptr; - if (_backend) { - _backend->deinit(); - _backend.reset(); + // Deinit (and destroy) the backend outside of _mutex: its blocking unregister can wait + // for an in-flight message callback to finish, and that callback may be in + // process_data() waiting for _mutex. + if (backend) { + backend->deinit(); } } @@ -59,10 +66,15 @@ void LogStreamingImpl::enable() {} void LogStreamingImpl::disable() { - std::lock_guard lock(_mutex); - if (_backend) { - _backend->deinit(); - _backend.reset(); + std::unique_ptr backend; + { + std::lock_guard lock(_mutex); + backend = std::move(_backend); + } + + // Deinit (and destroy) the backend outside of _mutex, see deinit() above. + if (backend) { + backend->deinit(); } } diff --git a/src/mavsdk/plugins/mavlink_direct/mavlink_direct_impl.cpp b/src/mavsdk/plugins/mavlink_direct/mavlink_direct_impl.cpp index 039141d298..03cd6d00fd 100644 --- a/src/mavsdk/plugins/mavlink_direct/mavlink_direct_impl.cpp +++ b/src/mavsdk/plugins/mavlink_direct/mavlink_direct_impl.cpp @@ -62,6 +62,13 @@ void MavlinkDirectImpl::init() void MavlinkDirectImpl::deinit() { + // Synchronise with any in-flight I/O-thread dispatch: _callbacks()/exec() holds + // the CallbackList's internal mutex, and clear() acquires the same mutex, so + // clear() blocks until the current exec() finishes. After clear() the list is + // empty; subsequent exec() calls are no-ops, making it safe to destroy the + // CallbackList. This also prevents any further user callbacks from being enqueued. + _callbacks.clear(); + // Unsubscribe from SystemImpl - this automatically prevents dangling callbacks if (_system_subscription.valid()) { _system_impl->unregister_libmav_message_handler(_system_subscription); @@ -110,8 +117,7 @@ MavlinkDirect::Result MavlinkDirectImpl::send_message(MavlinkDirect::MavlinkMess } if (message.target_component_id != 0) { // For messages that have target_component field, set it - libmav_message.set( - "target_component_id", static_cast(message.target_component_id)); + libmav_message.set("target_component", static_cast(message.target_component_id)); } if (_debugging) { diff --git a/src/mavsdk/plugins/mavlink_passthrough/include/plugins/mavlink_passthrough/mavlink_passthrough.h b/src/mavsdk/plugins/mavlink_passthrough/include/plugins/mavlink_passthrough/mavlink_passthrough.h index 7959c59523..13131b0d3a 100644 --- a/src/mavsdk/plugins/mavlink_passthrough/include/plugins/mavlink_passthrough/mavlink_passthrough.h +++ b/src/mavsdk/plugins/mavlink_passthrough/include/plugins/mavlink_passthrough/mavlink_passthrough.h @@ -91,7 +91,8 @@ class MAVSDK_PUBLIC MavlinkPassthrough : public PluginBase { * * @return A reference to the stream. */ - friend std::ostream& operator<<(std::ostream& str, MavlinkPassthrough::Result const& result); + friend MAVSDK_PUBLIC std::ostream& + operator<<(std::ostream& str, MavlinkPassthrough::Result const& result); /** * @brief Send message (deprecated). From a1505e5d216291ca8bf70eecdb59e1825e1fec6c Mon Sep 17 00:00:00 2001 From: Julian Oes Date: Thu, 16 Jul 2026 12:44:32 +1200 Subject: [PATCH 7/9] ci: update Windows CI to use Visual Studio 18 2026 Backport of ca430cc7f (#2900). The windows-2025 runner image migrated from VS 2022 (VS 17) to VS 2026 (VS 18) in June 2026; update all cmake generator strings accordingly. --- .github/workflows/windows.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index 6187304b67..ba6dc8682e 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -59,7 +59,7 @@ jobs: choco install jom echo "C:\Program Files\jom" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append - name: configure - run: cmake -G"Visual Studio 17 2022" -A ${{ matrix.platform }} + run: cmake -G"Visual Studio 18 2026" -A ${{ matrix.platform }} $env:superbuild $env:cmake_prefix_path -DCMAKE_BUILD_TYPE=RelWithDebInfo @@ -144,7 +144,7 @@ jobs: choco install jom echo "C:\Program Files\jom" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append - name: configure - run: cmake -G"Visual Studio 17 2022" -A ${{ matrix.platform }} + run: cmake -G"Visual Studio 18 2026" -A ${{ matrix.platform }} $env:superbuild $env:cmake_prefix_path -DCMAKE_BUILD_TYPE=${{ matrix.config }} @@ -223,7 +223,7 @@ jobs: run: | rem Build examples in Debug configuration cd /d %GITHUB_WORKSPACE% - cmake -G"Visual Studio 17 2022" -A ${{ matrix.platform }} ^ + cmake -G"Visual Studio 18 2026" -A ${{ matrix.platform }} ^ -DCMAKE_BUILD_TYPE=Debug ^ -DCMAKE_PREFIX_PATH=%GITHUB_WORKSPACE%\combined ^ -Bbuild-examples-debug ^ @@ -235,7 +235,7 @@ jobs: run: | rem Build examples in RelWithDebInfo configuration cd /d %GITHUB_WORKSPACE% - cmake -G"Visual Studio 17 2022" -A ${{ matrix.platform }} ^ + cmake -G"Visual Studio 18 2026" -A ${{ matrix.platform }} ^ -DCMAKE_BUILD_TYPE=RelWithDebInfo ^ -DCMAKE_PREFIX_PATH=%GITHUB_WORKSPACE%\combined ^ -Bbuild-examples-release ^ From 2f4a4488b7da37298e87f4ce842d571ba96f9595 Mon Sep 17 00:00:00 2001 From: Julian Oes Date: Thu, 16 Jul 2026 14:04:01 +1200 Subject: [PATCH 8/9] offboard: give watchdog grace period from when setpoints start Backport of dfc69b5e0 (#2927). process_heartbeat() stops setpoint streaming when the vehicle is not in offboard mode more than 3 seconds after the grace start. That timestamp was only written in start(), so before the first start() it was still epoch 0 and the guard always passed: any heartbeat between the first set_*() and start() reset the setpoint state, and start() then failed with NoSetpointSet. Frequent under lockstep SITL with a speed factor. Stamp the grace start whenever streaming transitions out of NotActive so the watchdog covers the gap between the first setpoint and start(). --- src/mavsdk/plugins/offboard/offboard_impl.cpp | 25 ++++++++++++++++--- src/mavsdk/plugins/offboard/offboard_impl.h | 3 ++- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/mavsdk/plugins/offboard/offboard_impl.cpp b/src/mavsdk/plugins/offboard/offboard_impl.cpp index 6751fb627b..e32103af05 100644 --- a/src/mavsdk/plugins/offboard/offboard_impl.cpp +++ b/src/mavsdk/plugins/offboard/offboard_impl.cpp @@ -51,7 +51,7 @@ Offboard::Result OffboardImpl::start() if (_mode == Mode::NotActive) { return Offboard::Result::NoSetpointSet; } - _last_started = _time.steady_time(); + _watchdog_grace_start = _time.steady_time(); } return offboard_result_from_command_result(_system_impl->set_flight_mode(FlightMode::Offboard)); @@ -81,7 +81,7 @@ void OffboardImpl::start_async(Offboard::ResultCallback callback) } return; } - _last_started = _time.steady_time(); + _watchdog_grace_start = _time.steady_time(); } _system_impl->set_flight_mode_async( @@ -134,6 +134,7 @@ Offboard::Result OffboardImpl::set_position_ned(Offboard::PositionNedYaw positio _call_every_cookie = _system_impl->add_call_every([this]() { send_position_ned(); }, SEND_INTERVAL_S); + note_setpoints_started(); _mode = Mode::PositionNed; } else { // We're already sending these kind of setpoints. Since the setpoint change, let's @@ -159,6 +160,7 @@ Offboard::Result OffboardImpl::set_position_global(Offboard::PositionGlobalYaw p _call_every_cookie = _system_impl->add_call_every([this]() { send_position_global(); }, SEND_INTERVAL_S); + note_setpoints_started(); _mode = Mode::PositionGlobalAltRel; } else { // We're already sending these kind of setpoints. Since the setpoint change, let's @@ -184,6 +186,7 @@ Offboard::Result OffboardImpl::set_velocity_ned(Offboard::VelocityNedYaw velocit _call_every_cookie = _system_impl->add_call_every([this]() { send_velocity_ned(); }, SEND_INTERVAL_S); + note_setpoints_started(); _mode = Mode::VelocityNed; } else { // We're already sending these kind of setpoints. Since the setpoint change, let's @@ -210,6 +213,7 @@ Offboard::Result OffboardImpl::set_position_velocity_ned( _call_every_cookie = _system_impl->add_call_every( [this]() { send_position_velocity_ned(); }, SEND_INTERVAL_S); + note_setpoints_started(); _mode = Mode::PositionVelocityNed; } else { // We're already sending these kind of setpoints. Since the setpoint change, let's @@ -240,6 +244,7 @@ Offboard::Result OffboardImpl::set_position_velocity_acceleration_ned( _call_every_cookie = _system_impl->add_call_every( [this]() { send_position_velocity_acceleration_ned(); }, SEND_INTERVAL_S); + note_setpoints_started(); _mode = Mode::PositionVelocityAccelerationNed; } else { // We're already sending these kind of setpoints. Since the setpoint change, let's @@ -265,6 +270,7 @@ Offboard::Result OffboardImpl::set_acceleration_ned(Offboard::AccelerationNed ac _call_every_cookie = _system_impl->add_call_every( [this]() { send_acceleration_ned(); }, SEND_INTERVAL_S); + note_setpoints_started(); _mode = Mode::AccelerationNed; } else { // We're already sending these kind of setpoints. Since the setpoint change, let's @@ -291,6 +297,7 @@ OffboardImpl::set_velocity_body(Offboard::VelocityBodyYawspeed velocity_body_yaw _call_every_cookie = _system_impl->add_call_every([this]() { send_velocity_body(); }, SEND_INTERVAL_S); + note_setpoints_started(); _mode = Mode::VelocityBody; } else { // We're already sending these kind of setpoints. Since the setpoint change, let's @@ -316,6 +323,7 @@ Offboard::Result OffboardImpl::set_attitude(Offboard::Attitude attitude) _call_every_cookie = _system_impl->add_call_every([this]() { send_attitude(); }, SEND_INTERVAL_S); + note_setpoints_started(); _mode = Mode::Attitude; } else { // We're already sending these kind of setpoints. Since the setpoint change, let's @@ -341,6 +349,7 @@ Offboard::Result OffboardImpl::set_attitude_rate(Offboard::AttitudeRate attitude _call_every_cookie = _system_impl->add_call_every([this]() { send_attitude_rate(); }, SEND_INTERVAL_S); + note_setpoints_started(); _mode = Mode::AttitudeRate; } else { // We're already sending these kind of setpoints. Since the setpoint change, let's @@ -366,6 +375,7 @@ Offboard::Result OffboardImpl::set_actuator_control(Offboard::ActuatorControl ac _call_every_cookie = _system_impl->add_call_every( [this]() { send_actuator_control(); }, SEND_INTERVAL_S); + note_setpoints_started(); _mode = Mode::ActuatorControl; } else { // We're already sending these kind of values. Since the value changes, let's @@ -851,7 +861,7 @@ void OffboardImpl::process_heartbeat(const mavlink_message_t& message) // possibly stale heartbeats for some time. std::lock_guard lock(_mutex); if (!offboard_mode_active && _mode != Mode::NotActive && - _time.elapsed_since_s(_last_started) > 3.0) { + _time.elapsed_since_s(_watchdog_grace_start) > 3.0) { // It seems that we are no longer in offboard mode but still trying to send // setpoints. Let's stop for now. stop_sending_setpoints(); @@ -859,6 +869,15 @@ void OffboardImpl::process_heartbeat(const mavlink_message_t& message) } } +void OffboardImpl::note_setpoints_started() +{ + // We assume that we already acquired the mutex in this function. + + if (_mode == Mode::NotActive) { + _watchdog_grace_start = _time.steady_time(); + } +} + void OffboardImpl::stop_sending_setpoints() { // We assume that we already acquired the mutex in this function. diff --git a/src/mavsdk/plugins/offboard/offboard_impl.h b/src/mavsdk/plugins/offboard/offboard_impl.h index f862af8e9f..9136615cc8 100644 --- a/src/mavsdk/plugins/offboard/offboard_impl.h +++ b/src/mavsdk/plugins/offboard/offboard_impl.h @@ -68,6 +68,7 @@ class OffboardImpl : public PluginImplBase { offboard_result_from_command_result(MavlinkCommandSender::Result result); void stop_sending_setpoints(); + void note_setpoints_started(); Time _time{}; @@ -94,7 +95,7 @@ class OffboardImpl : public PluginImplBase { Offboard::Attitude _attitude{}; Offboard::AttitudeRate _attitude_rate{}; Offboard::ActuatorControl _actuator_control{}; - SteadyTimePoint _last_started{}; + SteadyTimePoint _watchdog_grace_start{}; CallEveryHandler::Cookie _call_every_cookie{}; From 46af30374adb29c713f1562c4e6c9a3216587c48 Mon Sep 17 00:00:00 2001 From: Julian Oes Date: Fri, 17 Jul 2026 13:37:33 +1200 Subject: [PATCH 9/9] core: fix data race on _next_cookie in CallEveryHandler::add _next_cookie++ is a read-modify-write that ran before the lock_guard was taken, so two concurrent add() calls raced on it. TSAN caught this in the Intercept system test: the main thread adding a call_every via TelemetryImpl::enable() raced against the callback thread adding one via MavsdkImpl::start_sending_heartbeats(). Take the lock before incrementing, as main already does. The fix landed on main as part of the Asio migration (f8156b065, #2810), which is not backportable to v3, so apply the equivalent minimal change here. This is a pre-existing race in v3, not a regression from the other backports in this branch. --- src/mavsdk/core/call_every_handler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mavsdk/core/call_every_handler.cpp b/src/mavsdk/core/call_every_handler.cpp index 1b3cb2dbe6..f68c4e538e 100644 --- a/src/mavsdk/core/call_every_handler.cpp +++ b/src/mavsdk/core/call_every_handler.cpp @@ -18,9 +18,9 @@ CallEveryHandler::Cookie CallEveryHandler::add(std::function callback, d _time.shift_steady_time_by(before, -interval_s - 0.001); new_entry.last_time = before; new_entry.interval_s = interval_s; - new_entry.cookie = _next_cookie++; std::lock_guard lock(_mutex); + new_entry.cookie = _next_cookie++; _entries.push_back(new_entry); return new_entry.cookie;