px4_task_t is a plain int and callers hold -1 for "no task" (e.g. dataman's
g_task_id after stop). The bounds checks only tested id < PX4_MAX_TASKS, which is
true for negative ids, so px4_task_join(-1) and px4_task_delete(-1) would index
taskmap[-1] out of bounds. Check id >= 0 as well.
Signed-off-by: Julian Oes <julian@oes.ch>
process_add_subscription() (CONFIG_ORB_COMMUNICATOR only) read _data_valid and
the topic buffer contents while passing a pointer into _data straight to
send_message(), with no synchronization against a concurrent publisher. A
publisher's write() memcpy's into _data and sets _data_valid under ATOMIC_ENTER,
so the remote-init send can read a torn/partially-updated sample - most clearly
for o_queue == 1, where the read slot ((gen - 1) % 1) is exactly the slot being
written.
This is a pre-existing latent race (the previous px4::atomic<uint8_t*> _data only
guarded the pointer, never the bytes) that neither ASan nor TSan can catch because
CONFIG_ORB_COMMUNICATOR is not built in those configs. On VOXL2 it runs on the
muorb RX thread concurrently with publishers.
Snapshot the most recent sample into a temporary buffer under ATOMIC_ENTER, then
send it outside the critical section - mirroring copy(). The buffer is allocated
before the critical section (it cannot be allocated under ATOMIC_ENTER on NuttX)
and send_message() must run outside it anyway, since it can be slow and may call
back into DeviceNode. This only affects the rare subscription-add path, not the
publish hot path.
Signed-off-by: Julian Oes <julian@oes.ch>
The daemon server's client-handler thread spawns module tasks (e.g. dataman).
Start px4::init_once()/init() before creating px4_daemon::Server so platform init
- uORB, work queues, logging - completes with a happens-before edge to those
tasks. Otherwise a module task racing the still-running init reads globals like
uORB::Manager's instance pointer or the log message advertisement without
synchronization (ThreadSanitizer flags it) - and an atomic would not make it
correct, since the publication could simply be skipped.
tx_message_count (every send) and tx_buffer_overruns took _tstatus_mutex on the
hot path just to increment a counter. Make them px4::atomic, like the existing
_bytes_* counters, bumped lock-free from either thread's send path and folded
into _tstatus (cumulative) under the mutex at the ~1 Hz publish. The mutex is
kept for the genuinely shared low-frequency fields. (Suggested in review.)
The param access mutex must be recursive because a layered param lookup
re-enters the lock on the same thread (Layer::get() -> _parent->get()).
Initialize it in param_init() - which runs once before any thread accesses
parameters - via pthread_mutexattr_settype(PTHREAD_MUTEX_RECURSIVE), replacing
the previous pthread_once lazy init. This avoids the non-portable glibc static
initializer PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP, which is unavailable on QURT
and gated behind _GNU_SOURCE elsewhere (broke the voxl2 and SITL builds).
_data (the lazily-allocated topic buffer) was a px4::atomic only to make the
double-checked-locking publish safe against concurrent readers. It is allocated
under the node lock() and written exactly once, so reading it under ATOMIC_ENTER
- which is that same lock() on POSIX - serializes the subscriber side against the
allocation without needing an atomic. On NuttX it is a set-once, word-aligned
pointer. The copy() guard is moved inside ATOMIC_ENTER accordingly. (Suggested in
review.)
DeviceNode::open() took the cdev lock only to call mark_as_advertised(), but
_advertised is a px4::atomic, so the single store needs no lock. (Suggested in
review.)
Add a test_and_ci page covering the PX4_ASAN / PX4_TSAN / PX4_MSAN / PX4_UBSAN
build variables, the Linux `sudo sysctl vm.mmap_rnd_bits=28` workaround for the
sanitizer runtime's startup memory mapping, and the TSan suppressions file
(test/sitl_tsan.supp) for the Gazebo simulation transport. Linked from SUMMARY.md
and the test_and_ci index.
Running SITL with the Gazebo bridge under ThreadSanitizer reports many data races
inside libzmq and gz-transport (and the GZBridge glue) - third-party simulation
transport that is not in the flight path or the SIH-based CI and that we cannot
fix. Add a suppressions file so the TSan signal stays usable for finding real
races. Use via TSAN_OPTIONS="suppressions=test/sitl_tsan.supp".
init() registered the vehicle_local_position callback before calling
set_interval_us() and initialising _time_stamp_last_loop. The moment the callback
is registered it fires (and Run() is scheduled) on the publisher's thread, racing
those writes - ThreadSanitizer flags both. Register the callback last, once the
object is fully initialised.
_interval_us and _last_update are read in SubscriptionCallbackWorkItem::call()
(and SubscriptionBlocking::call()) on the publishing thread, while the subscriber
sets them - e.g. from mixer_module's setMaxTopicUpdateRate() on a reconfigure.
That is a genuine cross-thread access that ThreadSanitizer flags.
Instead of making these fields atomic in the base SubscriptionInterval - which is
used single-threaded by 100+ subscriptions - parameterize the class on whether the
interval / last-update storage is atomic and expose two aliases:
SubscriptionInterval plain, single-threaded (the common case)
SubscriptionIntervalAtomic atomic, for callback subscriptions
SubscriptionCallback - and hence all callback subscriptions
(SubscriptionCallbackWorkItem, SubscriptionBlocking, CallbackHandler) - uses the
atomic variant; every other subscription stays plain with no synchronization
overhead.
The updated()/update()/copy() bodies stay out-of-line with explicit instantiation
of both variants, so they are not inlined at every call site (flash size).
The publisher fires callbacks via SubscriptionCallbackWorkItem::call(), which read
the subscriber's _last_generation (and called updated(), which reads it again) to
decide whether to schedule the work item. That made the subscriber's read cursor a
cross-thread field - publisher reads it while the subscriber writes it in copy() -
which TSan flagged and which we had wrapped in an atomic.
Instead, hand the publishing node's generation into call() and let the callback keep
its own _last_scheduled_generation cursor for the count gate (_required_updates).
call() runs under the node lock, so that cursor is only ever touched there and is
serialized - no atomic needed, and using the real generation (not a plain counter)
means a coalesced/missed call() does not drift the batch. The redundant updated()
generation check is dropped (call() only runs right after a publish, so it is always
true); the interval gate (_last_update / _interval_us) is kept unchanged.
With the publisher no longer reading it, _last_generation reverts to a plain unsigned
(subscriber-thread only). Behaviour is unchanged: the count and interval throttles
fire at the same rates (verified via work_queue status - gyro_fft 62.5 Hz,
flight_mode_manager 50 Hz) and TSan stays clean.
Times perf_count and perf_begin/perf_end in tight loops with hrt, plus a
plain non-atomic counter loop as reference, to quantify the overhead of
the atomic perf_counter change. Run with: microbench microbench_perf
MavlinkMissionManager keeps several pieces of mission state as static
class members so all instances share one view of the active mission:
_mission_dataman_id, _fence_dataman_id, _safepoint_dataman_id,
_current_seq, _count[], _crc32[]. Multiple instances' receiver
threads (and their send() ticks) read and write these concurrently,
which TSan flags.
Add a static pthread_mutex_t _shared_state_mutex (recursive, so the
nested call inside update_geofence_count -> update_active_mission is
fine) initialized once via pthread_once, and take it around every
read-modify-write or read-then-publish path:
- update_active_mission
- update_geofence_count
- update_safepoint_count
- send() (covers _current_seq / _count / _crc32 access)
- check_active_mission (the publisher of the dataman bump)
Mutex declaration is in the public section so the pthread_once
initializer can reach it; it remains a class-level static.
Three closely-related races flagged by TSan with multi-instance
mavlink under SIH:
- _protocol_version is touched by both threads (task_main writes it
in setProtocolVersion via mavlink_update_parameters; receiver
reads it via getProtocolVersion and itself writes it on auto-
upgrade to v2). Make it px4::atomic<uint8_t>.
- setProtocolVersion's get_status()->flags RMW touches the per-
channel mavlink_channel_statuses[] global, which the receiver
thread also reads/writes from mavlink_frame_char_buffer. Take
lock_send() around the body, matching the existing pattern around
mavlink_parse_char in the receiver.
- get_use_hil_gps() read _param_mav_usehilgps directly from the
receiver thread while task_main was writing it via updateParams.
Add an atomic snapshot _use_hil_gps_atomic, refresh it in
mavlink_update_parameters(), and return the atomic load from the
getter. Same pattern as _boot_complete and the other cross-thread
state in this class.
Drive-by: the receiver's "upgrade to v2" check at the top of
MavlinkReceiver::run was reading _mavlink.get_status()->flags
outside lock_send(). mavlink_parse_char() copies the channel-
global flags into the receiver-local _status, so read _status.flags
instead — no lock needed and no race.
Two fixes for the singleton MavlinkCommandSender used across all
Mavlink instances:
1. initialize() races between instances. Multiple Mavlink::start()
calls hit it concurrently; the existing nullptr check on _instance
is not atomic, and re-running px4_sem_init on a semaphore another
thread is already using is a TSan-flagged data race. Move the
actual init into do_initialize() and call it through pthread_once,
which is portable across POSIX and NuttX.
2. check_timeout() is invoked every Mavlink main loop tick on every
instance. Under load they queue up on the singleton's lock and
form a convoy that delays heartbeat sending. Switch to try_lock:
if another instance is already inside, skip this cycle and retry
on the next tick. The wait was never useful here — the work is
"scan for timed-out commands", which is fine to defer one tick.
Per-channel mavlink_status_t and the per-instance telemetry_status_s
are touched by both the main mavlink thread (sending streams,
publishing status) and the receiver thread (mavlink_parse_char,
counting RX bytes). Until the library is refactored so per-channel
state is owned by the instance and TX seq becomes atomic, take the
existing recursive lock_send() / lock_telemetry_status() helpers in
all the spots that needed them but didn't have them:
mavlink_main.cpp:
- Wrap the per-iteration streams / ulog / events / forwarding loop
with one lock_send() so a stream's send() never races with the
receiver's parse_char on the same channel status. The mutex is
recursive, so nested helpers that take it again are fine.
- lock_send() around the individual sends in handleMavlinkShellOutput,
handleCommands and handleAndGetCurrentCommandAck (these run after
the streams loop, but with the lock dropped between them).
mavlink_receiver.cpp:
- lock_send() around mavlink_parse_char() only — NOT around
handle_message(), because some handlers (e.g. param requests
flowing into configure_stream_threadsafe) busy-wait for the main
mavlink thread, which itself wants lock_send. Holding it across
handle_message produces a circular wait.
- lock_send() around request_message() invocations in
handle_request_message_command (synchronous send paths from RX).
- lock_telemetry_status() around the RX-side _tstatus update so it
doesn't race with publish_telemetry_status() on the main thread.
mavlink_parameters.cpp:
- lock_send() in the hash-param value send, send_param() and
send_error() — these run on both the main thread (no outer lock
in some paths) and the RX thread.
mavlink_mission.cpp:
- lock_send() around every mavlink_msg_mission_*_send_struct call.
streams/LINK_NODE_STATUS.hpp:
- lock_telemetry_status() around the tstatus copy.
Mirrors the comments left in code: the proper fix is to refactor the
shared globals out of the MAVLink library; this is the
"add the lock everywhere it was missing" interim.
The receiver thread reads _mavlink_start_time via get_start_time() in
its first iterations, so writing it after _receiver.start() races
with the receiver. Move the assignment up so the value is published
before the receiver can observe it.
Three closely related thread-safety fixes around state shared between
the Mavlink main thread, the receiver thread, and configure_stream
callers from arbitrary threads:
- _boot_complete becomes px4::atomic_bool. set_boot_complete() runs
on the main mavlink thread; boot_complete() is read from receiver
threads to gate forwarding decisions.
- _subscribe_to_stream becomes px4::atomic<char *>. Producers
(configure_stream_threadsafe on any thread) hand off a stream name
to the consumer (main mavlink thread, in check_requested_subscriptions);
the rate field is sequenced by the atomic store/load on the pointer.
Wall-clock usleep replaces px4_usleep here because in lockstep mode
px4_usleep waits in sim time, which can stall startup if the main
thread isn't advancing time yet.
- mavlink_system.sysid / .compid are written exactly once across
all instances using compare_exchange latches. The values are
singletons (every instance reads the same param), but the field is
read concurrently by _mav_finalize_message_chan_send, so two
instances writing the same value at construction is still a race
TSan flags. Latch only on a *valid* write so an early constructor
with params not yet loaded doesn't permanently lock the latch
closed.
GCC does not provide __has_feature, and its preprocessor tokenizes both
operands of && before evaluating the operator, so
#if defined(__SANITIZE_ADDRESS__) || defined(__has_feature) && __has_feature(address_sanitizer)
failed to compile with 'missing binary operator before token "("' on
toolchains such as the voxl2 linaro GCC. Add the standard __has_feature
fallback shim so the check uses __SANITIZE_ADDRESS__ on GCC and the real
__has_feature on Clang. Behavior is unchanged.
Signed-off-by: Julian Oes <julian@oes.ch>
system_exit() (= _exit()) on Linux skips C++ destructors and atexit
hooks, including ASan's leak-and-error report. Under sanitizer builds
we want that report. Switch to plain exit() when __SANITIZE_ADDRESS__
is defined; native posix shutdown still uses _exit().
Two related fixes for HRT callout queue corruption under lockstep SIH:
1. WorkQueue::Run() popped a WorkItem, released the work-queue lock,
then called work->Run(). If another thread called Deinit on the
item in that window, wq->Remove was a no-op (item already popped)
and Deinit returned while Run() was still executing. Run() could
then call ScheduleDelayed → hrt_call_after on an object that the
caller of Deinit has since destructed and reused via placement new
— the hrt_call metadata ends up living inside another object's
fields, which later overwrite the flink pointer and tear the hrt
callout queue.
Fix: WorkItem carries a _run_in_progress atomic flag set by
WorkQueue::Run around work->Run(). Deinit spins on that flag after
wq->Remove so it cannot return while a Run() is still executing.
WorkQueue captures its worker tid in the constructor (the ctor
runs on the worker thread) so Deinit can skip the self-wait when
called from inside Run() itself (e.g. should_exit() paths that
invoke ScheduleClear).
2. _hrt_lock was a px4_sem_t and hrt_call_invoke unlocked around the
callback so callbacks could re-enter hrt_call_*. That also exposed
the queue to other threads mid-invocation. Switch _hrt_lock to a
PTHREAD_MUTEX_RECURSIVE pthread_mutex and hold it across the
callback — matches NuttX's enter_critical_section() nesting
semantics, lets callbacks reschedule themselves safely, and
prevents concurrent queue manipulation.
Together these eliminate the HRT queue tearing observed in the
SIH-at-20x MAVSDK integration soak (torn flink chains with the tail
unreachable from the head; orphan nodes pointing into reused
memory).
Move the helper from the header into WorkQueue.cpp. Drop the
SignalWorkerThread() call from request_stop() — request_stop sets
_should_exit but the worker is woken up via the existing exit path
in Detach already, so the extra signal here is redundant.
fuzztest's coverage instrumentation is incompatible with Thread
Sanitizer. Add px4_setup_gtest_without_fuzztest() macro to
cmake/px4_add_gtest.cmake that fetches GTest standalone and stubs out
fuzztest cmake functions. Guard all fuzztest-specific code on
TARGET fuzztest::fuzztest so it compiles cleanly without fuzztest.
Split set_absolute_time() into two phases: iterate the waiter list under
_timed_waits_mutex, then broadcast under a separate _broadcast_mutex.
This eliminates the lock-order-inversion cycle between cond_timedwait()
(holds passed_lock -> acquires _timed_waits_mutex) and set_absolute_time()
(held _timed_waits_mutex -> acquired passed_lock).
Fix tests to have each thread lock its own mutex before calling
cond_timedwait, as required by POSIX (the calling thread must own the
mutex passed to pthread_cond_wait). The previous cross-thread ownership
caused TSAN's deadlock detector to overflow its 64-entry limit.
Move registerCallback() after all member variable writes in
SensorSelectionUpdate() and reorder Start() to complete initial
sensor selection before registering the sensor_selection callback.
This prevents Run() from reading partially-written members like
_selected_sensor_device_id and _filter_sample_rate_hz on the work
queue thread while Start() is still initializing them.
This fixes TSAN issues in unit tests.
This fixes TSAN issues on destruction of AtomicTransaction happening
during the unit tests.
The static _MutexHolder instance wraps a pthread_mutex_t in a C++ class,
creating a destructor ordering problem during process exit when worker
threads may still hold the lock. Replace with a raw pthread_mutex_t
initialized via pthread_once, eliminating the C++ destructor entirely.
perf counters are written on the hot path (perf_begin / perf_end /
perf_count) and read concurrently by the `perf` command and the logger,
which ThreadSanitizer flags as data races.
Wrap the counter fields in PerfVar: px4::atomic on POSIX (where TSan runs
and native 64-bit atomics are essentially free) and a plain scalar on
flight hardware (NuttX/QURT). The call sites are identical in both
configurations.
The embedded path matters because a 64-bit atomic on Cortex-M has no
native instruction and lowers to a locked, interrupt-disabling
__atomic_* sequence. Measured on fmu-v6x (microbench microbench_perf):
perf_count 73.7 ns -> 171.8 ns
perf_begin+end 982.9 ns -> 1831.7 ns
~2x on instrumentation that wraps many module Run() loops. These are
single-writer diagnostic counters; the only effect of the plain path is
that a concurrent reader may observe an off-by-one torn read of a 64-bit
field, which is benign and self-correcting (the counter list itself is,
and remains, mutex-protected). The conditional version restores embedded
performance (76.6 ns / 1015 ns) while keeping SITL TSan-clean.
Single TSan/ASan-clean implementation that works on both POSIX and NuttX:
- size()/byteSize() take the AtomicTransaction lock so they don't race
with store() updating _next_slot/_n_slots.
- _grow() snapshots _n_slots under the lock for sizing the malloc, so
another thread's growth between the malloc and the memcpy can't make
the memcpy exceed the allocation. Both platforms use the unlock-
around-malloc + CAS-retry pattern: NuttX requires it (malloc can't
be called with IRQs disabled), and on POSIX it's safe because the
AtomicTransaction mutex still serializes everything else and the
brief unlock window doesn't touch shared state.
- ~DynamicSparseLayer() takes the lock, zeros _next_slot / _n_slots,
stores nullptr into _slots, then frees the buffer. This fixes the
heap-use-after-free TSan caught at process exit on POSIX, where the
static DynamicSparseLayer instances in parameters.cpp are torn down
by cxa_atexit while other threads (commander etc.) are still calling
param_get(). Any reader that acquires the lock post-destruction sees
_next_slot == 0 and falls through to the parent layer; the parent
chain in parameters.cpp is in reverse-declaration order, so the
parent is still alive when the child is destroyed.
Add DynamicSparseLayerTest with concurrent stress tests that reproduce
the races. ConcurrentMultipleWriters pre-allocates to TOTAL to avoid
the known ABA limitation in the CAS retry loop with concurrent _grow().
Drive-by: drop the unused void *p member from param_value_u — nothing
in the tree reads or writes it, and on 64-bit POSIX it was silently
doubling the size of every parameter slot.
Originally found while running the new SIH-based CI tests.
- Separate stopped motor mask into separate state variables (one for
stopping due to flight phase, one for stopping due to NaN thrust in
each direction) and combine them on demand in getStoppedMotors. This
removes the need of hard to follow &= and |= operation involving
shared state.
- use the new nanToZero to clean _thrust_sp for CA internal use
- Introduce lateral/longitudinal/vertical motor masks in base
ActuatorEffecctiveness (and remove previous from individual
Effectiveness classes)
- Populate the bitmasks from all relevant effectivenesses by calling
the new _rotors.setMotorDirectionBitmasks()
- isAlignedWithCoordinateAxis checks axis alignment with given
angle tolerance
- motor stopping is the same as before, using the result of
getStoppedMotors
