public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
27+ messages / 2 participants
[nested] [flat]
* [PATCH v60 16/17] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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.31.0.121.g9198c13e34
--zefjd6adjk3g4ecc
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="v60-0017-ci-add.patch"
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid.
@ 2021-03-03 05:24 Andres Freund <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw)
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
--pm3lfdw7knjptu4k--
^ permalink raw reply [nested|flat] 27+ messages in thread
* pg_tablespace_location() failure with allow_in_place_tablespaces
@ 2022-03-04 06:44 Michael Paquier <[email protected]>
2022-03-07 11:36 ` Re: pg_tablespace_location() failure with allow_in_place_tablespaces Michael Paquier <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Michael Paquier @ 2022-03-04 06:44 UTC (permalink / raw)
To: Postgres hackers <[email protected]>
Hi all,
While playing with tablespaces and recovery in a TAP test, I have
noticed that retrieving the location of a tablespace created with
allow_in_place_tablespaces enabled fails in pg_tablespace_location(),
because readlink() sees a directory in this case.
The use may be limited to any automated testing and
allow_in_place_tablespaces is a developer GUC, still it seems to me
that there is an argument to allow the case rather than tweak any
tests to hardcode a path with the tablespace OID. And any other code
paths are able to handle such tablespaces, be they in recovery or in
tablespace create/drop.
A junction point is a directory on WIN32 as far as I recall, but
pgreadlink() is here to ensure that we get the correct path on
a source found as pgwin32_is_junction(), so we can rely on that. This
stuff has led me to the attached.
Thoughts?
--
Michael
Attachments:
[text/x-diff] tbspace-inplace-location.patch (4.3K, ../../[email protected]/2-tbspace-inplace-location.patch)
download | inline diff:
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index e79eb6b478..59b8f8196c 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -15,6 +15,7 @@
#include "postgres.h"
#include <sys/file.h>
+#include <sys/stat.h>
#include <dirent.h>
#include <fcntl.h>
#include <math.h>
@@ -307,8 +308,7 @@ pg_tablespace_location(PG_FUNCTION_ARGS)
{
Oid tablespaceOid = PG_GETARG_OID(0);
char sourcepath[MAXPGPATH];
- char targetpath[MAXPGPATH];
- int rllen;
+ struct stat st;
/*
* It's useful to apply this function to pg_class.reltablespace, wherein
@@ -333,20 +333,57 @@ pg_tablespace_location(PG_FUNCTION_ARGS)
*/
snprintf(sourcepath, sizeof(sourcepath), "pg_tblspc/%u", tablespaceOid);
- rllen = readlink(sourcepath, targetpath, sizeof(targetpath));
- if (rllen < 0)
+ /*
+ * Before reading the link, check if it is a link or a directory.
+ * A directory is possible for a tablespace created with
+ * allow_in_place_tablespaces enabled. On Windows, junction points
+ * are directories, which is why this is checked first.
+ */
+ if (lstat(sourcepath, &st) < 0)
+ {
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not read symbolic link \"%s\": %m",
+ errmsg("could not stat file \"%s\": %m",
sourcepath)));
- if (rllen >= sizeof(targetpath))
- ereport(ERROR,
- (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
- errmsg("symbolic link \"%s\" target is too long",
- sourcepath)));
- targetpath[rllen] = '\0';
+ }
+ else if (
+#ifndef WIN32
+ S_ISLNK(st.st_mode)
+#else
+ pgwin32_is_junction(pathbuf)
+#endif
+ )
+ {
+ char targetpath[MAXPGPATH];
+ int rllen;
- PG_RETURN_TEXT_P(cstring_to_text(targetpath));
+ rllen = readlink(sourcepath, targetpath, sizeof(targetpath));
+ if (rllen < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not read symbolic link \"%s\": %m",
+ sourcepath)));
+ if (rllen >= sizeof(targetpath))
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("symbolic link \"%s\" target is too long",
+ sourcepath)));
+ targetpath[rllen] = '\0';
+
+ PG_RETURN_TEXT_P(cstring_to_text(targetpath));
+ }
+ else if (S_ISDIR(st.st_mode))
+ {
+ /*
+ * For a directory, return the relative path of the source,
+ * as created by allow_in_place_tablespaces. This is useful
+ * for regression tests.
+ */
+ PG_RETURN_TEXT_P(cstring_to_text(sourcepath));
+ }
+ else
+ elog(ERROR, "\"%s\" is not a directory or symbolic link",
+ sourcepath);
#else
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
diff --git a/src/test/regress/expected/tablespace.out b/src/test/regress/expected/tablespace.out
index 2dfbcfdebe..473fe8c28e 100644
--- a/src/test/regress/expected/tablespace.out
+++ b/src/test/regress/expected/tablespace.out
@@ -24,6 +24,15 @@ SELECT spcoptions FROM pg_tablespace WHERE spcname = 'regress_tblspacewith';
DROP TABLESPACE regress_tblspacewith;
-- create a tablespace we can use
CREATE TABLESPACE regress_tblspace LOCATION '';
+-- This returns a relative path as of an effect of allow_in_place_tablespaces,
+-- masking the tablespace OID used in the path name.
+SELECT regexp_replace(pg_tablespace_location(oid), '(pg_tblspc)/(\d+)', '\1/NNN')
+ FROM pg_tablespace WHERE spcname = 'regress_tblspace';
+ regexp_replace
+----------------
+ pg_tblspc/NNN
+(1 row)
+
-- try setting and resetting some properties for the new tablespace
ALTER TABLESPACE regress_tblspace SET (random_page_cost = 1.0, seq_page_cost = 1.1);
ALTER TABLESPACE regress_tblspace SET (some_nonexistent_parameter = true); -- fail
diff --git a/src/test/regress/sql/tablespace.sql b/src/test/regress/sql/tablespace.sql
index 896f05cea3..0949a28488 100644
--- a/src/test/regress/sql/tablespace.sql
+++ b/src/test/regress/sql/tablespace.sql
@@ -22,6 +22,10 @@ DROP TABLESPACE regress_tblspacewith;
-- create a tablespace we can use
CREATE TABLESPACE regress_tblspace LOCATION '';
+-- This returns a relative path as of an effect of allow_in_place_tablespaces,
+-- masking the tablespace OID used in the path name.
+SELECT regexp_replace(pg_tablespace_location(oid), '(pg_tblspc)/(\d+)', '\1/NNN')
+ FROM pg_tablespace WHERE spcname = 'regress_tblspace';
-- try setting and resetting some properties for the new tablespace
ALTER TABLESPACE regress_tblspace SET (random_page_cost = 1.0, seq_page_cost = 1.1);
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: pg_tablespace_location() failure with allow_in_place_tablespaces
2022-03-04 06:44 pg_tablespace_location() failure with allow_in_place_tablespaces Michael Paquier <[email protected]>
@ 2022-03-07 11:36 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Michael Paquier @ 2022-03-07 11:36 UTC (permalink / raw)
To: Postgres hackers <[email protected]>; +Cc: Thomas Munro <[email protected]>
On Fri, Mar 04, 2022 at 03:44:22PM +0900, Michael Paquier wrote:
> The use may be limited to any automated testing and
> allow_in_place_tablespaces is a developer GUC, still it seems to me
> that there is an argument to allow the case rather than tweak any
> tests to hardcode a path with the tablespace OID. And any other code
> paths are able to handle such tablespaces, be they in recovery or in
> tablespace create/drop.
>
> A junction point is a directory on WIN32 as far as I recall, but
> pgreadlink() is here to ensure that we get the correct path on
> a source found as pgwin32_is_junction(), so we can rely on that. This
> stuff has led me to the attached.
Thomas, I'd rather fix this for the sake of the tests. One point is
that the function returns a relative path for in-place tablespaces,
but it would be easy enough to append a DataDir. What do you think?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 27+ messages in thread
end of thread, other threads:[~2022-03-07 11:36 UTC | newest]
Thread overview: 27+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-03-03 05:24 [PATCH v60 16/17] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]>
2022-03-04 06:44 pg_tablespace_location() failure with allow_in_place_tablespaces Michael Paquier <[email protected]>
2022-03-07 11:36 ` Re: pg_tablespace_location() failure with allow_in_place_tablespaces Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox