Hi, On 2021-03-05 10:57:52 -0800, Andres Freund wrote: > On 2021-03-04 12:48:59 -0800, Andres Freund wrote: > > On 2021-03-04 21:36:23 +0100, Magnus Hagander wrote: > > > > I think that's a good answer for pg_ctl - not so sure about postgres > > > > itself, at least once it's up and running. I don't know what lead to all > > > > of this autodetection stuff, but is there the possibility of blocking on > > > > whatever stderr is set too as a service? > > > > > > > > Perhaps we could make the service detection more reliable by checking > > > > whether stderr is actually something useful? > > > > > > So IIRC, and mind that this is like 15 years ago, there is something > > > that looks like stderr, but the contents are thrown away. It probably > > > exists specifically so that programs won't crash when run as a > > > service... > > > > Yea, that'd make sense. > > > > I wish we had tests for the service stuff, but that's from long before > > there were tap tests... > > After fighting with a windows VM for a bit (ugh), it turns out that yes, > there is stderr, but that fileno(stderr) returns -2, and > GetStdHandle(STD_ERROR_HANDLE) returns NULL (not INVALID_HANDLE_VALUE). > > The complexity however is that while that's true for pg_ctl within > pgwin32_ServiceMain: > checking what stderr=00007FF8687DFCB0 is (handle: 0, fileno=-2) > but not for postmaster or backends > WARNING: 01000: checking what stderr=00007FF880F5FCB0 is (handle: 92, > fileno=2) > > which makes sense in a way, because we don't tell CreateProcessAsUser() > that it should pass stdin/out/err down (which then seems to magically > get access to the "topmost" console applications output - damn, this > stuff is weird).
That part is not too hard to address - it seems we only need to do that in pg_ctl pgwin32_doRunAsService(). It seems that the stdin/stderr/stdout being set to invalid will then be passed down to postmaster children. https://docs.microsoft.com/en-us/windows/console/getstdhandle "If an application does not have associated standard handles, such as a service running on an interactive desktop, and has not redirected them, the return value is NULL." There does seem to be some difference between what services get as std* - GetStdHandle() returns NULL, and what explicitly passing down invalid handles to postmaster does - GetStdHandle() returns INVALID_HANDLE_VALUE. But passing down NULL rather than INVALID_HANDLE_VALUE to postmaster seems to lead to postmaster re-opening console buffers. Patch attached. I'd like to commit something to address this issue to master soon - it allows us to run a lot more tests in cirrus-ci... But probably not backpatch it [yet] - there've not really been field complaints, and who knows if there end up being some unintentional side-effects... > You'd earlier mentioned that other distributions may not use pg_ctl > register - but I assume they use pg_ctl runservice? Or do they actually > re-implement all those magic incantations in pg_ctl.c? It seems that we, in addition to the above patch, should add a guc that pg_ctl runservice passes down to postgres. And then rip out the call to pgwin32_is_service() from the backend. That doesn't require other distributions to use pg_ctl register, just pg_ctl runservice - which I think they need to do anyway, unless they want to duplicate all the logic around pgwin32_SetServiceStatus()? Greetings, Andres Freund
>From 86fb1f3da2db85d513f491d88b638ea7cc1327b8 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Tue, 2 Mar 2021 21:24:11 -0800 Subject: [PATCH v2] 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. In addition to this change, it likely would be a good idea to have pg_ctl runservice pass down a flag indicating that postgres is running as a service. Author: Andres Freund <and...@anarazel.de> Discussion: https://postgr.es/m/20210305185752.3up5eq2eanb7o...@alap3.anarazel.de --- src/port/win32security.c | 13 +++++++++++-- src/bin/pg_ctl/pg_ctl.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/port/win32security.c b/src/port/win32security.c index 4a673fde19a..b57ce61d752 100644 --- a/src/port/win32security.c +++ b/src/port/win32security.c @@ -95,8 +95,9 @@ 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) + * 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,11 +122,19 @@ 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; + stderr_handle = GetStdHandle(STD_ERROR_HANDLE); + if (stderr_handle != INVALID_HANDLE_VALUE && stderr_handle != NULL) + { + _is_service = 0; + return _is_service; + } + /* First check for LocalSystem */ if (!AllocateAndInitializeSid(&NtAuthority, 1, SECURITY_LOCAL_SYSTEM_RID, 0, 0, 0, 0, 0, 0, 0, diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index 7985da0a943..c99e3c507de 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) { -- 2.29.2.540.g3cf59784d4
