[PATCH 0/3] MR2536: services: Change running service state once its process dies.
Fixes Epic Online Services update hanging when the service needs to be restarted (a helper process is waiting for either service's mutex to be available or QueryServiceStatus to tell that the server is stopped after the service had just exited the process without notifying service control manager). It seems to me that using a job object is much simpler than the other ways (like waiting on multiple process handles and managing process addition and deletion in the waiters list). I tested that on real up to date Windows 10 machine and there the process has the job object which object has JOB_OBJECT_LIMIT_BREAKAWAY_OK | JOB_OBJECT_LIMIT_SILENT_BREAKAWAY_OK limit flags (and only those). That job has also the other service processes in it but not services.exe itself. However, for some reason on any Testbot machine that is not the case and IsProcessInJob() reports that the process doesn't have a job. But since I do see that job on a real machine I think using job object here is fine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2536
From: Paul Gofman <pgofman(a)codeweavers.com> --- programs/services/services.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/programs/services/services.c b/programs/services/services.c index 8a3ff754329..9efedab43d2 100644 --- a/programs/services/services.c +++ b/programs/services/services.c @@ -47,6 +47,7 @@ static DWORD default_preshutdown_timeout = 180000; static DWORD autostart_delay = 120000; static void *environment = NULL; static HKEY service_current_key = NULL; +static HANDLE job_object; static const BOOL is_win64 = (sizeof(void *) > sizeof(int)); @@ -1102,6 +1103,8 @@ found: release_process(process); return err; } + if (!AssignProcessToJobObject(job_object, pi.hProcess)) + WINE_ERR("Could not add object to job.\n"); process->process_id = pi.dwProcessId; process->process = pi.hProcess; @@ -1284,9 +1287,18 @@ int __cdecl main(int argc, char *argv[]) 'C','o','n','t','r','o','l','\\', 'S','e','r','v','i','c','e','C','u','r','r','e','n','t',0}; static const WCHAR svcctl_started_event[] = SVCCTL_STARTED_EVENT; + JOBOBJECT_EXTENDED_LIMIT_INFORMATION job_limit; HANDLE started_event; DWORD err; + job_object = CreateJobObjectW(NULL, NULL); + job_limit.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_BREAKAWAY_OK | JOB_OBJECT_LIMIT_SILENT_BREAKAWAY_OK; + if (!SetInformationJobObject(job_object, JobObjectExtendedLimitInformation, &job_limit, sizeof(job_limit))) + { + WINE_ERR("Failed to initialized job object, err %lu.\n", GetLastError()); + return GetLastError(); + } + started_event = CreateEventW(NULL, TRUE, FALSE, svcctl_started_event); err = RegCreateKeyExW(HKEY_LOCAL_MACHINE, service_current_key_str, 0, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2536
From: Paul Gofman <pgofman(a)codeweavers.com> --- programs/services/rpc.c | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/programs/services/rpc.c b/programs/services/rpc.c index e56d25ea1af..863a605ef35 100644 --- a/programs/services/rpc.c +++ b/programs/services/rpc.c @@ -868,11 +868,32 @@ static void fill_notify(struct sc_notify_handle *notify, struct service_entry *s SetEvent(notify->event); } +static void notify_service_state(struct service_entry *service) +{ + struct sc_service_handle *service_handle; + DWORD mask; + + mask = 1 << (service->status.dwCurrentState - SERVICE_STOPPED); + LIST_FOR_EACH_ENTRY(service_handle, &service->handles, struct sc_service_handle, entry) + { + struct sc_notify_handle *notify = service_handle->notify; + if (notify && (notify->notify_mask & mask)) + { + fill_notify(notify, service); + sc_notify_release(notify); + service_handle->notify = NULL; + service_handle->status_notified = TRUE; + } + else + service_handle->status_notified = FALSE; + } +} + DWORD __cdecl svcctl_SetServiceStatus(SC_RPC_HANDLE handle, SERVICE_STATUS *status) { - struct sc_service_handle *service, *service_handle; + struct sc_service_handle *service; struct process_entry *process; - DWORD err, mask; + DWORD err; WINE_TRACE("(%p, %p)\n", handle, status); @@ -902,21 +923,7 @@ DWORD __cdecl svcctl_SetServiceStatus(SC_RPC_HANDLE handle, SERVICE_STATUS *stat release_process(process); } - mask = 1 << (service->service_entry->status.dwCurrentState - SERVICE_STOPPED); - LIST_FOR_EACH_ENTRY(service_handle, &service->service_entry->handles, struct sc_service_handle, entry) - { - struct sc_notify_handle *notify = service_handle->notify; - if (notify && (notify->notify_mask & mask)) - { - fill_notify(notify, service->service_entry); - sc_notify_release(notify); - service_handle->notify = NULL; - service_handle->status_notified = TRUE; - } - else - service_handle->status_notified = FALSE; - } - + notify_service_state(service->service_entry); service_unlock(service->service_entry); return ERROR_SUCCESS; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2536
From: Paul Gofman <pgofman(a)codeweavers.com> --- programs/services/rpc.c | 2 +- programs/services/services.c | 61 +++++++++++++++- programs/services/services.h | 1 + programs/services/tests/service.c | 117 ++++++++++++++++++++++++++++++ 4 files changed, 178 insertions(+), 3 deletions(-) diff --git a/programs/services/rpc.c b/programs/services/rpc.c index 863a605ef35..b3d5c9403fe 100644 --- a/programs/services/rpc.c +++ b/programs/services/rpc.c @@ -868,7 +868,7 @@ static void fill_notify(struct sc_notify_handle *notify, struct service_entry *s SetEvent(notify->event); } -static void notify_service_state(struct service_entry *service) +void notify_service_state(struct service_entry *service) { struct sc_service_handle *service_handle; DWORD mask; diff --git a/programs/services/services.c b/programs/services/services.c index 9efedab43d2..2db74f99e0f 100644 --- a/programs/services/services.c +++ b/programs/services/services.c @@ -47,7 +47,7 @@ static DWORD default_preshutdown_timeout = 180000; static DWORD autostart_delay = 120000; static void *environment = NULL; static HKEY service_current_key = NULL; -static HANDLE job_object; +static HANDLE job_object, job_completion_port; static const BOOL is_win64 = (sizeof(void *) > sizeof(int)); @@ -1280,6 +1280,50 @@ static void load_registry_parameters(void) RegCloseKey( key ); } +static DWORD WINAPI process_monitor_thread_proc( void *arg ) +{ + struct scmdatabase *db = active_database; + struct service_entry *service; + struct process_entry *process; + OVERLAPPED *overlapped; + ULONG_PTR value; + DWORD key, pid; + + while (GetQueuedCompletionStatus(job_completion_port, &key, &value, &overlapped, INFINITE)) + { + if (!key) + break; + if (key != JOB_OBJECT_MSG_EXIT_PROCESS) + continue; + pid = (ULONG_PTR)overlapped; + WINE_TRACE("pid %04lx exited.\n", pid); + scmdatabase_lock(db); + LIST_FOR_EACH_ENTRY(service, &db->services, struct service_entry, entry) + { + if (service->status.dwCurrentState != SERVICE_RUNNING || !service->process + || service->process->process_id != pid) continue; + + WINE_TRACE("Stopping service %s.\n", debugstr_w(service->config.lpBinaryPathName)); + service->status.dwCurrentState = SERVICE_STOPPED; + service->status.dwControlsAccepted = 0; + service->status.dwWin32ExitCode = ERROR_PROCESS_ABORTED; + service->status.dwServiceSpecificExitCode = 0; + service->status.dwCheckPoint = 0; + service->status.dwWaitHint = 0; + SetEvent(service->status_changed_event); + + process = service->process; + service->process = NULL; + process->use_count--; + release_process(process); + notify_service_state(service); + } + scmdatabase_unlock(db); + } + WINE_TRACE("Terminating.\n"); + return 0; +} + int __cdecl main(int argc, char *argv[]) { static const WCHAR service_current_key_str[] = { 'S','Y','S','T','E','M','\\', @@ -1288,7 +1332,8 @@ int __cdecl main(int argc, char *argv[]) 'S','e','r','v','i','c','e','C','u','r','r','e','n','t',0}; static const WCHAR svcctl_started_event[] = SVCCTL_STARTED_EVENT; JOBOBJECT_EXTENDED_LIMIT_INFORMATION job_limit; - HANDLE started_event; + JOBOBJECT_ASSOCIATE_COMPLETION_PORT port_info; + HANDLE started_event, process_monitor_thread; DWORD err; job_object = CreateJobObjectW(NULL, NULL); @@ -1298,6 +1343,15 @@ int __cdecl main(int argc, char *argv[]) WINE_ERR("Failed to initialized job object, err %lu.\n", GetLastError()); return GetLastError(); } + job_completion_port = CreateIoCompletionPort(INVALID_HANDLE_VALUE, NULL, 0, 1); + port_info.CompletionPort = job_completion_port; + port_info.CompletionKey = job_object; + if (!SetInformationJobObject(job_object, JobObjectAssociateCompletionPortInformation, + &port_info, sizeof(port_info))) + { + WINE_ERR("Failed to set completion port for job, err %lu.\n", GetLastError()); + return GetLastError(); + } started_event = CreateEventW(NULL, TRUE, FALSE, svcctl_started_event); @@ -1316,8 +1370,11 @@ int __cdecl main(int argc, char *argv[]) if ((err = RPC_Init()) == ERROR_SUCCESS) { scmdatabase_autostart_services(active_database); + process_monitor_thread = CreateThread(NULL, 0, process_monitor_thread_proc, NULL, 0, NULL); SetEvent(started_event); WaitForSingleObject(exit_event, INFINITE); + PostQueuedCompletionStatus(job_completion_port, 0, 0, NULL); + WaitForSingleObject(process_monitor_thread, INFINITE); scmdatabase_wait_terminate(active_database); if (delayed_autostart_cleanup) { diff --git a/programs/services/services.h b/programs/services/services.h index 908a36e6514..1d9dbfdcbff 100644 --- a/programs/services/services.h +++ b/programs/services/services.h @@ -93,6 +93,7 @@ void release_service(struct service_entry *service); void service_lock(struct service_entry *service); void service_unlock(struct service_entry *service); DWORD service_start(struct service_entry *service, DWORD service_argc, LPCWSTR *service_argv); +void notify_service_state(struct service_entry *service); /* Process functions */ diff --git a/programs/services/tests/service.c b/programs/services/tests/service.c index f069347ff37..99cf0838bf0 100644 --- a/programs/services/tests/service.c +++ b/programs/services/tests/service.c @@ -616,6 +616,122 @@ static void test_runner(void (*p_run_test)(void)) CloseHandle(thread); } +static void CALLBACK notify_cb(void *user) +{ +} + +static inline void test_kill_service_process(void) +{ + static const char *argv[2] = {"param1", "param2"}; + SC_HANDLE service_handle = register_service("simple_service"); + SERVICE_STATUS_PROCESS status2; + SERVICE_NOTIFYW notify; + SERVICE_STATUS status; + DWORD bytes, error; + HANDLE process; + BOOL res; + + if(!service_handle) + return; + + trace("starting...\n"); + res = StartServiceA(service_handle, 2, argv); + ok(res, "StartService failed: %lu\n", GetLastError()); + if(!res) { + DeleteService(service_handle); + CloseServiceHandle(service_handle); + return; + } + expect_event("RUNNING"); + + memset(¬ify, 0, sizeof(notify)); + notify.dwVersion = SERVICE_NOTIFY_STATUS_CHANGE; + notify.dwNotificationStatus = 0xdeadbeef; + notify.pfnNotifyCallback = notify_cb; + notify.pContext = ¬ify; + error = NotifyServiceStatusChangeW(service_handle, SERVICE_NOTIFY_STOPPED | SERVICE_NOTIFY_RUNNING + | SERVICE_NOTIFY_PAUSED | SERVICE_NOTIFY_PAUSE_PENDING | SERVICE_NOTIFY_STOP_PENDING, ¬ify); + ok(error == ERROR_SUCCESS, "got error %lu.\n", error); + + /* This shouldn't wait, we are supposed to get service start notification before. */ + SleepEx(5000, TRUE); + ok(!notify.dwNotificationStatus, "got %#lx.\n", notify.dwNotificationStatus); + ok(notify.dwNotificationTriggered == SERVICE_NOTIFY_RUNNING, "got %#lx.\n", notify.dwNotificationTriggered); + ok(notify.ServiceStatus.dwCurrentState == SERVICE_RUNNING, "got %#lx.\n", notify.ServiceStatus.dwCurrentState); + ok(!notify.ServiceStatus.dwWin32ExitCode, "got %#lx.\n", notify.ServiceStatus.dwWin32ExitCode); + + res = QueryServiceStatus(service_handle, &status); + ok(res, "got error %lu.\n", GetLastError()); + ok(status.dwServiceType == SERVICE_WIN32_OWN_PROCESS, "got %lx.\n", status.dwServiceType); + ok(status.dwCurrentState == SERVICE_RUNNING, "got %lx.\n", status.dwCurrentState); + + res = QueryServiceStatusEx(service_handle, SC_STATUS_PROCESS_INFO, (BYTE *)&status2, sizeof(status2), &bytes); + ok(res, "got error %lu.\n", GetLastError()); + ok(status2.dwCurrentState == SERVICE_RUNNING, "got %lx.\n", status2.dwCurrentState); + ok(status2.dwProcessId, "got %ld\n", status2.dwProcessId); + + process = OpenProcess(PROCESS_TERMINATE, FALSE, status2.dwProcessId); + ok(!!process, "got NULL.\n"); + res = TerminateProcess(process, 0xdeadbeef); + ok(res, "got error %lu.\n", GetLastError()); + CloseHandle(process); + + memset(¬ify, 0, sizeof(notify)); + notify.dwVersion = SERVICE_NOTIFY_STATUS_CHANGE; + notify.dwNotificationStatus = 0xdeadbeef; + notify.pfnNotifyCallback = notify_cb; + notify.pContext = ¬ify; + error = NotifyServiceStatusChangeW(service_handle, SERVICE_NOTIFY_STOPPED | SERVICE_NOTIFY_RUNNING + | SERVICE_NOTIFY_PAUSED | SERVICE_NOTIFY_PAUSE_PENDING | SERVICE_NOTIFY_STOP_PENDING, ¬ify); + ok(error == ERROR_SUCCESS, "got error %lu.\n", error); + + SleepEx(3000, TRUE); + ok(!notify.dwNotificationStatus, "got %#lx.\n", notify.dwNotificationStatus); + ok(notify.dwNotificationTriggered == SERVICE_NOTIFY_STOPPED, "got %#lx.\n", notify.dwNotificationTriggered); + ok(notify.ServiceStatus.dwCurrentState == SERVICE_STOPPED, "got %#lx.\n", notify.ServiceStatus.dwCurrentState); + ok(notify.ServiceStatus.dwWin32ExitCode == ERROR_PROCESS_ABORTED, "got %#lx.\n", notify.ServiceStatus.dwWin32ExitCode); + ok(!notify.ServiceStatus.dwServiceSpecificExitCode, "got %#lx.\n", notify.ServiceStatus.dwWin32ExitCode); + + res = QueryServiceStatus(service_handle, &status); + ok(res, "got error %lu.\n", error); + ok(status.dwServiceType == SERVICE_WIN32_OWN_PROCESS, "got %lx.\n", status.dwServiceType); + ok(status.dwCurrentState == SERVICE_STOPPED, "got %lx.\n", status.dwCurrentState); + ok(!status.dwControlsAccepted, "got %lx\n", status.dwControlsAccepted); + ok(status.dwWin32ExitCode == ERROR_PROCESS_ABORTED, "got %ld.\n", status.dwWin32ExitCode); + ok(!status.dwServiceSpecificExitCode, "got %ld.\n", + status.dwServiceSpecificExitCode); + ok(!status.dwCheckPoint, "got %ld.\n", status.dwCheckPoint); + ok(!status.dwWaitHint, "got %ld.\n", status.dwWaitHint); + + res = QueryServiceStatusEx(service_handle, SC_STATUS_PROCESS_INFO, (BYTE *)&status2, sizeof(status2), &bytes); + ok(res, "got error %lu.\n", error); + ok(!status2.dwProcessId, "got %ld.\n", status2.dwProcessId); + + res = DeleteService(service_handle); + ok(res, "got error %lu.\n", error); + + res = QueryServiceStatus(service_handle, &status); + ok(res, "got error %lu.\n", error); + ok(status.dwServiceType == SERVICE_WIN32_OWN_PROCESS, "got %lx.\n", status.dwServiceType); + ok(status.dwCurrentState == SERVICE_STOPPED, "got %lx.\n", status.dwCurrentState); + ok(status.dwControlsAccepted == 0, "got %lx.\n", status.dwControlsAccepted); + ok(status.dwWin32ExitCode == ERROR_PROCESS_ABORTED, "got %ld.\n", status.dwWin32ExitCode); + ok(status.dwServiceSpecificExitCode == 0, "got %ld.\n", + status.dwServiceSpecificExitCode); + ok(!status.dwCheckPoint, "got %ld.\n", status.dwCheckPoint); + ok(!status.dwWaitHint, "got %ld.\n", status.dwWaitHint); + + res = QueryServiceStatusEx(service_handle, SC_STATUS_PROCESS_INFO, (BYTE *)&status2, sizeof(status2), &bytes); + ok(res, "got error %lu.\n", GetLastError()); + ok(!status2.dwProcessId, "got %ld.\n", status2.dwProcessId); + + CloseServiceHandle(service_handle); + + res = QueryServiceStatus(service_handle, &status); + ok(!res, "QueryServiceStatus should have failed.\n"); + ok(GetLastError() == ERROR_INVALID_HANDLE, "got %ld.\n", GetLastError()); +} + START_TEST(service) { char **argv; @@ -641,6 +757,7 @@ START_TEST(service) if(argc < 3) { test_runner(test_service); test_runner(test_no_stop); + test_runner(test_kill_service_process); }else { strcpy(service_name, argv[3]); sprintf(named_pipe_name, "\\\\.\\pipe\\%s_pipe", service_name); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2536
participants (2)
-
Paul Gofman -
Paul Gofman (＠gofman)