fix(device): synchronize the status-transfer flags and stop trusting cancel's NOT_FOUND

.github/workflows/build.yml

@@ -129,8 +129,12 @@ jobs:
               "libuvc.teardown.status_xfer_stops_before_control_release",
               "libuvc.teardown.every_claimed_interface_released_control_last",
               "libuvc.teardown.no_status_endpoint_unchanged",
-              "libuvc.teardown.undeliverable_status_xfer_quarantines"
-            ] | sort) and (.tests | length == 23)
+              "libuvc.teardown.sparse_interfaces_control_released_last",
+              "libuvc.teardown.high_index_interfaces_released",
+              "libuvc.teardown.cancel_not_found_still_drains",
+              "libuvc.teardown.undeliverable_status_xfer_quarantines",
+              "libuvc.race.close_races_status_callback"
+            ] | sort) and (.tests | length == 27)
           ' "$result_dir/inventory.json" \
             2>&1 | tee "$result_dir/inventory-check.log"
 
@@ -179,3 +183,57 @@ jobs:
       - name: Enforce ccache bound
         if: ${{ always() }}
         run: ccache -c
+
+  # The teardown path hands a device handle between the closing thread and the
+  # libusb event thread, so the ordering assertions above cannot see a missing
+  # happens-before edge on their own -- only an instrumented run can. This job is
+  # what makes libuvc.race.close_races_status_callback a proof rather than a
+  # stress test; TSan works from vector clocks, so it fails on unsynchronized
+  # accesses even when they never overlap in wall-clock time.
+  thread-sanitizer:
+    name: ThreadSanitizer (teardown + race)
+    runs-on: ubuntu-24.04
+
+    steps:
+      - uses: actions/checkout@v7
+
+      - name: Install dependencies
+        run: |
+          sudo apt-get update -qq
+          sudo apt-get install -y \
+            build-essential cmake pkg-config libusb-1.0-0-dev libjpeg-dev
+
+      - name: Configure with -fsanitize=thread
+        run: |
+          cmake -S . -B build/tsan \
+            -DCMAKE_POLICY_VERSION_MINIMUM=3.5 \
+            -DCMAKE_BUILD_TYPE=Debug \
+            -DCMAKE_BUILD_TARGET=Static \
+            -DBUILD_SHARED_LIBS=OFF \
+            -DBUILD_EXAMPLE=OFF \
+            -DBUILD_TEST=OFF \
+            -DBUILD_TESTING=ON \
+            -DLIBUVC_SANITIZE=thread
+
+      - name: Build
+        run: cmake --build build/tsan --parallel
+
+      # Scoped to the concurrent teardown cases on purpose: the descriptor,
+      # negotiation and transfer suites --wrap free(), which a sanitized build
+      # replaces, so routing those calls back to __real_free would report a
+      # mismatched allocator rather than anything about this code.
+      - name: Run teardown and race cases under TSan
+        run: |
+          ctest --test-dir build/tsan --output-on-failure \
+            -R 'libuvc\.(teardown|race)\.'
+
+      - name: Fail on any ThreadSanitizer report
+        run: |
+          set -o pipefail
+          ./build/tsan/uvc_status_race_assertions \
+            --case close_races_status_callback 2>&1 | tee tsan.log
+          if grep -q "ThreadSanitizer" tsan.log; then
+            echo "FAIL: ThreadSanitizer reported a problem in the close/callback race"
+            exit 1
+          fi
+          echo "PASS: no ThreadSanitizer reports"

CHANGELOG.ceralive.md

@@ -65,6 +65,63 @@ the upstream history, see `changelog.txt`.
   (`libuvc.teardown.*`) that `--wrap` the libusb entry points into an ordered
   operation log and drive the real `uvc_close()`.
 
+- **Data race and use-after-free in the teardown fix above.** The stop path was
+  correct in shape but not actually synchronized, and both defects are on the
+  same `uvc_close()` the entry above added:
+
+  1. **`status_xfer_submitted` was shared across threads as bare `volatile`.**
+     It is written by `_uvc_status_callback()` on the libusb event thread and
+     read by `uvc_stop_status_xfer()` on the closing thread. Three of those
+     accesses sat outside `status_mutex`: the callback's terminal-status clear,
+     the closing thread's poll loop, and `uvc_free_devh()`'s check. `volatile`
+     supplies neither atomicity nor any happens-before edge — C classifies the
+     concurrent accesses as a data race outright, and on the weakly-ordered
+     aarch64 this fork ships on, the closing thread can observe the flag clear
+     while the callback's earlier stores are still invisible, then free both the
+     transfer and the handle out from under it. Every read and write of
+     `status_xfer_submitted` and `status_xfer_stopping` is now made holding
+     `status_mutex`, and both lost their misleading `volatile`. The bounded drain
+     takes and drops the mutex per iteration rather than spanning its sleep;
+     holding it would block the very callback it waits for. The flag is also set
+     BEFORE `libusb_submit_transfer()` in `uvc_open_internal()` instead of after,
+     so a callback that completes while the opening thread is still between the
+     two statements can no longer have its clear overwritten with a stale 1.
+
+  2. **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 completion callback has not
+     run yet. `uvc_stop_status_xfer()` returned success immediately, so
+     `uvc_close()` went on to release the interface, free a transfer whose
+     cancellation was still pending (undefined behaviour by libusb's own
+     contract) and free the `devh` that the pending callback dereferences.
+     `_uvc_status_callback()` is now the only thing that clears
+     `status_xfer_submitted`, and the existing bounded wait plus quarantine is
+     the only exit — so a callback that never arrives degrades to the safe,
+     already-designed leak instead of a use-after-free.
+
+  No deadlock is introduced. The lock order is uniform and one-way —
+  `status_mutex` first, libusb entry points under it, never the reverse — and
+  libusb invokes transfer callbacks from its event-handling thread with no
+  internal transfer lock held, with cancellation documented as asynchronous, so
+  there is no path back into `status_mutex` from inside libusb.
+
+  Covered by a new `libuvc.race.close_races_status_callback` case that drives the
+  real `uvc_close()` against a real libusb event thread over repeated iterations,
+  by `libuvc.teardown.cancel_not_found_still_drains`, and by a new
+  `LIBUVC_SANITIZE` CMake option with a CI job that fails 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()`.
+
+- **Teardown release order is now covered on a generic interface layout.** The
+  original cases all used VideoControl at interface 0 with at most one
+  VideoStreaming interface — the reproduction device's shape, which a release
+  loop that merely special-cased index 0 would also have satisfied.
+  `libuvc.teardown.sparse_interfaces_control_released_last` and
+  `libuvc.teardown.high_index_interfaces_released` drive a nonzero VideoControl
+  index with three scattered VideoStreaming interfaces, at low and high indices.
+  No production change; the existing logic was already generic.
+
 - **Reject inconsistent UVC descriptor lengths before parser dispatch.** The
   VideoControl and VideoStreaming descriptor scanners now return
   `UVC_ERROR_INVALID_DEVICE` for a declared length below the three-byte header

CMakeLists.txt

@@ -29,6 +29,25 @@ option(ENABLE_UVC_DEBUGGING "Enable UVC debugging" OFF)
 option(LIBUVC_AUTO_DETACH_KERNEL_DRIVER
   "Auto-detach the kernel driver (e.g. uvcvideo) when claiming UVC interfaces (CeraLive)"
   ON)
+set(LIBUVC_SANITIZE "" CACHE STRING
+  "Sanitizer to build library and tests with: thread, address, or empty (CeraLive)")
+set_property(CACHE LIBUVC_SANITIZE PROPERTY STRINGS "" thread address)
+
+if(LIBUVC_SANITIZE)
+  if(NOT CMAKE_C_COMPILER_ID MATCHES "GNU|Clang")
+    message(FATAL_ERROR "LIBUVC_SANITIZE requires a GNU-compatible compiler")
+  endif()
+  # Instrumentation has to reach the library, not just the test executable:
+  # the races being hunted live in src/device.c, on the libusb event thread.
+  add_compile_options(-fsanitize=${LIBUVC_SANITIZE} -fno-omit-frame-pointer -g)
+  # Not add_link_options(): that needs CMake 3.13 and this project declares 3.1.
+  foreach(linker_flags
+      CMAKE_EXE_LINKER_FLAGS
+      CMAKE_SHARED_LINKER_FLAGS)
+    set(${linker_flags} "${${linker_flags}} -fsanitize=${LIBUVC_SANITIZE}")
+  endforeach()
+  message(STATUS "Building with -fsanitize=${LIBUVC_SANITIZE}")
+endif()
 
 set(libuvc_DESCRIPTION "A cross-platform library for USB video devices")
 set(libuvc_URL "https://github.com/libuvc/libuvc")
@@ -162,12 +181,14 @@ if(BUILD_TESTING)
   add_executable(uvc_negotiation_assertions tests/negotiation_assertions.c)
   add_executable(uvc_transfer_assertions tests/transfer_assertions.c)
   add_executable(uvc_teardown_assertions tests/teardown_assertions.c)
+  add_executable(uvc_status_race_assertions tests/status_race_assertions.c)
 
   foreach(test_target
       uvc_descriptor_assertions
       uvc_negotiation_assertions
       uvc_transfer_assertions
-      uvc_teardown_assertions)
+      uvc_teardown_assertions
+      uvc_status_race_assertions)
     target_link_libraries(${test_target}
       PRIVATE uvc_static LibUSB::LibUSB ${threads})
     if(JPEG_FOUND)
@@ -184,6 +205,10 @@ if(BUILD_TESTING)
     PROPERTY LINK_FLAGS " -Wl,--wrap=libusb_submit_transfer -Wl,--wrap=libusb_free_transfer -Wl,--wrap=free")
   set_property(TARGET uvc_teardown_assertions APPEND_STRING
     PROPERTY LINK_FLAGS " -Wl,--wrap=libusb_submit_transfer -Wl,--wrap=libusb_cancel_transfer -Wl,--wrap=libusb_release_interface -Wl,--wrap=libusb_attach_kernel_driver -Wl,--wrap=libusb_set_interface_alt_setting -Wl,--wrap=libusb_close -Wl,--wrap=libusb_unref_device -Wl,--wrap=nanosleep")
+  # nanosleep is deliberately NOT wrapped here: the race case needs uvc_close()'s
+  # bounded drain to really sleep so the event thread really interleaves with it.
+  set_property(TARGET uvc_status_race_assertions APPEND_STRING
+    PROPERTY LINK_FLAGS " -Wl,--wrap=libusb_submit_transfer -Wl,--wrap=libusb_cancel_transfer -Wl,--wrap=libusb_release_interface -Wl,--wrap=libusb_attach_kernel_driver -Wl,--wrap=libusb_set_interface_alt_setting -Wl,--wrap=libusb_close -Wl,--wrap=libusb_unref_device")
 
   foreach(case_name
       h264
@@ -216,13 +241,23 @@ if(BUILD_TESTING)
       status_xfer_stops_before_control_release
       every_claimed_interface_released_control_last
       no_status_endpoint_unchanged
+      sparse_interfaces_control_released_last
+      high_index_interfaces_released
+      cancel_not_found_still_drains
       undeliverable_status_xfer_quarantines)
     add_test(NAME libuvc.teardown.${case_name}
       COMMAND uvc_teardown_assertions --case ${case_name})
   endforeach()
   set_tests_properties(
     libuvc.teardown.undeliverable_status_xfer_quarantines
     PROPERTIES TIMEOUT 5)
+
+  add_test(NAME libuvc.race.close_races_status_callback
+    COMMAND uvc_status_race_assertions --case close_races_status_callback)
+  # Generous because the case is deliberately iterated, and a sanitized build
+  # multiplies that; a hang here means the drain deadlocked, not that it is slow.
+  set_tests_properties(libuvc.race.close_races_status_callback
+    PROPERTIES TIMEOUT 180)
 endif()
 
 if(BUILD_EXAMPLE)

README.md

@@ -64,29 +64,55 @@ Linux CTest suite. Configure, build, inspect, and run its static build with:
       -DBUILD_TESTING=ON
     cmake --build build/regression --parallel
     ctest --test-dir build/regression --show-only=json-v1 \
-      | jq -e '.tests | length == 19'
+      | jq -e '.tests | length == 27'
     ctest --test-dir build/regression --output-on-failure
 
-The 23 cases are grouped as descriptor (11: `h264`, `h265`,
+The 27 cases are grouped as descriptor (11: `h264`, `h265`,
 `truncated_format`, `truncated_frame`, `degenerate_h26x`,
 `scanner_vc_header_short`, `scanner_vc_oversized`, `scanner_vc_zero`,
 `scanner_vs_header_short`, `scanner_vs_oversized`, `scanner_vs_zero`),
 negotiation (5: `h264`, `h265`, `near_match`, `probe_set_error`,
 `probe_get_error`), transfer (3: `terminal_statuses`, `retry_success`,
-`retry_failure`), and teardown (4: `status_xfer_stops_before_control_release`,
+`retry_failure`), teardown (7: `status_xfer_stops_before_control_release`,
 `every_claimed_interface_released_control_last`,
-`no_status_endpoint_unchanged`, `undeliverable_status_xfer_quarantines`).
+`no_status_endpoint_unchanged`, `sparse_interfaces_control_released_last`,
+`high_index_interfaces_released`, `cancel_not_found_still_drains`,
+`undeliverable_status_xfer_quarantines`), and race
+(1: `close_races_status_callback`).
 CI runs this suite without camera hardware on Ubuntu 22.04
 and Ubuntu 24.04. See
 `docs/evidence/uvc-camera-compat-stability.md` for its exact scope.
 
-Adjust the `jq` length assertion above to `23` when running it.
+### Sanitized builds
+
+`LIBUVC_SANITIZE` builds the library **and** the tests with a sanitizer —
+instrumenting only the test executable would miss the interesting code, since
+the teardown races live in `src/device.c` and run on the libusb event thread:
+
+    cmake -S . -B build/tsan \
+      -DCMAKE_POLICY_VERSION_MINIMUM=3.5 \
+      -DCMAKE_BUILD_TYPE=Debug \
+      -DCMAKE_BUILD_TARGET=Static \
+      -DBUILD_SHARED_LIBS=OFF \
+      -DBUILD_EXAMPLE=OFF \
+      -DBUILD_TEST=OFF \
+      -DBUILD_TESTING=ON \
+      -DLIBUVC_SANITIZE=thread
+    cmake --build build/tsan --parallel
+    ctest --test-dir build/tsan --output-on-failure -R 'libuvc\.(teardown|race)\.'
+
+`libuvc.race.close_races_status_callback` is the case this exists for: it drives
+`uvc_close()` against a real libusb event thread, and only an instrumented run
+can see a missing happens-before edge between them. Keep the `-R` filter — the
+descriptor, negotiation and transfer suites `--wrap` `free()`, which a sanitized
+build replaces, so including them reports a mismatched allocator rather than
+anything about this code. A dedicated CI job runs exactly this.
 
 ### Device teardown contract
 
-`uvc_close()` owns the whole USB teardown and must keep two invariants that are
-not visible from the call site — both are regression-locked by the
-`libuvc.teardown.*` cases:
+`uvc_close()` owns the whole USB teardown and must keep four invariants that are
+not visible from the call site — all regression-locked by the
+`libuvc.teardown.*` and `libuvc.race.*` cases:
 
 1. **The VideoControl status interrupt transfer is stopped before any interface
    is released.** It re-arms itself from `_uvc_status_callback()`, so a
@@ -99,6 +125,22 @@ not visible from the call site — both are regression-locked by the
    failed negotiation can leave the streaming interface claimed, and reattaching
    the driver to VideoControl is what triggers `uvcvideo`'s probe — a probe that
    claims the streaming interfaces itself, so it must run after they are free.
+   The order is derived from `devh->claimed` and
+   `info->ctrl_if.bInterfaceNumber`, never assumed: a UVC function may sit at any
+   interface index and expose several VideoStreaming interfaces.
+3. **`status_xfer_submitted` and `status_xfer_stopping` are read and written
+   ONLY under `status_mutex`, on both threads.** They are shared between the
+   closing thread and the libusb event thread. `volatile` (what they used to be)
+   gives neither atomicity nor a happens-before edge, so the close could observe
+   the transfer "done" and free it — and the handle — while the callback was
+   still inside both. The mutex's unlock/lock pair is what orders the callback's
+   last write before the free.
+4. **A cancel reporting `LIBUSB_ERROR_NOT_FOUND` is still waited out.** libusb
+   documents that code as *"not in progress, already complete, or already
+   cancelled"*, and in the last of those the completion callback has not run yet;
+   freeing the transfer there is undefined behaviour by libusb's own contract.
+   `_uvc_status_callback()` is therefore the only thing that ever clears
+   `status_xfer_submitted`, and the bounded drain is the only way out of the stop.
 
 ## Developing with libuvc