diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 387e960..e3bf678 100644 --- a/.github/workflows/build.yml +++ b/.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" diff --git a/CHANGELOG.ceralive.md b/CHANGELOG.ceralive.md index 9bf6410..926973b 100644 --- a/CHANGELOG.ceralive.md +++ b/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 diff --git a/CMakeLists.txt b/CMakeLists.txt index 3a301a6..92cfdd8 100644 --- a/CMakeLists.txt +++ b/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,6 +241,9 @@ 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}) @@ -223,6 +251,13 @@ if(BUILD_TESTING) 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) diff --git a/README.md b/README.md index 1e6d435..3346032 100644 --- a/README.md +++ b/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 diff --git a/include/libuvc/libuvc_internal.h b/include/libuvc/libuvc_internal.h index 110bfdf..fd19f4a 100644 --- a/include/libuvc/libuvc_internal.h +++ b/include/libuvc/libuvc_internal.h @@ -346,26 +346,55 @@ struct uvc_device_handle { * device handle too, mirroring the stream-handle leak. Never cleared. */ uint8_t has_quarantined_stream; /** Serializes _uvc_status_callback()'s decide-and-resubmit against - * uvc_stop_status_xfer()'s stop-and-cancel. Both flags below are read and - * written under it. Without the mutex the two interleave: the callback can - * read status_xfer_stopping as 0, lose the CPU, and issue its resubmission - * after uvc_stop_status_xfer() has already returned and uvc_close() has - * released the VideoControl interface -- the exact URB-after-release the stop - * exists to prevent. Initialized in uvc_open_internal(), destroyed in - * uvc_free_devh(). */ + * uvc_stop_status_xfer()'s stop-and-cancel. Without the mutex the two + * interleave: the callback can read status_xfer_stopping as 0, lose the CPU, + * and issue its resubmission after uvc_stop_status_xfer() has already returned + * and uvc_close() has released the VideoControl interface -- the exact + * URB-after-release the stop exists to prevent. + * + * It is also the ONLY synchronization for the two flags below: EVERY read and + * EVERY write of either one, on either thread, is made holding this mutex. + * That is not bookkeeping tidiness -- it is what gives uvc_close() the + * happens-before edge it needs. Observing status_xfer_submitted == 0 through + * this mutex means the event thread's last touch of `status_xfer` and of this + * handle is ordered BEFORE the release/close/free that follows, because an + * unlock/lock pair is a release/acquire pair. A bare `volatile` flag (what + * this used to be) supplies neither atomicity nor that edge: C makes the + * concurrent accesses a data race outright, and on the weakly-ordered aarch64 + * the fork actually ships on, the closing thread can see the flag clear while + * the callback's earlier stores are still invisible -- then free the transfer + * and the handle out from under it. + * + * The lock order is uniform and one-way: status_mutex is taken FIRST and libusb + * entry points are called under it, never the reverse. libusb invokes transfer + * callbacks with no internal transfer lock held and cancellation is + * asynchronous, so there is no path back into this mutex from inside libusb and + * no inversion to deadlock on. The bounded wait in uvc_stop_status_xfer() must + * therefore drop the mutex around its sleep -- holding it would block the very + * callback it waits for. + * + * Initialized in uvc_open_internal(), destroyed in uvc_free_devh(). */ pthread_mutex_t status_mutex; - /** Non-zero while `status_xfer` is submitted to libusb -- set when - * uvc_open_internal() submits it, cleared by _uvc_status_callback() on the - * libusb event thread once the transfer is no longer resubmitted. Polled by - * uvc_stop_status_xfer() on the closing thread, hence `volatile`. */ - volatile uint8_t status_xfer_submitted; + /** Non-zero while `status_xfer` is submitted to libusb -- set by + * uvc_open_internal() before it submits, cleared by _uvc_status_callback() on + * the libusb event thread once the transfer is no longer resubmitted. Polled + * by uvc_stop_status_xfer() on the closing thread. Guarded by status_mutex on + * every access; see there for why `volatile` was not enough. + * + * It is set BEFORE the submit, not after: the callback can run to completion on + * the event thread while the opening thread is still between the two + * statements, and a post-submit store would then overwrite the callback's clear + * with a stale 1, permanently marking a transfer libusb no longer owns as + * in-flight. */ + uint8_t status_xfer_submitted; /** Set by uvc_stop_status_xfer() BEFORE it cancels `status_xfer`: tells * _uvc_status_callback() to stop resubmitting. Without it the callback keeps * re-arming the interrupt URB on the VideoControl interface AFTER uvc_close() * released that interface and reattached the kernel driver -- usbfs then logs * "did not claim interface N before use" and steals the interface back, so the - * final libusb_close() leaves it bound to no driver at all. Never cleared. */ - volatile uint8_t status_xfer_stopping; + * final libusb_close() leaves it bound to no driver at all. Never cleared. + * Guarded by status_mutex on every access. */ + uint8_t status_xfer_stopping; /** Set when uvc_stop_status_xfer()'s bounded wait expired with `status_xfer` * still owned by libusb. Quarantines the handle for the same reason * has_quarantined_stream does: a late callback dereferences devh. */ diff --git a/src/device.c b/src/device.c index 3da64e0..e83de3c 100644 --- a/src/device.c +++ b/src/device.c @@ -391,18 +391,26 @@ static uvc_error_t uvc_open_internal( _uvc_status_callback, internal_devh, 0); + /* Marked in-flight BEFORE the submit, and under the mutex, because the + * moment libusb accepts the transfer the event thread may run the callback + * to completion -- including clearing this flag on a terminal status. + * Setting it afterwards would overwrite that clear with a stale 1 and leave + * a transfer libusb no longer owns looking permanently submitted. libusb + * itself is entered under status_mutex here for the same reason + * uvc_stop_status_xfer() does: one uniform lock order. */ + pthread_mutex_lock(&internal_devh->status_mutex); + internal_devh->status_xfer_submitted = 1; ret = libusb_submit_transfer(internal_devh->status_xfer); UVC_DEBUG("libusb_submit_transfer() = %d", ret); + if (ret) + internal_devh->status_xfer_submitted = 0; + pthread_mutex_unlock(&internal_devh->status_mutex); if (ret) { fprintf(stderr, "uvc: device has a status interrupt endpoint, but unable to read from it\n"); goto fail; } - /* libusb now owns the transfer, and _uvc_status_callback() re-arms it after - * every completion. uvc_close() MUST stop it before releasing the - * VideoControl interface it rides on -- see uvc_stop_status_xfer(). */ - internal_devh->status_xfer_submitted = 1; } if (dev->ctx->own_usb_ctx && dev->ctx->open_devices == NULL @@ -1827,14 +1835,23 @@ uvc_error_t uvc_parse_vs( * @pre Streaming must be stopped, and threads must have died */ void uvc_free_devh(uvc_device_handle_t *devh) { + int submitted; + UVC_ENTER(); if (devh->info) uvc_free_device_info(devh->info); /* libusb forbids freeing a transfer it still owns. status_xfer_submitted is - * only still set on the quarantine path, which never reaches here. */ - if (devh->status_xfer && !devh->status_xfer_submitted) + * only still set on the quarantine path, which never reaches here -- but it is + * read under status_mutex anyway, both because that is the flag's contract and + * because the acquire is what orders the event thread's last write to + * `status_xfer` before this free. */ + pthread_mutex_lock(&devh->status_mutex); + submitted = devh->status_xfer_submitted; + pthread_mutex_unlock(&devh->status_mutex); + + if (devh->status_xfer && !submitted) libusb_free_transfer(devh->status_xfer); pthread_mutex_destroy(&devh->status_mutex); @@ -1866,12 +1883,20 @@ void uvc_free_devh(uvc_device_handle_t *devh) { * interrupt endpoint were affected, which is why it reproduced on one camera and * not another on the same board. * + * Returning 1 is a memory-ordering claim as much as a state claim: uvc_close() + * goes straight on to release interfaces, close the usbfs handle and free both + * `status_xfer` and `devh`, so the caller needs everything the event thread did + * inside _uvc_status_callback() to be ordered before that. Reading + * status_xfer_submitted under status_mutex is what supplies it -- the unlock in + * the callback and the lock here are a release/acquire pair. The flag was + * previously polled bare (`volatile`), which orders nothing across threads. + * * @return 1 when the transfer is confirmed no longer submitted, 0 on timeout. */ static int uvc_stop_status_xfer(uvc_device_handle_t *devh) { struct timespec poll; int waited_ms; - int cancelled; + int submitted; if (!devh->status_xfer) return 1; @@ -1883,29 +1908,37 @@ static int uvc_stop_status_xfer(uvc_device_handle_t *devh) { * outlive this call. */ pthread_mutex_lock(&devh->status_mutex); devh->status_xfer_stopping = 1; - if (!devh->status_xfer_submitted) { - pthread_mutex_unlock(&devh->status_mutex); - return 1; + submitted = devh->status_xfer_submitted; + if (submitted) { + /* The return value is deliberately not used to shortcut the wait. + * LIBUSB_ERROR_NOT_FOUND is documented as "not in progress, already + * complete, OR ALREADY CANCELLED" -- and in that last case the completion + * callback has not run yet. Treating NOT_FOUND as "done" (as this did) hands + * uvc_close() a transfer whose cancellation is still pending, which libusb + * documents as undefined behaviour to free, and lets the callback dereference + * a devh that is already free()d. So _uvc_status_callback() is the only thing + * that ever clears status_xfer_submitted, and the wait below is the only way + * out; a callback that genuinely never arrives hits the bound and quarantines + * the handle, which is the safe outcome either way. */ + libusb_cancel_transfer(devh->status_xfer); } - cancelled = libusb_cancel_transfer(devh->status_xfer) == LIBUSB_SUCCESS; pthread_mutex_unlock(&devh->status_mutex); - if (!cancelled) { - /* NOT_FOUND means libusb has already reaped it; the callback either ran - * under the lock above or will now observe status_xfer_stopping. */ - devh->status_xfer_submitted = 0; - return 1; - } - + /* The wait must NOT span the sleep holding status_mutex: the callback that + * clears the flag takes the same mutex, so a held lock would block the very + * completion this is waiting for and turn every cancellation into a timeout. */ poll.tv_sec = 0; poll.tv_nsec = (long) LIBUVC_STATUS_STOP_POLL_MS * 1000000L; for (waited_ms = 0; - devh->status_xfer_submitted && waited_ms < LIBUVC_STATUS_STOP_TIMEOUT_MS; + submitted && waited_ms < LIBUVC_STATUS_STOP_TIMEOUT_MS; waited_ms += LIBUVC_STATUS_STOP_POLL_MS) { nanosleep(&poll, NULL); + pthread_mutex_lock(&devh->status_mutex); + submitted = devh->status_xfer_submitted; + pthread_mutex_unlock(&devh->status_mutex); } - return devh->status_xfer_submitted ? 0 : 1; + return submitted ? 0 : 1; } /** @internal @@ -2208,7 +2241,14 @@ void LIBUSB_CALL _uvc_status_callback(struct libusb_transfer *transfer) { case LIBUSB_TRANSFER_CANCELLED: case LIBUSB_TRANSFER_NO_DEVICE: UVC_DEBUG("not processing/resubmitting, status = %d", transfer->status); + /* This is the completion uvc_stop_status_xfer()'s bounded wait is watching + * for -- a cancel lands here -- so the clear must go through status_mutex + * like every other access. The unlock is what publishes this thread's last + * touch of `transfer` and `devh` to the closing thread, which frees both as + * soon as it observes the flag. */ + pthread_mutex_lock(&devh->status_mutex); devh->status_xfer_submitted = 0; + pthread_mutex_unlock(&devh->status_mutex); UVC_EXIT_VOID(); return; case LIBUSB_TRANSFER_COMPLETED: diff --git a/tests/status_race_assertions.c b/tests/status_race_assertions.c new file mode 100644 index 0000000..4b93d79 --- /dev/null +++ b/tests/status_race_assertions.c @@ -0,0 +1,292 @@ +/* uvc_close() versus a REAL libusb event thread. + * + * teardown_assertions.c models the event thread without a thread: it delivers + * completions from inside the wraps, which pins the teardown ORDER but can never + * exhibit a data race, because only one thread ever exists. The bug this file + * covers is the opposite kind. 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, and both used to do it outside + * devh->status_mutex. `volatile` made that look synchronized while supplying + * neither atomicity nor any happens-before edge, so uvc_close() could observe the + * flag clear and go on to release the interface, close the handle and free both + * the transfer and the devh while the callback was still inside them. + * + * So here the event thread is real, and it re-arms the URB as fast as the + * production callback lets it while uvc_close() runs concurrently. Two things + * are then asserted per iteration: + * + * - no submission is recorded at or after the first interface release (the + * teardown invariant, now under genuine concurrency rather than a scripted + * interleaving), and + * - the close never had to quarantine, which is what proves the bounded drain + * still completes: holding status_mutex across its sleep would block the very + * callback it waits for and turn every single close into a timeout. + * + * Under a plain build this is a stress test. Under -DLIBUVC_SANITIZE=thread it is + * a proof: ThreadSanitizer reports the unsynchronized pair from vector clocks, so + * it fails on the pre-fix code whether or not the two accesses happen to overlap + * in wall-clock time, and on x86 they usually do not. + * + * Lock order matches production exactly -- devh->status_mutex first, then the + * stand-in for libusb (bus_mutex) -- so the harness cannot invent an inversion + * that the real code does not have. The event thread therefore claims the URB + * under bus_mutex alone and DROPS it before entering the callback. + */ +#include "libuvc/libuvc.h" +#include "libuvc/libuvc_internal.h" + +#include +#include +#include +#include +#include + +void LIBUSB_CALL _uvc_status_callback(struct libusb_transfer *transfer); + +#define ITERATIONS 200 + +#define CHECK(expression) do { \ + if (!(expression)) { \ + fprintf(stderr, "CHECK failed at %s:%d: %s\n", \ + __FILE__, __LINE__, #expression); \ + return EXIT_FAILURE; \ + } \ +} while (0) + +static pthread_mutex_t bus_mutex = PTHREAD_MUTEX_INITIALIZER; + +/* Everything libusb would own, guarded by bus_mutex. */ +static struct { + struct libusb_transfer *xfer; + int urb_in_flight; + int cancel_requested; + int stop; + int submits; + int first_release_at; + int last_submit_at; + int op_count; +} bus; + +static int next_op(void) { + return bus.op_count++; +} + +int __wrap_libusb_submit_transfer(struct libusb_transfer *transfer) { + (void) transfer; + pthread_mutex_lock(&bus_mutex); + bus.last_submit_at = next_op(); + bus.urb_in_flight = 1; + bus.submits++; + pthread_mutex_unlock(&bus_mutex); + return LIBUSB_SUCCESS; +} + +int __wrap_libusb_cancel_transfer(struct libusb_transfer *transfer) { + int in_flight; + + (void) transfer; + pthread_mutex_lock(&bus_mutex); + next_op(); + in_flight = bus.urb_in_flight; + bus.cancel_requested = 1; + pthread_mutex_unlock(&bus_mutex); + + /* Losing the race to the event thread is the interesting case, not an error: + * libusb reports NOT_FOUND once it has reaped the URB, and the callback for it + * may still be pending. */ + return in_flight ? LIBUSB_SUCCESS : LIBUSB_ERROR_NOT_FOUND; +} + +int __wrap_libusb_release_interface(libusb_device_handle *devh, + int interface_number) { + (void) devh; + (void) interface_number; + pthread_mutex_lock(&bus_mutex); + if (bus.first_release_at < 0) + bus.first_release_at = next_op(); + else + next_op(); + pthread_mutex_unlock(&bus_mutex); + return LIBUSB_SUCCESS; +} + +int __wrap_libusb_set_interface_alt_setting(libusb_device_handle *devh, + int interface_number, + int alternate_setting) { + (void) devh; + (void) interface_number; + (void) alternate_setting; + pthread_mutex_lock(&bus_mutex); + next_op(); + pthread_mutex_unlock(&bus_mutex); + return LIBUSB_SUCCESS; +} + +int __wrap_libusb_attach_kernel_driver(libusb_device_handle *devh, + int interface_number) { + (void) devh; + (void) interface_number; + pthread_mutex_lock(&bus_mutex); + next_op(); + pthread_mutex_unlock(&bus_mutex); + return LIBUSB_SUCCESS; +} + +void __wrap_libusb_close(libusb_device_handle *devh) { + (void) devh; + pthread_mutex_lock(&bus_mutex); + next_op(); + pthread_mutex_unlock(&bus_mutex); +} + +void __wrap_libusb_unref_device(libusb_device *dev) { + (void) dev; +} + +/* One real libusb_handle_events() thread: reap whatever URB is in flight and run + * the production callback, which re-arms it until uvc_close() stops it. */ +static void *event_thread_main(void *arg) { + (void) arg; + + for (;;) { + struct libusb_transfer *xfer = NULL; + int cancelled = 0; + int stop; + + pthread_mutex_lock(&bus_mutex); + stop = bus.stop; + if (!stop && bus.urb_in_flight) { + bus.urb_in_flight = 0; + cancelled = bus.cancel_requested; + xfer = bus.xfer; + } + pthread_mutex_unlock(&bus_mutex); + + if (stop) + return NULL; + if (xfer == NULL) { + sched_yield(); + continue; + } + + xfer->status = cancelled ? LIBUSB_TRANSFER_CANCELLED + : LIBUSB_TRANSFER_TIMED_OUT; + _uvc_status_callback(xfer); + + /* A terminal completion is libusb's last word on this transfer; touching + * either the transfer or the handle after it is the use-after-free the close + * path is required to prevent. */ + if (cancelled) + return NULL; + } +} + +/* uvc_close() must start while the event thread is ACTIVELY re-arming, otherwise + * the stop flag is set before the callback ever runs and the two never overlap. + * Waiting for the first resubmission is what puts the close into the middle of + * that loop instead of ahead of it. Bounded so a broken callback cannot hang. */ +static int wait_for_resubmission(void) { + long spins; + + for (spins = 0; spins < 100000000L; spins++) { + int submits; + + pthread_mutex_lock(&bus_mutex); + submits = bus.submits; + pthread_mutex_unlock(&bus_mutex); + + if (submits > 0) + return 1; + sched_yield(); + } + return 0; +} + +static uvc_context_t ctx; +static uvc_device_t dev; + +static int run_one_iteration(int *submits_out) { + uvc_device_handle_t *devh = calloc(1, sizeof(*devh)); + uvc_device_info_t *info = calloc(1, sizeof(*info)); + struct libusb_transfer *xfer = libusb_alloc_transfer(0); + pthread_t event_thread; + int first_release_at, last_submit_at; + + CHECK(devh != NULL && info != NULL && xfer != NULL); + + memset(&ctx, 0, sizeof(ctx)); + memset(&dev, 0, sizeof(dev)); + dev.ctx = &ctx; + dev.ref = 2; + ctx.own_usb_ctx = 0; + + info->ctrl_if.bInterfaceNumber = 0; + info->ctrl_if.bEndpointAddress = 0x81; + + devh->dev = &dev; + devh->info = info; + devh->claimed = 1u << 0; + devh->status_xfer = xfer; + devh->status_xfer_submitted = 1; + pthread_mutex_init(&devh->status_mutex, NULL); + + xfer->user_data = devh; + + memset(&bus, 0, sizeof(bus)); + bus.xfer = xfer; + bus.urb_in_flight = 1; + bus.first_release_at = -1; + bus.last_submit_at = -1; + + DL_APPEND(ctx.open_devices, devh); + + CHECK(pthread_create(&event_thread, NULL, event_thread_main, NULL) == 0); + CHECK(wait_for_resubmission()); + + uvc_close(devh); + + pthread_mutex_lock(&bus_mutex); + bus.stop = 1; + pthread_mutex_unlock(&bus_mutex); + CHECK(pthread_join(event_thread, NULL) == 0); + + pthread_mutex_lock(&bus_mutex); + *submits_out = bus.submits; + first_release_at = bus.first_release_at; + last_submit_at = bus.last_submit_at; + pthread_mutex_unlock(&bus_mutex); + + CHECK(ctx.has_quarantined_device == 0); + CHECK(ctx.open_devices == NULL); + CHECK(first_release_at >= 0); + CHECK(last_submit_at < first_release_at); + + return EXIT_SUCCESS; +} + +/* The event thread re-arming while uvc_close() tears the handle down. */ +static int check_close_races_status_callback(void) { + int iteration, total_submits = 0; + + for (iteration = 0; iteration < ITERATIONS; iteration++) { + int submits = 0; + if (run_one_iteration(&submits) != EXIT_SUCCESS) { + fprintf(stderr, " failed on iteration %d of %d\n", iteration, ITERATIONS); + return EXIT_FAILURE; + } + total_submits += submits; + } + + /* Without at least one resubmission the run proved nothing: it would mean the + * event thread never got to race the close at all. */ + CHECK(total_submits > 0); + return EXIT_SUCCESS; +} + +int main(int argc, char **argv) { + CHECK(argc == 3 && strcmp(argv[1], "--case") == 0); + if (strcmp(argv[2], "close_races_status_callback") == 0) + return check_close_races_status_callback(); + fprintf(stderr, "unknown case: %s\n", argv[2]); + return EXIT_FAILURE; +} diff --git a/tests/teardown_assertions.c b/tests/teardown_assertions.c index d2b0eac..a1d2792 100644 --- a/tests/teardown_assertions.c +++ b/tests/teardown_assertions.c @@ -38,6 +38,7 @@ void LIBUSB_CALL _uvc_status_callback(struct libusb_transfer *transfer); typedef enum { OP_SUBMIT, OP_CANCEL, + OP_CALLBACK, OP_SETALT, OP_RELEASE, OP_ATTACH, @@ -56,15 +57,17 @@ static int op_count; static int urb_in_flight; static int cancel_requested; static int deliver_callbacks; +static int cancel_returns_not_found; static const char *op_name(op_kind kind) { switch (kind) { - case OP_SUBMIT: return "submit"; - case OP_CANCEL: return "cancel"; - case OP_SETALT: return "set_alt"; - case OP_RELEASE: return "release_if"; - case OP_ATTACH: return "attach_driver"; - case OP_CLOSE: return "close"; + case OP_SUBMIT: return "submit"; + case OP_CANCEL: return "cancel"; + case OP_CALLBACK: return "status_callback"; + case OP_SETALT: return "set_alt"; + case OP_RELEASE: return "release_if"; + case OP_ATTACH: return "attach_driver"; + case OP_CLOSE: return "close"; } return "?"; } @@ -92,6 +95,14 @@ static int index_of_first(op_kind kind, int iface) { return -1; } +static int index_of_first_any(op_kind kind) { + int i; + for (i = 0; i < op_count && i < MAX_OPS; i++) + if (ops[i].kind == kind) + return i; + return -1; +} + static int index_of_last(op_kind kind) { int i, found = -1; for (i = 0; i < op_count && i < MAX_OPS; i++) @@ -118,6 +129,7 @@ static void pump_event_thread(void) { urb_in_flight = 0; status_xfer->status = cancel_requested ? LIBUSB_TRANSFER_CANCELLED : LIBUSB_TRANSFER_TIMED_OUT; + record(OP_CALLBACK, -1); _uvc_status_callback(status_xfer); } @@ -134,6 +146,11 @@ int __wrap_libusb_cancel_transfer(struct libusb_transfer *transfer) { if (!urb_in_flight) return LIBUSB_ERROR_NOT_FOUND; cancel_requested = 1; + /* "already cancelled" is one of the three things libusb reports as NOT_FOUND, + * and it is the one where the completion callback has NOT run yet. The URB + * therefore stays in flight here: the cancellation is pending, not finished. */ + if (cancel_returns_not_found) + return LIBUSB_ERROR_NOT_FOUND; return LIBUSB_SUCCESS; } @@ -183,9 +200,11 @@ static uvc_context_t ctx; static uvc_device_t dev; /* Build the handle uvc_open_internal() would have produced: `claimed` carries the - * interfaces libuvc holds, and a non-zero control endpoint means the status - * interrupt transfer was submitted and is self-re-arming. */ -static uvc_device_handle_t *make_handle(uint32_t claimed, uint8_t ctrl_endpoint) { + * interfaces libuvc holds, `ctrl_iface` is the VideoControl interface number, and + * a non-zero control endpoint means the status interrupt transfer was submitted + * and is self-re-arming. */ +static uvc_device_handle_t *make_handle_on(uint32_t claimed, uint8_t ctrl_endpoint, + uint8_t ctrl_iface) { uvc_device_handle_t *devh = calloc(1, sizeof(*devh)); uvc_device_info_t *info = calloc(1, sizeof(*info)); @@ -204,7 +223,7 @@ static uvc_device_handle_t *make_handle(uint32_t claimed, uint8_t ctrl_endpoint) * this handle instead of killing and joining an event-handler thread. */ ctx.own_usb_ctx = 0; - info->ctrl_if.bInterfaceNumber = 0; + info->ctrl_if.bInterfaceNumber = ctrl_iface; info->ctrl_if.bEndpointAddress = ctrl_endpoint; devh->dev = &dev; @@ -215,6 +234,7 @@ static uvc_device_handle_t *make_handle(uint32_t claimed, uint8_t ctrl_endpoint) urb_in_flight = 0; cancel_requested = 0; deliver_callbacks = 1; + cancel_returns_not_found = 0; status_xfer = NULL; if (ctrl_endpoint) { @@ -234,6 +254,10 @@ static uvc_device_handle_t *make_handle(uint32_t claimed, uint8_t ctrl_endpoint) return devh; } +static uvc_device_handle_t *make_handle(uint32_t claimed, uint8_t ctrl_endpoint) { + return make_handle_on(claimed, ctrl_endpoint, 0); +} + /* A camera whose VideoControl interface carries a status interrupt endpoint. The * URB must stop before the interface it rides on is released: a submission that * lands after libusb_attach_kernel_driver() makes usbfs re-claim the interface, @@ -297,6 +321,86 @@ static int check_handle_without_status_endpoint_is_unchanged(void) { return EXIT_SUCCESS; } +/* Both teardown invariants over an ARBITRARY interface layout. The two cases + * above pin the reproduction device's shape -- VideoControl at interface 0 with + * at most one VideoStreaming interface next to it -- which a release loop that + * simply special-cased index 0 would also satisfy. A UVC function may sit + * anywhere in a configuration's interface space and expose several + * VideoStreaming interfaces, so the order has to come from devh->claimed and + * info->ctrl_if.bInterfaceNumber and nothing else. */ +static int check_generic_layout(int ctrl_iface, const int *stream_ifaces, + int stream_count, uint8_t ctrl_endpoint) { + uint32_t claimed = 1u << ctrl_iface; + uvc_device_handle_t *devh; + int release_ctrl, i; + + for (i = 0; i < stream_count; i++) + claimed |= 1u << stream_ifaces[i]; + + devh = make_handle_on(claimed, ctrl_endpoint, (uint8_t) ctrl_iface); + CHECK(devh != NULL); + uvc_close(devh); + + release_ctrl = index_of_first(OP_RELEASE, ctrl_iface); + CHECK(release_ctrl >= 0); + CHECK(count_of(OP_RELEASE) == stream_count + 1); + + for (i = 0; i < stream_count; i++) { + int release_stream = index_of_first(OP_RELEASE, stream_ifaces[i]); + CHECK(release_stream >= 0); + CHECK(release_stream < release_ctrl); + CHECK(index_of_first(OP_ATTACH, stream_ifaces[i]) > release_stream); + } + + CHECK(index_of_first(OP_ATTACH, ctrl_iface) > release_ctrl); + CHECK(index_of_last(OP_SUBMIT) < index_of_first_any(OP_RELEASE)); + CHECK(index_of_last(OP_CLOSE) == op_count - 1); + return EXIT_SUCCESS; +} + +/* VideoControl at a nonzero index, three VideoStreaming interfaces around it, + * and a status interrupt endpoint -- so the status stop and the release order + * are both exercised on a layout nothing in the fix can have been tuned to. */ +static int check_sparse_interfaces_control_released_last(void) { + static const int stream_ifaces[] = { 1, 5, 7 }; + return check_generic_layout(3, stream_ifaces, 3, 0x83); +} + +/* The same contract far away from the low bits the reproduction used, which is + * where a release scan that stopped early or assumed contiguity would show up. + * No status endpoint here, so this isolates the interface walk itself. */ +static int check_high_index_interfaces_released(void) { + static const int stream_ifaces[] = { 9, 17, 24 }; + return check_generic_layout(2, stream_ifaces, 3, 0); +} + +/* A cancel that reports LIBUSB_ERROR_NOT_FOUND must still be 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: + * the close would go on to release the interface, free the transfer libusb is + * still about to complete (undefined behaviour by libusb's own contract) and + * free the devh that the pending callback dereferences. So the callback has to + * land BEFORE the first USB operation of the teardown, not somewhere in the + * middle of it. */ +static int check_cancel_not_found_still_drains(void) { + uvc_device_handle_t *devh = make_handle(1u << 0, 0x81); + int callback; + + CHECK(devh != NULL); + cancel_returns_not_found = 1; + uvc_close(devh); + + CHECK(count_of(OP_CANCEL) == 1); + callback = index_of_first_any(OP_CALLBACK); + CHECK(callback >= 0); + CHECK(callback < index_of_first_any(OP_SETALT)); + CHECK(callback < index_of_first(OP_RELEASE, 0)); + CHECK(count_of(OP_SUBMIT) == 0); + CHECK(count_of(OP_RELEASE) == 1); + CHECK(index_of_last(OP_CLOSE) == op_count - 1); + return EXIT_SUCCESS; +} + /* A wedged event thread must not hang the close, and must not let it free a * handle libusb still references through the transfer's user_data. */ static int check_undeliverable_status_xfer_quarantines_the_handle(void) { @@ -323,6 +427,12 @@ int main(int argc, char **argv) { return check_every_claimed_interface_is_released_control_last(); if (strcmp(argv[2], "no_status_endpoint_unchanged") == 0) return check_handle_without_status_endpoint_is_unchanged(); + if (strcmp(argv[2], "sparse_interfaces_control_released_last") == 0) + return check_sparse_interfaces_control_released_last(); + if (strcmp(argv[2], "high_index_interfaces_released") == 0) + return check_high_index_interfaces_released(); + if (strcmp(argv[2], "cancel_not_found_still_drains") == 0) + return check_cancel_not_found_still_drains(); if (strcmp(argv[2], "undeliverable_status_xfer_quarantines") == 0) return check_undeliverable_status_xfer_quarantines_the_handle(); fprintf(stderr, "unknown case: %s\n", argv[2]);