diff --git a/docs/windows-driver.md b/docs/windows-driver.md index 98ce3a05..ac714753 100644 --- a/docs/windows-driver.md +++ b/docs/windows-driver.md @@ -58,6 +58,15 @@ normal desktop application to use the broker without running as administrator while keeping broker ownership and privileged device operations in the Windows service. +A client that finds the pipe missing waits up to five seconds for it, in case +the broker is still starting. It first asks the service manager whether the +service can still answer: if the `libvirtualhid_broker` service is not +installed, or is stopped or stopping, the request fails at once with +`ERROR_SERVICE_DOES_NOT_EXIST` or `ERROR_SERVICE_NOT_ACTIVE` instead of +spending the wait. The client never starts the service itself, and the service +has no trigger start, so nothing would have appeared. Any other service state, +or an unreadable service manager, keeps the wait. + Status, current-license validation, activation, replacement, deactivation, virtual HID device creation, and owned-device destruction are available to authenticated local users without elevation. Before sending any request, clients compare the diff --git a/src/platform/windows/windows_broker_client.cpp b/src/platform/windows/windows_broker_client.cpp index d05eb110..a3d8f891 100644 --- a/src/platform/windows/windows_broker_client.cpp +++ b/src/platform/windows/windows_broker_client.cpp @@ -82,6 +82,50 @@ namespace lvh::detail::windows_broker { return last_error == ERROR_SEM_TIMEOUT || last_error == ERROR_FILE_NOT_FOUND; } + static UniqueServiceHandle make_unique_service_handle(SC_HANDLE handle) { + return {handle, &::CloseServiceHandle}; + } + + /** + * @brief Whether waiting on the broker pipe has any chance. + * + * The retry loop exists for a broker that's still starting. A missing, + * stopped, or stopping service never gets started by us (no trigger start + * either), so waiting out the timeout just stalls the caller. + * + * @param[out] error Set to the reason when this returns false. + * @return False only when the service is missing, stopped, or stopping. + */ + static bool broker_service_may_answer(DWORD &error) { + auto service_manager = make_unique_service_handle( + ::OpenSCManagerW(nullptr, nullptr, SC_MANAGER_CONNECT) + ); + if (!service_manager) { + return true; + } + + auto service = make_unique_service_handle( + ::OpenServiceW(service_manager.get(), broker_service_name, SERVICE_QUERY_STATUS) + ); + if (!service) { + if (::GetLastError() != ERROR_SERVICE_DOES_NOT_EXIST) { + return true; + } + error = ERROR_SERVICE_DOES_NOT_EXIST; + return false; + } + + SERVICE_STATUS_PROCESS service_status {}; + if (DWORD bytes_needed = 0; ::QueryServiceStatusEx(service.get(), SC_STATUS_PROCESS_INFO, std::bit_cast(std::as_writable_bytes(std::span {&service_status, 1}).data()), sizeof(service_status), &bytes_needed) == FALSE) { + return true; + } + if (service_status.dwCurrentState != SERVICE_STOPPED && service_status.dwCurrentState != SERVICE_STOP_PENDING) { + return true; + } + error = ERROR_SERVICE_NOT_ACTIVE; + return false; + } + static UniqueHandle connect_to_broker_pipe() { DWORD last_error = ERROR_FILE_NOT_FOUND; for (auto attempt = 0U; attempt < pipe_wait_timeout / pipe_retry_interval; ++attempt) { @@ -90,6 +134,9 @@ namespace lvh::detail::windows_broker { } last_error = ::GetLastError(); + if (last_error == ERROR_FILE_NOT_FOUND && attempt == 0U && !broker_service_may_answer(last_error)) { + break; + } if (!wait_to_retry_broker_pipe(last_error)) { ::SetLastError(last_error); return make_unique_handle(INVALID_HANDLE_VALUE); @@ -100,10 +147,6 @@ namespace lvh::detail::windows_broker { return make_unique_handle(INVALID_HANDLE_VALUE); } - static UniqueServiceHandle make_unique_service_handle(SC_HANDLE handle) { - return {handle, &::CloseServiceHandle}; - } - static std::string windows_error_message(DWORD error_code) { std::array message_buffer {}; const auto message_size = ::FormatMessageA( diff --git a/tests/fixtures/include/fixtures/windows_broker_client_test_hooks.hpp b/tests/fixtures/include/fixtures/windows_broker_client_test_hooks.hpp index f5197f3e..05416a3c 100644 --- a/tests/fixtures/include/fixtures/windows_broker_client_test_hooks.hpp +++ b/tests/fixtures/include/fixtures/windows_broker_client_test_hooks.hpp @@ -15,6 +15,11 @@ namespace lvh::detail::test { enum class BrokerServiceScenario { pipe_unavailable_once, pipe_never_available, + pipe_service_missing, + pipe_service_stopped, + pipe_service_stop_pending, + pipe_service_query_failure, + pipe_service_manager_unavailable, pipe_access_denied, pipe_busy_once, pipe_busy_timeout_once, @@ -37,6 +42,8 @@ namespace lvh::detail::test { std::uint32_t create_attempts = 0; std::uint32_t sleep_attempts = 0; std::uint32_t wait_attempts = 0; + /// The Win32 error the client left for its caller (fake SetLastError). + std::uint32_t last_error = 0; bool transacted = false; }; diff --git a/tests/fixtures/windows_broker_client_test_hooks.cpp b/tests/fixtures/windows_broker_client_test_hooks.cpp index 8ce22d24..78846e72 100644 --- a/tests/fixtures/windows_broker_client_test_hooks.cpp +++ b/tests/fixtures/windows_broker_client_test_hooks.cpp @@ -69,6 +69,10 @@ namespace { return fake_state().last_error; } + void WINAPI fake_set_last_error(DWORD error) { + fake_state().last_error = error; + } + HANDLE WINAPI fake_create_file_a( LPCSTR, DWORD, @@ -82,7 +86,7 @@ namespace { ++fake_state().create_attempts; const auto scenario = fake_state().scenario; - if ((scenario == pipe_unavailable_once && fake_state().create_attempts == 1U) || scenario == pipe_never_available) { + if ((scenario == pipe_unavailable_once && fake_state().create_attempts == 1U) || scenario == pipe_never_available || scenario == pipe_service_missing || scenario == pipe_service_stopped || scenario == pipe_service_stop_pending || scenario == pipe_service_query_failure || scenario == pipe_service_manager_unavailable) { fake_state().last_error = ERROR_FILE_NOT_FOUND; return INVALID_HANDLE_VALUE; } @@ -134,14 +138,22 @@ namespace { } SC_HANDLE WINAPI fake_open_service_manager(LPCWSTR, LPCWSTR, DWORD) { - if (fake_state().scenario == lvh::detail::test::BrokerServiceScenario::service_manager_failure) { + using enum lvh::detail::test::BrokerServiceScenario; + + if (fake_state().scenario == service_manager_failure || fake_state().scenario == pipe_service_manager_unavailable) { return nullptr; } return fake_service_manager_handle(); } SC_HANDLE WINAPI fake_open_service(SC_HANDLE, LPCWSTR, DWORD) { - if (fake_state().scenario == lvh::detail::test::BrokerServiceScenario::service_failure) { + using enum lvh::detail::test::BrokerServiceScenario; + + if (fake_state().scenario == pipe_service_missing) { + fake_state().last_error = ERROR_SERVICE_DOES_NOT_EXIST; + return nullptr; + } + if (fake_state().scenario == service_failure) { return nullptr; } return fake_service_handle(); @@ -156,12 +168,18 @@ namespace { ) { using enum lvh::detail::test::BrokerServiceScenario; - if (fake_state().scenario == service_query_failure) { + if (fake_state().scenario == service_query_failure || fake_state().scenario == pipe_service_query_failure) { return FALSE; } SERVICE_STATUS_PROCESS status {}; - status.dwCurrentState = fake_state().scenario == service_stopped ? SERVICE_STOPPED : SERVICE_RUNNING; + if (fake_state().scenario == service_stopped || fake_state().scenario == pipe_service_stopped) { + status.dwCurrentState = SERVICE_STOPPED; + } else if (fake_state().scenario == pipe_service_stop_pending) { + status.dwCurrentState = SERVICE_STOP_PENDING; + } else { + status.dwCurrentState = SERVICE_RUNNING; + } status.dwProcessId = fake_state().scenario == service_process_mismatch ? broker_process_id + 1UL : broker_process_id; *bytes_needed = sizeof(status); std::memcpy(buffer, &status, sizeof(status)); @@ -197,6 +215,7 @@ namespace { #define OpenSCManagerW fake_open_service_manager #define OpenServiceW fake_open_service #define QueryServiceStatusEx fake_query_service_status +#define SetLastError fake_set_last_error #define SetNamedPipeHandleState fake_set_named_pipe_handle_state #define Sleep fake_sleep #define TransactNamedPipe fake_transact_named_pipe @@ -213,6 +232,7 @@ namespace { #undef OpenSCManagerW #undef OpenServiceW #undef QueryServiceStatusEx +#undef SetLastError #undef SetNamedPipeHandleState #undef Sleep #undef TransactNamedPipe @@ -245,6 +265,7 @@ namespace lvh::detail::test { .create_attempts = fake_state().create_attempts, .sleep_attempts = fake_state().sleep_attempts, .wait_attempts = fake_state().wait_attempts, + .last_error = fake_state().last_error, .transacted = fake_state().transacted, }; } diff --git a/tests/unit/test_windows_broker_client.cpp b/tests/unit/test_windows_broker_client.cpp index e39372e2..3a8b5286 100644 --- a/tests/unit/test_windows_broker_client.cpp +++ b/tests/unit/test_windows_broker_client.cpp @@ -12,6 +12,16 @@ // standard includes #include +#ifndef NOMINMAX + #define NOMINMAX +#endif +#ifndef WIN32_LEAN_AND_MEAN + #define WIN32_LEAN_AND_MEAN +#endif + +// platform includes +#include + namespace { struct FailureCase { @@ -26,6 +36,8 @@ namespace { std::uint32_t create_attempts; std::uint32_t sleep_attempts; std::uint32_t wait_attempts; + std::uint32_t closed_service_handles; + std::uint32_t last_error; }; } // namespace @@ -68,12 +80,14 @@ TEST(WindowsBrokerClientTest, TransactsOnlyWithRunningInstalledBrokerService) { TEST(WindowsBrokerClientTest, RetriesWhileTheBrokerPipeIsBeingRecreated) { using enum lvh::detail::test::BrokerServiceScenario; - for (const auto &[scenario, name, create_attempts, sleep_attempts, wait_attempts] : { - PipeCase {pipe_unavailable_once, "missing once", 2U, 1U, 0U}, - PipeCase {pipe_busy_once, "busy then available", 2U, 0U, 1U}, - PipeCase {pipe_busy_timeout_once, "busy wait timed out", 2U, 0U, 1U}, - PipeCase {pipe_busy_disappears_once, "busy pipe disappeared", 2U, 0U, 1U}, + // A missing pipe checks the service before waiting: two more handles closed. + for (const auto &[scenario, name, create_attempts, sleep_attempts, wait_attempts, closed_service_handles, last_error] : { + PipeCase {pipe_unavailable_once, "missing once", 2U, 1U, 0U, 4U, 0U}, + PipeCase {pipe_busy_once, "busy then available", 2U, 0U, 1U, 2U, 0U}, + PipeCase {pipe_busy_timeout_once, "busy wait timed out", 2U, 0U, 1U, 2U, 0U}, + PipeCase {pipe_busy_disappears_once, "busy pipe disappeared", 2U, 0U, 1U, 2U, 0U}, }) { + static_cast(last_error); SCOPED_TRACE(name); const auto result = lvh::detail::test::verify_broker_service_scenario(scenario); @@ -83,7 +97,7 @@ TEST(WindowsBrokerClientTest, RetriesWhileTheBrokerPipeIsBeingRecreated) { EXPECT_EQ(result.sleep_attempts, sleep_attempts); EXPECT_EQ(result.wait_attempts, wait_attempts); EXPECT_EQ(result.closed_pipe_handles, 1U); - EXPECT_EQ(result.closed_service_handles, 2U); + EXPECT_EQ(result.closed_service_handles, closed_service_handles); EXPECT_TRUE(result.transacted); } } @@ -91,10 +105,17 @@ TEST(WindowsBrokerClientTest, RetriesWhileTheBrokerPipeIsBeingRecreated) { TEST(WindowsBrokerClientTest, ReportsBrokerPipeConnectionFailures) { using enum lvh::detail::test::BrokerServiceScenario; - for (const auto &[scenario, name, create_attempts, sleep_attempts, wait_attempts] : { - PipeCase {pipe_access_denied, "access denied", 1U, 0U, 0U}, - PipeCase {pipe_busy_failure, "busy wait failed", 1U, 0U, 1U}, - PipeCase {pipe_never_available, "retry deadline exhausted", 500U, 500U, 0U}, + // Only wait when the service could still show up. Missing or stopped fails + // on the first attempt with no sleep; unknown status keeps the full wait. + for (const auto &[scenario, name, create_attempts, sleep_attempts, wait_attempts, closed_service_handles, last_error] : { + PipeCase {pipe_access_denied, "access denied", 1U, 0U, 0U, 0U, ERROR_ACCESS_DENIED}, + PipeCase {pipe_busy_failure, "busy wait failed", 1U, 0U, 1U, 0U, ERROR_ACCESS_DENIED}, + PipeCase {pipe_never_available, "retry deadline exhausted", 500U, 500U, 0U, 2U, ERROR_FILE_NOT_FOUND}, + PipeCase {pipe_service_missing, "broker service not installed", 1U, 0U, 0U, 1U, ERROR_SERVICE_DOES_NOT_EXIST}, + PipeCase {pipe_service_stopped, "broker service stopped", 1U, 0U, 0U, 2U, ERROR_SERVICE_NOT_ACTIVE}, + PipeCase {pipe_service_stop_pending, "broker service stopping", 1U, 0U, 0U, 2U, ERROR_SERVICE_NOT_ACTIVE}, + PipeCase {pipe_service_query_failure, "service status unknown", 500U, 500U, 0U, 2U, ERROR_FILE_NOT_FOUND}, + PipeCase {pipe_service_manager_unavailable, "service manager unavailable", 500U, 500U, 0U, 0U, ERROR_FILE_NOT_FOUND}, }) { SCOPED_TRACE(name); const auto result = @@ -103,11 +124,12 @@ TEST(WindowsBrokerClientTest, ReportsBrokerPipeConnectionFailures) { EXPECT_FALSE(result.status.ok()); EXPECT_EQ(result.status.code(), lvh::ErrorCode::backend_unavailable); EXPECT_FALSE(result.status.message().empty()); + EXPECT_EQ(result.last_error, last_error); EXPECT_EQ(result.create_attempts, create_attempts); EXPECT_EQ(result.sleep_attempts, sleep_attempts); EXPECT_EQ(result.wait_attempts, wait_attempts); EXPECT_EQ(result.closed_pipe_handles, 0U); - EXPECT_EQ(result.closed_service_handles, 0U); + EXPECT_EQ(result.closed_service_handles, closed_service_handles); EXPECT_FALSE(result.transacted); } }