From 01606cfec52fe4af7c61cdda32d1dcf15d4268cb Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Tue, 7 Sep 2021 11:56:13 -0700 Subject: [PATCH v2 1/3] windows: Fix windows logging problems This commit is a combination of 54213cb30968c6679050c005ebd1363b77d797c5, 950e64fa46b164df87b5eb7c6e15213ab9880f87 and 92f478657c5544eba560047c39eba8a030ddb83e. These three commits need to be backpatched together. 54213cb30968c6679050c005ebd1363b77d797c5: 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 950e64fa46b164df87b5eb7c6e15213ab9880f87: Use STDOUT/STDERR_FILENO in most of syslogger. This fixes problems on windows when logging collector is used in a service, failing with: FATAL: could not redirect stderr: Bad file descriptor This is triggered by 76e38b37a5. The problem is that STDOUT/STDERR_FILENO aren't defined on windows, which lead us to use _fileno(stdout) etc, but that doesn't work if stdout/stderr are closed. Author: Andres Freund Reported-By: Sandeep Thakkar Message-Id: 20220520164558.ozb7lm6unakqzezi@alap3.anarazel.de (on pgsql-packagers) Backpatch: 15-, where 76e38b37a5 came in 92f478657c5544eba560047c39eba8a030ddb83e: windows: msvc: Define STDIN/OUT/ERR_FILENO. Because they are not available we've used _fileno(stdin) in some places, but that doesn't reliably work, because stdin might be closed. This is the prerequisite of the subsequent commit, fixing a failure introduced in 76e38b37a5. Author: Andres Freund Reported-By: Sandeep Thakkar Message-Id: 20220520164558.ozb7lm6unakqzezi@alap3.anarazel.de (on pgsql-packagers) Backpatch: 15-, where 76e38b37a5 came in --- src/backend/postmaster/syslogger.c | 18 +++++++-------- src/bin/pg_ctl/pg_ctl.c | 33 ++++++++++++++++++++++++++++ src/include/port/win32_msvc/unistd.h | 8 +++++++ src/port/win32security.c | 18 ++++++++++++--- 4 files changed, 65 insertions(+), 12 deletions(-) diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c index cad43bdef23..0bf1e438f5b 100644 --- a/src/backend/postmaster/syslogger.c +++ b/src/backend/postmaster/syslogger.c @@ -196,12 +196,12 @@ SysLoggerMain(int argc, char *argv[]) * if they fail then presumably the file descriptors are closed and * any writes will go into the bitbucket anyway. */ - close(fileno(stdout)); - close(fileno(stderr)); + close(STDOUT_FILENO); + close(STDERR_FILENO); if (fd != -1) { - (void) dup2(fd, fileno(stdout)); - (void) dup2(fd, fileno(stderr)); + (void) dup2(fd, STDOUT_FILENO); + (void) dup2(fd, STDERR_FILENO); close(fd); } } @@ -213,7 +213,7 @@ SysLoggerMain(int argc, char *argv[]) */ #ifdef WIN32 else - _setmode(_fileno(stderr), _O_TEXT); + _setmode(STDERR_FILENO, _O_TEXT); #endif /* @@ -675,12 +675,12 @@ SysLogger_Start(void) #ifndef WIN32 fflush(stdout); - if (dup2(syslogPipe[1], fileno(stdout)) < 0) + if (dup2(syslogPipe[1], STDOUT_FILENO) < 0) ereport(FATAL, (errcode_for_file_access(), errmsg("could not redirect stdout: %m"))); fflush(stderr); - if (dup2(syslogPipe[1], fileno(stderr)) < 0) + if (dup2(syslogPipe[1], STDERR_FILENO) < 0) ereport(FATAL, (errcode_for_file_access(), errmsg("could not redirect stderr: %m"))); @@ -697,12 +697,12 @@ SysLogger_Start(void) fflush(stderr); fd = _open_osfhandle((intptr_t) syslogPipe[1], _O_APPEND | _O_BINARY); - if (dup2(fd, _fileno(stderr)) < 0) + if (dup2(fd, STDERR_FILENO) < 0) ereport(FATAL, (errcode_for_file_access(), errmsg("could not redirect stderr: %m"))); close(fd); - _setmode(_fileno(stderr), _O_BINARY); + _setmode(STDERR_FILENO, _O_BINARY); /* * Now we are done with the write end of the pipe. diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index 552e3a6a1c8..ed4ac27fc43 100644 --- a/src/bin/pg_ctl/pg_ctl.c +++ b/src/bin/pg_ctl/pg_ctl.c @@ -1773,6 +1773,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. @@ -1810,6 +1835,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/include/port/win32_msvc/unistd.h b/src/include/port/win32_msvc/unistd.h index b63f4770a1c..b7795ba03c4 100644 --- a/src/include/port/win32_msvc/unistd.h +++ b/src/include/port/win32_msvc/unistd.h @@ -1 +1,9 @@ /* src/include/port/win32_msvc/unistd.h */ + +/* + * MSVC does not define these, nor does _fileno(stdin) etc reliably work + * (returns -1 if stdin/out/err are closed). + */ +#define STDIN_FILENO 0 +#define STDOUT_FILENO 1 +#define STDERR_FILENO 2 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.40.1