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 <[email protected]>
Reviewed-By: Magnus Hagander <[email protected]>
Discussion: https://postgr.es/m/20210305185752[email protected]

diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 5c6d07ee79eabc73293f70fc5853fddee657b035..7fbbe7022e2e5bff5d8610e9a728170f9323c814 100644 (file)
--- 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 4a673fde19a475dc1d3f93455542610c3f8baa66..6a1bd9b865434ac4d8380c6adb8b656224396d71 100644 (file)
--- 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))