fix(windows): prevent crashes from buggy camera drivers during device enumeration #27
Conversation
…rs (#26)\n\nUse SEH to protect moniker->BindToObject during enumeration and device binding, so buggy drivers (e.g., unplugged Oculus Quest 3) don't crash the process. Log and skip problematic devices.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds SEH-protected DirectShow device binding to prevent crashes during enumeration/open, a new interactive device hot‑plug test executable, corresponding CMake target and messages, and a VSCode task to run the test. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/ccap_imp_windows.cpp (1)
129-144: Consider nullifying output parameter on failure/exception.When
BindToObjectfails or an exception occurs,*deviceFiltermay contain an indeterminate value (ifSUCCEEDED(hr)is false, COM may or may not have written to the output parameter). For safety, consider explicitly setting*deviceFilter = nullptrin the failure and exception paths.DeviceBindResult tryBindMonikerForOpen(IMoniker* moniker, IBaseFilter** deviceFilter) { #if CCAP_SEH_SUPPORTED __try { #endif HRESULT hr = moniker->BindToObject(0, 0, IID_IBaseFilter, (void**)deviceFilter); if (SUCCEEDED(hr)) { return DeviceBindResult::Success; } else { + *deviceFilter = nullptr; return DeviceBindResult::Failed; } #if CCAP_SEH_SUPPORTED } __except (EXCEPTION_EXECUTE_HANDLER) { + *deviceFilter = nullptr; return DeviceBindResult::Exception; } #endif }tests/test_device_hotplug.cpp (2)
33-38: Unused platform-specific headers.
conio.h(Windows) andtermios.h/unistd.h(non-Windows) are included but appear unused. ThewaitForUserInput()function usesstd::getline()instead. Consider removing these includes if they're not needed, or add a comment if they're reserved for future use.#ifdef _WIN32 -#include <conio.h> // For _kbhit() and _getch() -#else -#include <termios.h> -#include <unistd.h> +// Platform-specific headers reserved for future non-blocking input features #endif
264-272: Test always opens first device regardless of user action.The prompt asks users to note "one of the devices" and unplug it, but the test always attempts to open
devices[0]. If the user unplugs a different device, the test may not exercise the intended failure path. Consider either:
- Clarifying the instructions to specifically unplug the first device, or
- Adding a prompt for users to select which device index to test
- std::cout << "\n" << COLOR_YELLOW << "ACTION REQUIRED:\n" - << " 1. Note one of the devices listed above\n" - << " 2. Physically unplug that device\n" + std::cout << "\n" << COLOR_YELLOW << "ACTION REQUIRED:\n" + << " 1. Physically unplug the FIRST device [0] listed above\n" + << " (\"" << devices[0] << "\")\n" << " 3. Press Enter when ready\n" << COLOR_RESET;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.vscode/tasks.json(1 hunks)src/ccap_imp_windows.cpp(3 hunks)tests/CMakeLists.txt(1 hunks)tests/test_device_hotplug.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/ccap_imp_windows.cpp (1)
src/ccap_imp.cpp (2)
reportError(193-199)reportError(193-193)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Agent
- GitHub Check: macOS Build (Release-static)
- GitHub Check: Windows Build (Release-shared)
- GitHub Check: Windows Build (Release-static)
- GitHub Check: Windows Build (Debug-shared)
- GitHub Check: Windows Build (Debug-static)
🔇 Additional comments (9)
.vscode/tasks.json (1)
902-934: LGTM!The new VSCode task follows the established patterns in this file. The task configuration properly:
- Handles both Windows and Unix-like environments
- Sets appropriate working directory for the test executable
- Uses a dedicated panel with focus for the interactive test
- Depends on "Build Project (Debug)" to ensure the executable is built first
src/ccap_imp_windows.cpp (4)
94-104: LGTM!The SEH detection macro correctly identifies MSVC and MinGW-w64 with SEH support. The
DeviceBindResultenum provides a clean, type-safe way to distinguish between successful binding, normal failures, and SEH-caught exceptions.
107-126: LGTM!The helper function correctly wraps the critical
BindToObjectcall with SEH protection. The filter is properly released on success, and logging uses appropriate severity levels (info for bind failures, warning for exceptions). When SEH is not supported, the code gracefully degrades to unprotected calls.
455-461: LGTM!The SEH-protected helper is correctly integrated. Only devices that successfully bind are added to the list, and unavailable devices (both failed bindings and exception-causing devices) are silently skipped, which is the expected behavior for enumeration.
712-736: LGTM!The error handling logic correctly differentiates between:
- Success: Device opens and enumeration stops
- Specific device requested but failed: Reports appropriate error and stops
- No specific device and failure: Logs and continues to try next device
The exception case properly distinguishes between SEH-caught crashes (with specific messaging) and normal bind failures.
tests/test_device_hotplug.cpp (3)
40-100: LGTM!The helper functions provide clean, colored console output that improves test readability. The countdown timer and device list printing are well-implemented for an interactive test.
111-230: LGTM!The test function is well-structured with clear steps, appropriate exception handling, and helpful result analysis. Creating a new
Providerinstance for re-enumeration ensures fresh device discovery, which is crucial for testing the hot-plug scenario.
292-348: LGTM!The main function provides a user-friendly interactive experience with clear visual indicators, appropriate platform warnings, and proper exception handling. The 5-second countdown gives users time to prepare before the test begins.
tests/CMakeLists.txt (1)
217-230: LGTM!The executable configuration is appropriate for an interactive test - it links only against
ccap(not gtest) and correctly sets C++17 as required. The test is intentionally excluded from the automated test suite viagtest_discover_tests.
There was a problem hiding this comment.
Pull request overview
This PR implements crash prevention for Windows camera device enumeration when dealing with buggy DirectShow drivers, particularly addressing issues with devices like Oculus Quest 3 VR headsets that can cause application crashes when unplugged during device access operations.
Key Changes:
- Added Windows Structured Exception Handling (SEH) protection around DirectShow API calls that can throw unhandled exceptions from faulty drivers
- Introduced
DeviceBindResultenum for type-safe error handling with three states: Success, Failed, and Exception - Created an interactive test program to verify crash prevention during device hot-plug scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/ccap_imp_windows.cpp |
Implements SEH protection with helper functions tryBindMonikerToFilter() and tryBindMonikerForOpen() to safely bind device monikers; adds CCAP_SEH_SUPPORTED macro for compiler compatibility detection |
tests/test_device_hotplug.cpp |
New interactive test program with ANSI colored output to manually verify crash prevention during device enumeration and opening after hot-unplug events |
tests/CMakeLists.txt |
Adds build configuration for the new interactive test executable and custom target for manual execution |
.vscode/tasks.json |
Adds VS Code task to run the device hot-plug test with dedicated panel presentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix typo: run_device_hotplug_testg_test -> run_device_hotplug_test - Remove unused platform-specific headers (conio.h, termios.h, unistd.h) - Add <algorithm> header to fix Linux compilation error with std::find
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_device_hotplug.cpp (2)
251-266: Test always opensdevices[0]regardless of user's choice.The instructions tell the user to note and unplug "one of the devices," but the test always attempts to open
devices[0]. If the user unplugs a different device, the test may pass incorrectly.Consider prompting the user to enter the device index they unplugged:
std::cout << "\n" << COLOR_YELLOW << "ACTION REQUIRED:\n" << " 1. Note one of the devices listed above\n" << " 2. Physically unplug that device\n" - << " 3. Press Enter when ready\n" << COLOR_RESET; + << " 3. Enter the device index you unplugged (or press Enter for 0): " << COLOR_RESET; - waitForUserInput(); + std::string input; + std::getline(std::cin, input); + size_t deviceIndex = 0; + if (!input.empty()) { + try { + deviceIndex = std::stoul(input); + if (deviceIndex >= devices.size()) { + printWarning("Invalid index, using device 0."); + deviceIndex = 0; + } + } catch (...) { + printWarning("Invalid input, using device 0."); + } + } printInfo("Attempting to open the first device from the previous enumeration..."); + std::cout << " Target device: [" << deviceIndex << "] " << devices[deviceIndex] << "\n"; ccap::Provider testProvider; bool openResult = false; try { // Try to open the first device from our enumerated list // If it was unplugged, this should fail gracefully, not crash - openResult = testProvider.open(devices[0], false); + openResult = testProvider.open(devices[deviceIndex], false);
286-286: Unusedargcandargvparameters may trigger compiler warnings.If these parameters are intentionally unused, consider suppressing the warning:
-int main(int argc, char** argv) { +int main([[maybe_unused]] int argc, [[maybe_unused]] char** argv) {Or simply:
-int main(int argc, char** argv) { +int main() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/CMakeLists.txt(1 hunks)tests/test_device_hotplug.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Build (Release-clang-shared)
- GitHub Check: Build (Release-clang-static)
- GitHub Check: Build Fedora (Release-static)
- GitHub Check: Build (Debug-clang-shared)
- GitHub Check: Build (Debug-gcc-static)
- GitHub Check: Build Fedora (Debug-shared)
- GitHub Check: Build (Release-gcc-static)
- GitHub Check: Windows Build (Debug-shared)
- GitHub Check: Windows Build (Release-shared)
- GitHub Check: Windows Build (Release-static)
🔇 Additional comments (4)
tests/test_device_hotplug.cpp (4)
34-95: LGTM!The helper functions are well-organized with consistent naming, appropriate use of
constexprfor color codes, and correct use ofsize_tfor vector indexing. The countdown timer usingstd::this_thread::sleep_foris platform-independent.
105-224: LGTM!The test function is well-structured with clear step-by-step flow, comprehensive exception handling, and informative user prompts. Creating a new
Providerinstance for re-enumeration ensures fresh device discovery.
302-307: LGTM!Good addition of platform-specific guidance. The
#ifndef _WIN32check appropriately warns non-Windows users while still allowing them to run the test for basic functionality verification.
313-329: LGTM!The test orchestration is well-structured with proper exception handling, clear flow between test scenarios, and appropriate return codes for success/failure cases.
Summary
This PR addresses crashes that occur on Windows when enumerating camera devices with buggy drivers, particularly when devices like Oculus Quest 3 VR headsets are unplugged while the system attempts to access them.
Problem
Some camera devices with faulty DirectShow drivers cause the application to crash when:
The crash occurs in
IMoniker::BindToObject()calls, which can throw unhandled exceptions from poorly implemented drivers.Solution
Core Changes
__try/__except)tryBindMonikerToFilter(): Safe device enumerationtryBindMonikerForOpen(): Safe device openingDeviceBindResultenum for type-safe error handling (Success/Failed/Exception)CCAP_SEH_SUPPORTEDmacro to detect MSVC and MinGW-w64 SEH supportCode Quality
Testing
tests/test_device_hotplug.cppImpact
Technical Details
Files Changed:
src/ccap_imp_windows.cpp: Core SEH implementationtests/test_device_hotplug.cpp: New interactive test programtests/CMakeLists.txt: Test build configuration.vscode/tasks.json: Added test execution taskCompiler Compatibility:
__SEH__macroTesting Instructions
Build the test program:
Run via VS Code task: "Run Device Hot-plug Test (Debug)"
Follow interactive prompts to test device enumeration and hot-plug scenarios
Related Issue
Fixes #26
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.