fix(device): synchronize the status-transfer flags and stop trusting cancel's NOT_FOUND - #8
Merged
Merged
Conversation
…cancel's NOT_FOUND PR #7's teardown fix is correct in shape but was not actually synchronized. Two defects, both in the uvc_close() path it added. status_xfer_submitted is written by _uvc_status_callback() on the libusb event thread and read by uvc_stop_status_xfer() on the closing thread. status_mutex was added for exactly that, and its own comment claims both flags are read and written under it, but three accesses sat outside it: the callback's terminal-status clear, the closing thread's poll loop, and uvc_free_devh()'s check. volatile supplies neither atomicity nor a happens-before edge, so on the weakly-ordered aarch64 this fork ships on, the close can observe the flag clear while the callback's earlier stores are still invisible and free both the transfer and the handle out from under it. Every access now goes through status_mutex and both flags lose the misleading volatile. The bounded drain takes and drops the mutex per iteration rather than spanning its sleep, which would block the callback it waits for. The flag is also set before libusb_submit_transfer() rather than after, so a callback completing between the two statements can no longer have its clear overwritten with a stale 1. Separately, a cancel returning LIBUSB_ERROR_NOT_FOUND was treated as drained. libusb documents that code as "not in progress, already complete, or already cancelled", and in the last case the callback has not run yet - so the close released the interface, freed a transfer whose cancellation was still pending (undefined behaviour by libusb's own contract) and freed the devh that the pending callback dereferences. The callback is now the only thing that clears the flag, and the existing bounded wait plus quarantine is the only exit. No deadlock is introduced: the lock order is uniform and one-way, status_mutex first and libusb entry points under it, and libusb invokes callbacks from its event thread with no internal transfer lock held and documents cancellation as asynchronous, so nothing re-enters status_mutex from inside libusb. A new libuvc.race case drives the real uvc_close() against a real event thread over repeated iterations, and LIBUVC_SANITIZE plus a CI job fail on any ThreadSanitizer report. On the pre-fix code TSan reports data races in _uvc_status_callback() and uvc_free_devh(), a heap-use-after-free in _uvc_status_callback(), and a destroy-of-a-locked-mutex in uvc_free_devh(). Also closes a coverage gap: every teardown case used VideoControl at interface 0 with at most one VideoStreaming interface, which a loop that merely special-cased index 0 would satisfy. Two cases now drive a nonzero VideoControl index with three scattered VideoStreaming interfaces. No production change was needed for them.
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
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.
What
Two fixes in the
uvc_close()teardown path that PR #7 introduced, plus the testcoverage that proves both. PR #7's actual logic — VideoStreaming interfaces released
before VideoControl, status transfer stopped before its interface is released — is
correct and generic and is not restructured here.
status_xfer_submittedis now genuinely thread-safe. It is written by_uvc_status_callback()on the libusb event thread and read byuvc_stop_status_xfer()on the closing thread.status_mutexwas added for exactlythat, and its own comment claims "Both flags below are read and written under it" —
but three accesses sat outside it: the callback's terminal-status clear
(
device.c:2211), the closing thread's poll loop, anduvc_free_devh()'s check.Every access now goes through the mutex, and both flags drop the misleading
volatile.A cancel reporting
LIBUSB_ERROR_NOT_FOUNDis no longer treated as drained.Why
On (1) —
volatilein C supplies neither atomicity nor any happens-before edge; itis not among the things that establish one (only
_Atomicand mutexes are), and theconcurrent accesses are a data race outright. That matters on the shipping target:
RK3588 is aarch64, weakly ordered. The closing thread can observe the flag clear while
the callback's earlier stores are still invisible, then free the transfer and the
handle while the callback is still inside both — reintroducing the same use-after-free
class the
has_quarantined_*machinery exists to prevent, one indirection lower.There was also an ordering bug:
uvc_open_internal()set the flag afterlibusb_submit_transfer(), so a callback that completed in between had its clearoverwritten with a stale 1. It is now set before the submit, under the mutex.
On (2) — libusb documents
NOT_FOUNDas "not in progress, already complete, oralready cancelled", and in that last case the completion callback has not run yet.
It also documents that freeing a transfer whose cancellation is still pending is
undefined behaviour. The old code returned success immediately on
NOT_FOUND, so theclose skipped its drain, released the interface, freed that transfer, and freed the
devhthe pending callback dereferences. PR #7's comment is right that the callbackwill not resubmit — but it still dereferences
devh, which is the lifetimequestion, not the resubmission question. Fixed by deleting the shortcut: the callback
is now the only thing that clears the flag, and the existing bounded wait (500 ms) plus
quarantine is the only exit, so a callback that never arrives degrades to the safe,
already-designed intentional leak. Net less code.
Why a mutex and not
_Atomicpthread.his already an unconditional include oflibuvc_internal.h, so this addszero new portability surface —
<stdatomic.h>would, since the library still builds formacOS/Windows (MSVC only got it in VS 17.5) and the project sets no
C_STANDARDat all.An unlock/lock pair is also a full release/acquire, so it orders everything the
callback touched before the free, not just the flag. And the mutex already existed with
this contract written on it — the fix makes the code match its documentation rather than
adding a second parallel mechanism. Cost is nil: the status endpoint fires at bInterval
(8–32 ms) and the stop path runs once per close.
Why it cannot deadlock
status_mutexfirst, libusb entry points under it,never the reverse. Callback:
status_mutex→libusb_submit_transfer. Stop:status_mutex→libusb_cancel_transfer. libusb cannot takestatus_mutex.transfer lock held, and documents cancellation as asynchronous — so there is no
synchronous callback from inside
libusb_cancel_transfer()to self-deadlock anon-recursive mutex, and no path back into
status_mutexfrom inside libusb. Taking amutex in the callback is the documented requirement ("your callback functions MUST be
thread-safe"), and PR fix(device): release every claimed interface and stop the status transfer on close #7 already did it on the resubmit path.
nanosleep— that would blockthe very callback it waits for. It locks per iteration; this is commented in-code so a
future "simplification" cannot reintroduce it.
adds no new edge to the lock graph.
has_quarantined_status_xfer/has_quarantined_streamwere audited and areclosing-thread-only.
status_xfer_stoppingwas already correctly locked on both sides.How to verify
Both directions were captured. Stashing
src/device.c+libuvc_internal.hback tof3eda76while keeping the new tests, under TSan: 39 ThreadSanitizer warnings and 2failing cases —
data raceatdevice.c:2211/2234/2235(_uvc_status_callback) and1837/1838/1840/1842(uvc_free_devh)heap-use-after-freeatdevice.c:2233/2234/2236in_uvc_status_callbackdestroy of a locked mutexatdevice.c:1840inuvc_free_devh— the closedestroying
status_mutexand freeingdevhwhile the callback is inside itlibuvc.teardown.cancel_not_found_still_drainsalso fails with no sanitizer at all.With the fix restored: 27/27 green, 0 TSan reports. Both shared-library CI variants
(auto-detach ON and OFF) build clean, and
gcc -Wall -Wextraonsrc/device.creportsthe same 16 pre-existing
-Wunused-parameterwarnings before and after — no new ones.New tests
libuvc.race.close_races_status_callback— drives the realuvc_close()against areal libusb event thread over 200 iterations. It asserts no submission lands at or
after the first interface release, and that the close never had to quarantine (which is
what proves the drain still completes — holding the mutex across its sleep would turn
every close into a timeout). The harness mirrors production's lock order exactly so it
cannot invent an inversion the real code does not have.
libuvc.teardown.cancel_not_found_still_drains— aNOT_FOUNDcancel with thecompletion still pending must land the callback before the first USB operation of
the teardown, not somewhere in the middle of it.
libuvc.teardown.sparse_interfaces_control_released_last(VC=3, VS={1,5,7}, with astatus endpoint) and
libuvc.teardown.high_index_interfaces_released(VC=2,VS={9,17,24}, none) — closes the coverage gap where every case used VideoControl at
interface 0 with at most one VideoStreaming interface, a shape a loop that merely
special-cased index 0 would also satisfy. Synthetic indices through one parameterized
helper, deliberately not any real device's descriptor layout. No production change
was needed — PR fix(device): release every claimed interface and stop the status transfer on close #7's release logic was already generic, which is now proven.
No existing test was changed, skipped, or weakened.
Risks
NOT_FOUNDpath. A close that previously returnedimmediately now waits for the completion callback. In the common case (
NOT_FOUNDbecause the callback already ran) the flag is already clear and the drain exits with
zero added latency. The worst case is the existing 500 ms
LIBUVC_STATUS_STOP_TIMEOUT_MSbound followed by the existing quarantine — thealready-designed safe outcome, not a new failure mode.
volatileremoval is deliberate, not cosmetic. It only remains correct becauseevery access is now under the mutex; adding an unlocked read back would be a silent
regression. The header documents this and the race case would catch it under TSan.
thread-sanitizeris scoped with-R 'libuvc\.(teardown|race)\.'onpurpose: the descriptor/negotiation/transfer suites
--wrapfree(), which asanitized build replaces, so including them would report a mismatched allocator rather
than anything about this code.
LIBUVC_SANITIZEdefaults to off; normal builds areunaffected.
should still behave identically — the observable teardown sequence is unchanged on
every path that was already correct.