core: fix buffer overflows, deadlocks, and races in core#2925
Merged
Conversation
The helper scanned the fixed 239-byte data[] buffer using max_data_length regardless of the sender-supplied payload.size, and never bounds-checked the running start/end offsets. Because max_data_length is uint8_t and start is size_t, 'max_data_length - start' underflows to a huge value once start exceeds 239, so a FILE_TRANSFER_PROTOCOL message (e.g. CMD_RENAME) with a NUL-free field made strnlen read far past the buffer. Bound all scanning by the validated payload.size and clamp offsets so the memcpy can never read past data[].
…alue MAV_PARAM_EXT_TYPE_CUSTOM used strlen() on param_value, a char[128] that MAVLink does not guarantee to be null-terminated. A full 128-byte value with no zero byte made strlen scan past the field into adjacent struct memory; the std::min(128, ...) cap was applied only after the over-read. Use strnlen with the 128-byte bound, matching set_from_mavlink_param_ext_value.
uri_is_mavlinkftp parsed the optional [;comp=<N>] prefix of a COMPONENT_METADATA uri with std::stoi. The uri is autopilot-supplied, so a value greater than INT_MAX (e.g. mftp://[;comp=99999999999]/x.json) threw std::out_of_range, which under -fno-exceptions calls std::terminate() - a trivially triggerable remote DoS. Parse with std::strtol and validate the full string and the 0..255 range.
Connection::receive_libmav_message inserted into _system_ids without holding _system_ids_mutex, while receive_message and has_system_id (called from the send path on user threads) both lock it. A rehash on the io thread concurrent with a find() on a sender thread is a data race - undefined behaviour that can misroute messages or crash on any multi-system or forwarding setup.
The file handle from fopen(path, "wb") was never checked for nullptr. It was handed to libcurl's default fwrite writer (CURLOPT_WRITEFUNCTION == NULL) and unconditionally fclose()d. An unwritable local_path (missing parent directory or permissions) therefore caused a NULL FILE* dereference in fwrite or fclose. Bail out cleanly if the file cannot be opened.
A request that received a Success command ack schedules a 1s TimeoutHandler callback capturing 'this'. The destructor unregistered the message handler but never removed these timeout cookies, so if the object was destroyed (system disconnect/shutdown) while a request was in flight, TimeoutHandler would later invoke handle_timeout on freed memory. Remove all pending cookies on teardown, matching the mission-transfer WorkItem destructors.
The skip condition used 'message_id != msgid && target_component == compid', so a work item was matched whenever the message id differed OR the component matched. An unrelated incoming message could complete the wrong pending request with Result::Success and the wrong payload. Skip unless both the message id and component match, mirroring handle_timeout.
The call used the single-iterator erase overload with the iterator returned by std::remove_if. Unregistering a cookie that was not present makes remove_if return end(), so erase(end()) is undefined behaviour; even a matching cookie only worked by luck. Use the two-argument erase(remove_if(...), end()).
_configuration was written under _server_components_mutex, read under _mutex in get_configuration(), and read with no lock at all in get_mav_type/get_autopilot/ get_mav_autopilot/get_compatibility_mode/effective_autopilot and the component- type checks in message processing. Writer and readers shared no common mutex, so a set_configuration() on a user thread could race a read on the io thread. Introduce a dedicated leaf mutex _configuration_mutex that guards only _configuration and is never held while taking another lock, so it cannot invert with _mutex / _server_components_mutex. Route all reads through it (adding a get_component_type() helper).
The burst worker ran _send_burst_packet(), which locked _mutex, while _reset()/ _work_burst()/_work_terminate() set burst_stop and join()ed that thread while already holding _mutex. If the worker re-entered _send_burst_packet() before seeing burst_stop, it blocked forever on _mutex and join() never returned, permanently hanging the FTP server (and teardown). Make burst_stop atomic and have the worker try_lock _mutex, so it can observe the stop request and exit even while the joining thread holds the lock. _send_burst_packet() now requires the caller to hold _mutex.
process_param_set_internally called find_and_call_subscriptions_value_changed while holding _all_params_mutex. Those callbacks are arbitrary user code; if one re-entered a server API that also takes _all_params_mutex it would self-deadlock on the non-recursive mutex on the io thread, stalling all MAVLink processing. Post the notification onto the io_context so it runs after the lock is released, matching provide_server_param().
The MissingParam and non-extended WrongType branches set send_error=true and returned out of the whole function, skipping the trailing 'if (send_error) send_param_error(...)' block, so the server never emitted PARAM_ERROR. Clients then waited through their full retry/timeout cycle instead of getting an immediate DoesNotExist/TypeMismatch. Break out of the switch instead of returning so the deferred send runs.
_work_write seeks the output stream with ofstream.seekp() but then tested ifstream.fail() to detect a seek error, so a failed seek on the write stream went undetected. Check ofstream.fail().
When HOME is unset, the code dereferenced getpwuid(getuid())->pw_dir before the subsequent NULL check. getpwuid() returns NULL when the uid has no passwd entry (common in minimal containers/sandboxes), causing a NULL dereference. Check the passwd pointer before reading pw_dir.
julianoes
force-pushed
the
fix-core-memory-safety-and-concurrency
branch
from
933a2f9 to
bd2a22c
Compare
std::from_chars stops at the first non-digit and still reports success, so connection URLs like udpout://host:8080abc or serial:///dev/ttyUSB0:57600xyz were accepted with the leading number silently. Require that the entire string_view was consumed.
all_parameters(false) returned a filtered vector whose entries kept their full-set index, but param_by_index() indexes that vector by the raw wire index and count(false) reports the filtered size. When extended-only params are interspersed, the stored index no longer matched the position: param_by_index() returned the wrong param or tripped assert(param.index == param_index), and broadcast_all_parameters(false) advertised param_index values >= param_count. Reassign contiguous indices to the filtered view so it is self-consistent.
ACCUMULATION_BUFFER_SIZE was one max message (~280 bytes), but set_new_datagram appends a whole read (up to the 2048-byte connection receive buffer) and trims to the cap *before* parsing. A TCP read or batched UDP datagram carrying several messages therefore had all but its ~280-byte tail erased, so the libmav (MavlinkDirect) receive path dropped most messages on busy links. Size the cap to a full read plus one message; the parser already drains the buffer, so this only ever acts as a safety valve.
On a burst download gap, the client did std::vector<char>(payload->offset - current_offset) to zero-fill the missing range. payload->offset is a 32-bit value from the server checked only to be >= current_offset, so a crafted offset could request a ~4 GB allocation (and disk write) - OOM/abort under -fno-exceptions. Reject any offset beyond the known file size.
ReceiveIncomingMission::process_mission_item_int pushed every received MISSION_ITEM_INT without checking seq against _next_sequence and refreshed the timeout each time, so a peer streaming items (e.g. all seq 0) grew _items without bound while the transfer never timed out. Guard on _next_sequence == item_int.seq, mirroring the download client.
The inet_ntop return value was ignored and the (uninitialized) buffer was read unconditionally, so a failure would build the address from stack garbage and an empty result would still be returned as a successful (empty) string. Zero-init the buffer, only accept the address when inet_ntop succeeds, and return nullopt otherwise.
send_raw_bytes() calls _socket.send_to() synchronously on a caller thread while holding _remote_mutex, but stop() closed the socket from the io thread without taking that mutex. asio forbids closing a socket concurrently with another operation on it, so a send during teardown was a data race. Hold _remote_mutex while closing in both stop() paths.
The slow-callback watchdog lambda captured the loop-local UserCallback by reference and reads callback.filename (a std::string) on the io thread when it fires. TimeoutHandler executes expired callbacks outside its lock, so it could run just as the worker thread finished callback.func() and moved to the next iteration, destroying the captured object - a use-after-free (debug builds). Capture filename/linenumber by value instead.
process_mavlink_ftp_message wrote _target_system_id/_target_component_id outside _mutex, while the burst thread reads them (under _mutex) in _send_mavlink_ftp_message. Make them atomic so the concurrent write from a new incoming FTP message and the burst-thread read are not a data race.
_statustext_handler_callbacks and _param_changed_callbacks were mutated by register/unregister (which can run on a user thread via plugin init) and iterated on the io thread when messages arrive, with no synchronization - a data race and potential iterator invalidation. Guard each with its own mutex and snapshot the callbacks under the lock before invoking them, so callbacks never run while the lock is held (avoiding re-entrancy deadlock).
The crash handler called cpptrace::generate_trace().print(), but cpptrace v0.3.1 frequently failed to symbolize from a signal handler (its macOS backend in particular), so a SIGSEGV in CI printed only bare frame addresses that can't be mapped back to code. Bump cpptrace to v1.0.4, whose symbolizer resolves function/file/line reliably (verified locally: a forced crash now prints a fully symbolized backtrace down to the faulting test line). The signal-safe raw-trace API is intentionally not used: it needs the libunwind unwinder backend to collect frames and returns nothing with the default one, which would be worse than generate_trace().
julianoes
commented
Jul 15, 2026
julianoes
left a comment
julianoes
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed, looks good.
julianoes
force-pushed
the
fix-core-memory-safety-and-concurrency
branch
from
71c7a31 to
13fc9bd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
An audit of
src/mavsdk/core/for buffer overflows, deadlocks, and other memory-safety / concurrency bugs. Each fix is its own commit. Build is clean and all unit tests (291) and system tests (95) pass.Memory safety (remotely reachable)
_data_as_stringOOB read — scanned the fixed 239-bytedata[]ignoringpayload.size, andmax_data_length - start(uint8_t − size_t) underflowed; aCMD_RENAMEwith a NUL-free field read far past the buffer. Now bounded bypayload.size.PARAM_EXT_SETcustom value OOB read — usedstrlen()on the non-terminatedchar[128]; switched tostrnlen().std::stoi→terminate()— aCOMPONENT_METADATAuri withcomp=> INT_MAX aborted under-fno-exceptions; parse withstrtol+ range check.fopenNULL deref indownload_file_to_path— unwritable path caused a NULLFILE*deref in libcurl's writer /fclose.offsettriggered a ~4 GB zero-fill vector; reject offsets past the file size.getpwuid()NULL deref inget_cache_directorywhenHOMEis unset and the uid has no passwd entry.inet_ntopresult unchecked inresolve_hostname_to_ip(could read uninitialized buffer / return empty as success).Concurrency (races, deadlocks, use-after-free)
MavlinkRequestMessageUAF — the destructor didn't cancel pendingTimeoutHandlercookies, so a timeout could fire on freed memory after teardown._reset()/_work_burstjoined the burst thread while holding_mutex, which the worker needs; madeburst_stopatomic and the workertry_lock._all_params_mutex— a callback re-entering a server API would self-deadlock; post the notification off the lock (matchingprovide_server_param)._configurationinconsistent locking — written under one mutex, read under another or none; now guarded by a dedicated leaf mutex._system_idsunlocked insert on the libmav receive path (raced the locked send-path reader).send_tovs socket close race during teardown — close now takes_remote_mutex.Correctness
unregister_statustext_handlerused the single-iteratorerase(remove_if(...))idiom → UB when the cookie isn't present.MavlinkRequestMessage::handle_any_messageused&&where it needed||, so an unrelated message could complete the wrong request.PARAM_ERRORnever sent for missing/wrong-type non-extendedPARAM_SET(returninstead ofbreak).param_by_index/ broadcasts were inconsistent withcount(); reindex contiguously.seq == _next_sequencelike the client._work_writecheckedifstream.fail()afterofstream.seekp().cli_argaccepted trailing garbage in port/baudrate (from_charsdidn't require full consumption).Not addressed (intentionally)
Two lower-confidence, design-level items were left out to avoid a destabilizing change:
on_io_thread()relying on asio'srunning_in_this_thread()across the DSO boundary (possible self-deadlock inCallbackList::exec/drain).stop()on the connection classes self-deadlocking if ever driven from the io thread.Testing
cmake --build build-debug— cleanunit_tests_runner— 291 passedsystem_tests_runner— 95 passed