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 <andres@anarazel.de>
Reviewed-By: Magnus Hagander <magnus@hagander.net>
Discussion: https://postgr.es/m/20210305185752.3up5eq2eanb7ofmb@alap3.anarazel.de

Author: anarazel

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)
 	{

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))