From 76e38b37a5f179d4c9d2865ff31b79130407530b Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Tue, 7 Sep 2021 11:56:13 -0700 Subject: [PATCH] windows: Only consider us to be running as service if stderr is invalid. Previously pgwin32_is_service() would falsely return true when postgres is started from somewhere within a service, but not as a service. That is e.g. always the case with windows docker containers, which some CI services use to run windows tests in. When postgres falsely thinks its running as a service, no messages are writting to stdout / stderr. That can be very confusing and causes a few tests to fail. To fix additionally check if stderr is invalid in pgwin32_is_service(). For that to work in backend processes, pg_ctl is changed to pass down handles so that postgres can do the same check (otherwise "default" handles are created). While this problem exists in all branches, there have been no reports by users, the prospective CI usage currently is only for master, and I am not a windows expert. So doing the change in only master for now seems the sanest approach. Author: Andres Freund Reviewed-By: Magnus Hagander Discussion: https://postgr.es/m/20210305185752.3up5eq2eanb7ofmb@alap3.anarazel.de --- src/bin/pg_ctl/pg_ctl.c | 33 +++++++++++++++++++++++++++++++++ src/port/win32security.c | 18 +++++++++++++++--- 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index 5c6d07ee79e..7fbbe7022e2 100644 --- a/src/bin/pg_ctl/pg_ctl.c +++ b/src/bin/pg_ctl/pg_ctl.c @@ -1737,6 +1737,31 @@ typedef BOOL (WINAPI * __SetInformationJobObject) (HANDLE, JOBOBJECTINFOCLASS, L typedef BOOL (WINAPI * __AssignProcessToJobObject) (HANDLE, HANDLE); typedef BOOL (WINAPI * __QueryInformationJobObject) (HANDLE, JOBOBJECTINFOCLASS, LPVOID, DWORD, LPDWORD); +/* + * Set up STARTUPINFO for the new process to inherit this process' handles. + * + * Process started as services appear to have "empty" handles (GetStdHandle() + * returns NULL) rather than invalid ones. But passing down NULL ourselves + * doesn't work, it's interpreted as STARTUPINFO->hStd* not being set. But we + * can pass down INVALID_HANDLE_VALUE - which makes GetStdHandle() in the new + * process (and its child processes!) return INVALID_HANDLE_VALUE. Which + * achieves the goal of postmaster running in a similar environment as pg_ctl. + */ +static void +InheritStdHandles(STARTUPINFO* si) +{ + si->dwFlags |= STARTF_USESTDHANDLES; + si->hStdInput = GetStdHandle(STD_INPUT_HANDLE); + if (si->hStdInput == NULL) + si->hStdInput = INVALID_HANDLE_VALUE; + si->hStdOutput = GetStdHandle(STD_OUTPUT_HANDLE); + if (si->hStdOutput == NULL) + si->hStdOutput = INVALID_HANDLE_VALUE; + si->hStdError = GetStdHandle(STD_ERROR_HANDLE); + if (si->hStdError == NULL) + si->hStdError = INVALID_HANDLE_VALUE; +} + /* * Create a restricted token, a job object sandbox, and execute the specified * process with it. @@ -1774,6 +1799,14 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_ser ZeroMemory(&si, sizeof(si)); si.cb = sizeof(si); + /* + * Set stdin/stdout/stderr handles to be inherited in the child + * process. That allows postmaster and the processes it starts to perform + * additional checks to see if running in a service (otherwise they get + * the default console handles - which point to "somewhere"). + */ + InheritStdHandles(&si); + Advapi32Handle = LoadLibrary("ADVAPI32.DLL"); if (Advapi32Handle != NULL) { diff --git a/src/port/win32security.c b/src/port/win32security.c index 4a673fde19a..6a1bd9b8654 100644 --- a/src/port/win32security.c +++ b/src/port/win32security.c @@ -95,8 +95,11 @@ pgwin32_is_admin(void) * We consider ourselves running as a service if one of the following is * true: * - * 1) We are running as LocalSystem (only used by services) - * 2) Our token contains SECURITY_SERVICE_RID (automatically added to the + * 1) Standard error is not valid (always the case for services, and pg_ctl + * running as a service "passes" that down to postgres, + * c.f. CreateRestrictedProcess()) + * 2) We are running as LocalSystem (only used by services) + * 3) Our token contains SECURITY_SERVICE_RID (automatically added to the * process token by the SCM when starting a service) * * The check for LocalSystem is needed, because surprisingly, if a service @@ -121,12 +124,21 @@ pgwin32_is_service(void) PSID ServiceSid; PSID LocalSystemSid; SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY}; + HANDLE stderr_handle; /* Only check the first time */ if (_is_service != -1) return _is_service; - /* First check for LocalSystem */ + /* Check if standard error is not valid */ + stderr_handle = GetStdHandle(STD_ERROR_HANDLE); + if (stderr_handle != INVALID_HANDLE_VALUE && stderr_handle != NULL) + { + _is_service = 0; + return _is_service; + } + + /* Check if running as LocalSystem */ if (!AllocateAndInitializeSid(&NtAuthority, 1, SECURITY_LOCAL_SYSTEM_RID, 0, 0, 0, 0, 0, 0, 0, &LocalSystemSid)) -- 2.39.5