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.
28+ messages / 2 participants
[nested] [flat]
* [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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ messages in thread
* [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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ messages in thread
* Re: CI and test improvements
@ 2024-06-14 15:22 Justin Pryzby <[email protected]>
2024-06-14 15:34 ` Re: CI and test improvements Andres Freund <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Justin Pryzby @ 2024-06-14 15:22 UTC (permalink / raw)
To: Nazir Bilal Yavuz <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; vignesh C <[email protected]>; Michael Paquier <[email protected]>; Andres Freund <[email protected]>; Andrew Dunstan <[email protected]>; Thomas Munro <[email protected]>; [email protected]; Noah Misch <[email protected]>; Anastasia Lubennikova <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; Melanie Plageman <[email protected]>; Peter Eisentraut <[email protected]>; Daniel Gustafsson <[email protected]>; samay sharma <[email protected]>
On Fri, Jun 14, 2024 at 05:36:54PM +0300, Nazir Bilal Yavuz wrote:
> Hi,
>
> On Thu, 13 Jun 2024 at 14:56, Justin Pryzby <[email protected]> wrote:
> >
> > ccache should be installed in the image rather than re-installed on each
> > invocation.
>
> ccache is installed in the Windows VM images now [1]. It can be used
> as 'set CC=ccache.exe cl.exe' in the Windows CI task.
>
> [1] https://github.com/anarazel/pg-vm-images/commit/03a9225ac962fb30b5c0722c702941e2d7c1e81e
Thanks. I think that works by using a "shim" created by choco in
C:\ProgramData\chocolatey\bin.
But going through that indirection seems to incur an extra 15sec of
compilation time; I think we'll want to do something to avoid that.
I'm not sure what the options are, like maybe installing ccache into a
fixed path like c:\ccache or installing a custom link, to a "pinned"
version of ccache.
--
Justin
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: CI and test improvements
2024-06-14 15:22 Re: CI and test improvements Justin Pryzby <[email protected]>
@ 2024-06-14 15:34 ` Andres Freund <[email protected]>
2024-06-18 13:36 ` Re: CI and test improvements Justin Pryzby <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Andres Freund @ 2024-06-14 15:34 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; Nazir Bilal Yavuz <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; vignesh C <[email protected]>; Michael Paquier <[email protected]>; Andrew Dunstan <[email protected]>; Thomas Munro <[email protected]>; [email protected]; Noah Misch <[email protected]>; Anastasia Lubennikova <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; Melanie Plageman <[email protected]>; Peter Eisentraut <[email protected]>; Daniel Gustafsson <[email protected]>; samay sharma <[email protected]>
Hi,
On June 14, 2024 8:22:01 AM PDT, Justin Pryzby <[email protected]> wrote:
>On Fri, Jun 14, 2024 at 05:36:54PM +0300, Nazir Bilal Yavuz wrote:
>> Hi,
>>
>> On Thu, 13 Jun 2024 at 14:56, Justin Pryzby <[email protected]> wrote:
>> >
>> > ccache should be installed in the image rather than re-installed on each
>> > invocation.
>>
>> ccache is installed in the Windows VM images now [1]. It can be used
>> as 'set CC=ccache.exe cl.exe' in the Windows CI task.
>>
>> [1] https://github.com/anarazel/pg-vm-images/commit/03a9225ac962fb30b5c0722c702941e2d7c1e81e
>
>Thanks. I think that works by using a "shim" created by choco in
>C:\ProgramData\chocolatey\bin.
>
>But going through that indirection seems to incur an extra 15sec of
>compilation time; I think we'll want to do something to avoid that.
>
>I'm not sure what the options are, like maybe installing ccache into a
>fixed path like c:\ccache or installing a custom link, to a "pinned"
>version of ccache.
Hm. There actually already is the mingw ccache installed. The images for mingw and msvc used to be separate (for startup performance when using containers), but we just merged them. So it might be easiest to just explicitly use the ccache from there (also an explicit path might be faster). Could you check if that's fast? If not, we can just install the binaries distributed by the project, it's just more work to keep up2date that way.
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: CI and test improvements
2024-06-14 15:22 Re: CI and test improvements Justin Pryzby <[email protected]>
2024-06-14 15:34 ` Re: CI and test improvements Andres Freund <[email protected]>
@ 2024-06-18 13:36 ` Justin Pryzby <[email protected]>
0 siblings, 0 replies; 28+ messages in thread
From: Justin Pryzby @ 2024-06-18 13:36 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; vignesh C <[email protected]>; Michael Paquier <[email protected]>; Andrew Dunstan <[email protected]>; Thomas Munro <[email protected]>; [email protected]; Noah Misch <[email protected]>; Anastasia Lubennikova <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; Melanie Plageman <[email protected]>; Peter Eisentraut <[email protected]>; Daniel Gustafsson <[email protected]>; samay sharma <[email protected]>
On Fri, Jun 14, 2024 at 08:34:37AM -0700, Andres Freund wrote:
> Hm. There actually already is the mingw ccache installed. The images for mingw and msvc used to be separate (for startup performance when using containers), but we just merged them. So it might be easiest to just explicitly use the ccache from there
> (also an explicit path might be faster).
I don't think the path search is significant; it's fast so long as
there's no choco stub involved.
> Could you check if that's fast?
Yes, it is.
> If not, we can just install the binaries distributed by the project, it's just more work to keep up2date that way.
I guess you mean a separate line to copy choco's ccache.exe somewhere.
--
Justin
Attachments:
[text/x-diff] 0001-cirrus-windows-add-compiler_warnings_script.patch (1.9K, ../../ZnGNeXbET2EL5zkw@pryzbyj2023/2-0001-cirrus-windows-add-compiler_warnings_script.patch)
download | inline diff:
From ad03da2cfa3ecf23334f8b31f2e4529ba3094512 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email protected]>
Date: Wed, 25 May 2022 21:53:22 -0500
Subject: [PATCH 1/7] cirrus/windows: add compiler_warnings_script
The goal is to fail due to warnings only after running tests.
(At least historically, it's too slow to run a separate windows VM to
compile with -Werror.)
https://www.postgresql.org/message-id/20220212212310.f645c6vw3njkgxka%40alap3.anarazel.de
I'm not sure how to write this test in windows shell; it's also easy to
write something that doesn't work in posix sh, since windows shell is
interpretting && and ||...
See also:
8a1ce5e54f6d144e4f8e19af7c767b026ee0c956
https://cirrus-ci.com/task/6241060062494720
https://cirrus-ci.com/task/6496366607204352
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
ci-os-only: windows
---
.cirrus.tasks.yml | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index 33646faeadf..5a2b64f64c2 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -566,12 +566,21 @@ task:
build_script: |
vcvarsall x64
- ninja -C build
+ ninja -C build |tee build.txt
+ REM Since pipes lose the exit status of the preceding command, rerun the compilation
+ REM without the pipe, exiting now if it fails, to avoid trying to run checks
+ ninja -C build > nul
check_world_script: |
vcvarsall x64
meson test %MTEST_ARGS% --num-processes %TEST_JOBS%
+ # This should be last, so check_world is run even if there are warnings
+ always:
+ compiler_warnings_script:
+ # this avoids using metachars which would be interpretted by the windows shell
+ - sh -c 'if grep ": warning " build.txt; then exit 1; fi; exit 0'
+
on_failure:
<<: *on_failure_meson
crashlog_artifacts:
--
2.42.0
[text/x-diff] 0002-cirrus-windows-ccache.patch (2.5K, ../../ZnGNeXbET2EL5zkw@pryzbyj2023/3-0002-cirrus-windows-ccache.patch)
download | inline diff:
From 78031c45afad402397e4efa1389e6396557cd405 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email protected]>
Date: Wed, 25 May 2022 22:05:13 -0500
Subject: [PATCH 2/7] cirrus/windows: ccache
ccache 4.7 added support for depend mode with MSVC, and ccache v4.10
supports PCH, so this is now viable.
https://www.postgresql.org/message-id/flat/20220522232606.GZ19626%40telsasoft.com
https://cirrus-ci.com/task/5213936495099904 build 29sec
---
.cirrus.tasks.yml | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index 5a2b64f64c2..6ebbc1ccd81 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -536,6 +536,12 @@ task:
env:
TEST_JOBS: 8 # wild guess, data based value welcome
+ CC: c:\msys64\ucrt64\bin\ccache.exe cl.exe
+ CCACHE_DIR: $CIRRUS_WORKING_DIR/.ccache
+ CCACHE_MAXSIZE: "500M"
+ CCACHE_SLOPPINESS: pch_defines,time_macros
+ CCACHE_DEPEND: 1
+
# Cirrus defaults to SetErrorMode(SEM_NOGPFAULTERRORBOX | ...). That
# prevents crash reporting from working unless binaries do SetErrorMode()
# themselves. Furthermore, it appears that either python or, more likely,
@@ -553,6 +559,9 @@ task:
setup_additional_packages_script: |
REM choco install -y --no-progress ...
+ ccache_cache:
+ folder: $CCACHE_DIR
+
setup_hosts_file_script: |
echo 127.0.0.1 pg-loadbalancetest >> c:\Windows\System32\Drivers\etc\hosts
echo 127.0.0.2 pg-loadbalancetest >> c:\Windows\System32\Drivers\etc\hosts
@@ -562,7 +571,7 @@ task:
# Use /DEBUG:FASTLINK to avoid high memory usage during linking
configure_script: |
vcvarsall x64
- meson setup --backend ninja --buildtype debug -Dc_link_args=/DEBUG:FASTLINK -Dcassert=true -Dinjection_points=true -Db_pch=true -Dextra_lib_dirs=c:\openssl\1.1\lib -Dextra_include_dirs=c:\openssl\1.1\include -DTAR=%TAR% -DPG_TEST_EXTRA="%PG_TEST_EXTRA%" build
+ meson setup build --backend ninja --buildtype debug -Dc_link_args=/DEBUG:FASTLINK -Dcassert=true -Dinjection_points=true -Db_pch=true -Dextra_lib_dirs=c:\openssl\1.1\lib -Dextra_include_dirs=c:\openssl\1.1\include -DTAR=%TAR% -DPG_TEST_EXTRA="%PG_TEST_EXTRA%" -Dc_args="/Z7"
build_script: |
vcvarsall x64
@@ -571,6 +580,9 @@ task:
REM without the pipe, exiting now if it fails, to avoid trying to run checks
ninja -C build > nul
+ always:
+ upload_caches: [ccache]
+
check_world_script: |
vcvarsall x64
meson test %MTEST_ARGS% --num-processes %TEST_JOBS%
--
2.42.0
[text/x-diff] 0003-WIP-ci-meson-allow-showing-only-failed-tests.patch (4.0K, ../../ZnGNeXbET2EL5zkw@pryzbyj2023/4-0003-WIP-ci-meson-allow-showing-only-failed-tests.patch)
download | inline diff:
From cc09e8363d56933b8d62ebafb8319c91fed3b03f Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email protected]>
Date: Fri, 25 Nov 2022 13:57:17 -0600
Subject: [PATCH 3/7] WIP: ci/meson: allow showing only failed tests ..
It's simpler and seems to make more sense to integrate this with
testwrap, rather than to run it after check-world, but only if it
failed, and finding a way to preserve the exit code.
https://www.postgresql.org/message-id/[email protected]
> It is wasteful to upload thousdands of logfiles to show a single
> failure. That would make our cirrus tasks faster - compressing and
> uploading the logs takes over a minute.
>
> It's also a lot friendlier to show fewer than 8 pages of test folders to
> search through to find the one that failed.
macos
linux-meson
ci-os-only: freebsd
---
.cirrus.tasks.yml | 14 ++++++++------
src/tools/testwrap | 6 ++++++
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index 6ebbc1ccd81..56061e61a3e 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -21,6 +21,7 @@ env:
PGCTLTIMEOUT: 120 # avoids spurious failures during parallel tests
TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
PG_TEST_EXTRA: kerberos ldap ssl libpq_encryption load_balance
+ PG_FAILED_TESTDIR: ${CIRRUS_WORKING_DIR}/failed.build
# What files to preserve in case tests fail
@@ -35,9 +36,9 @@ on_failure_ac: &on_failure_ac
on_failure_meson: &on_failure_meson
testrun_artifacts:
paths:
- - "build*/testrun/**/*.log"
- - "build*/testrun/**/*.diffs"
- - "build*/testrun/**/regress_log_*"
+ - failed.build*/**/*.log
+ - failed.build*/**/*.diffs
+ - failed.build*/**/regress_log_*
type: text/plain
# In theory it'd be nice to upload the junit files meson generates, so that
@@ -136,6 +137,7 @@ task:
CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
CFLAGS: -Og -ggdb
+ PG_FAILED_TESTDIR: ${CIRRUS_WORKING_DIR}/failed.build
PG_TEST_PG_UPGRADE_MODE: --link
<<: *freebsd_task_template
@@ -194,10 +196,10 @@ task:
ulimit -c unlimited
meson test $MTEST_ARGS --quiet --suite setup
export LD_LIBRARY_PATH="$(pwd)/build/tmp_install/usr/local/pgsql/lib/:$LD_LIBRARY_PATH"
- mkdir -p build/testrun
+ mkdir -p build/testrun ${PG_FAILED_TESTDIR}
build/tmp_install/usr/local/pgsql/bin/initdb -N build/runningcheck --no-instructions -A trust
echo "include '$(pwd)/src/tools/ci/pg_ci_base.conf'" >> build/runningcheck/postgresql.conf
- build/tmp_install/usr/local/pgsql/bin/pg_ctl -c -o '-c fsync=off' -D build/runningcheck -l build/testrun/runningcheck.log start
+ build/tmp_install/usr/local/pgsql/bin/pg_ctl -c -o '-c fsync=off' -D build/runningcheck -l ${PG_FAILED_TESTDIR}/runningcheck.log start
meson test $MTEST_ARGS --num-processes ${TEST_JOBS} --setup running
build/tmp_install/usr/local/pgsql/bin/pg_ctl -D build/runningcheck stop
EOF
@@ -400,7 +402,7 @@ task:
test_world_32_script: |
su postgres <<-EOF
ulimit -c unlimited
- PYTHONCOERCECLOCALE=0 LANG=C meson test $MTEST_ARGS -C build-32 --num-processes ${TEST_JOBS}
+ PYTHONCOERCECLOCALE=0 LANG=C PG_FAILED_TESTDIR=`pwd`/failed.build-32 meson test $MTEST_ARGS -C build-32 --num-processes ${TEST_JOBS}
EOF
on_failure:
diff --git a/src/tools/testwrap b/src/tools/testwrap
index d01e61051cb..7e5a17dbf94 100755
--- a/src/tools/testwrap
+++ b/src/tools/testwrap
@@ -49,4 +49,10 @@ if sp.returncode == 0:
else:
print('# test failed')
open(os.path.join(testdir, 'test.fail'), 'x')
+ faileddir = os.getenv('PG_FAILED_TESTDIR')
+ if faileddir:
+ parentdir = os.path.dirname(testdir)
+ newdest = os.path.join(faileddir, os.path.basename(parentdir), os.path.basename(testdir))
+ shutil.copytree(testdir, newdest)
+
sys.exit(sp.returncode)
--
2.42.0
[text/x-diff] 0004-WIP-cirrus-upload-changed-html-docs-as-artifacts.patch (3.3K, ../../ZnGNeXbET2EL5zkw@pryzbyj2023/5-0004-WIP-cirrus-upload-changed-html-docs-as-artifacts.patch)
download | inline diff:
From 3a399c6350ed2f341425431f184e382c3f46d981 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email protected]>
Date: Sat, 26 Feb 2022 19:39:10 -0600
Subject: [PATCH 4/7] WIP: cirrus: upload changed html docs as artifacts
This could be done on the client side (cfbot). One advantage of doing
it here is that fewer docs are uploaded - many patches won't upload docs
at all.
https://www.postgresql.org/message-id/flat/[email protected]
https://www.postgresql.org/message-id/CAB8KJ=i4qmEuopQ+PCSMBzGd4O-Xv0FCnC+q1x7hN9hsdvkBug@mail.gmail.com
https://cirrus-ci.com/task/5396696388599808
//-os-only:
---
.cirrus.tasks.yml | 25 ++++++++++++++++++++++++-
src/tools/ci/copy-changed-docs | 29 +++++++++++++++++++++++++++++
2 files changed, 53 insertions(+), 1 deletion(-)
create mode 100755 src/tools/ci/copy-changed-docs
diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index 56061e61a3e..0ce5b172331 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -23,6 +23,15 @@ env:
PG_TEST_EXTRA: kerberos ldap ssl libpq_encryption load_balance
PG_FAILED_TESTDIR: ${CIRRUS_WORKING_DIR}/failed.build
+ # The commit that this branch is rebased on. There's no easy way to find this.
+ # This does the right thing for cfbot, which always squishes all patches into a single commit.
+ # And does the right thing for any 1-patch commits.
+ # Patch series manually submitted to cirrus would benefit from setting this to the
+ # number of patches in the series (or directly to the commit the series was rebased on).
+ #BASE_COMMIT: HEAD~1
+ # For demo purposes:
+ BASE_COMMIT: HEAD~55
+
# What files to preserve in case tests fail
on_failure_ac: &on_failure_ac
@@ -769,7 +778,7 @@ task:
time make -s -j${BUILD_JOBS} world-bin
###
- # Verify docs can be built
+ # Verify docs can be built, and upload changed docs as artifacts
###
# XXX: Only do this if there have been changes in doc/ since last build
always:
@@ -781,6 +790,20 @@ task:
CLANG="ccache clang"
make -s -j${BUILD_JOBS} clean
time make -s -j${BUILD_JOBS} -C doc
+ cp -r doc new-docs
+
+ # Re-build HTML docs from the base commit.
+ git checkout "$BASE_COMMIT" -- doc
+ make -s -C doc clean
+ time make -s -C doc html
+ cp -r doc old-docs
+
+ copy_changed_docs_script:
+ - src/tools/ci/copy-changed-docs "old-docs" "new-docs" "html_docs"
+
+ html_docs_artifacts:
+ paths: ['html_docs/*.html', 'html_docs/*.png', 'html_docs/*.css']
+
###
# Verify headerscheck / cpluspluscheck succeed
diff --git a/src/tools/ci/copy-changed-docs b/src/tools/ci/copy-changed-docs
new file mode 100755
index 00000000000..1c921a8df6f
--- /dev/null
+++ b/src/tools/ci/copy-changed-docs
@@ -0,0 +1,29 @@
+#! /bin/sh
+# Copy HTML which differ between $old and $new into $outdir
+set -e
+
+old=$1
+new=$2
+outdir=$3
+
+# The index is large and changes often
+skippages="bookindex.html"
+
+mkdir "$outdir"
+cp "$new"/src/sgml/html/*.css "$new"/src/sgml/html/*.svg "$outdir"
+
+changed=`git diff --no-index --name-only "$old"/src/sgml/html "$new"/src/sgml/html` ||
+ [ $? -eq 1 ]
+
+for f in $changed
+do
+ # Avoid removed files
+ [ -f "$f" ] || continue
+
+ echo "$f" |grep -Ew "$skippages" >/dev/null &&
+ continue
+
+ cp -v "$f" "$outdir"
+done
+
+exit 0
--
2.42.0
[text/x-diff] 0005-html-index-file.patch (2.6K, ../../ZnGNeXbET2EL5zkw@pryzbyj2023/6-0005-html-index-file.patch)
download | inline diff:
From c0dd484060e18c4221e7ec883e621720eec230ed Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email protected]>
Date: Mon, 28 Feb 2022 23:18:19 -0600
Subject: [PATCH 5/7] +html index file
This allows linking to the artifacts from the last successful build.
//freebsd
ci-os-only: warnings
---
src/tools/ci/copy-changed-docs | 39 ++++++++++++++++++++++++++++++++--
1 file changed, 37 insertions(+), 2 deletions(-)
diff --git a/src/tools/ci/copy-changed-docs b/src/tools/ci/copy-changed-docs
index 1c921a8df6f..0efad386cca 100755
--- a/src/tools/ci/copy-changed-docs
+++ b/src/tools/ci/copy-changed-docs
@@ -5,6 +5,7 @@ set -e
old=$1
new=$2
outdir=$3
+branch=$CIRRUS_BRANCH
# The index is large and changes often
skippages="bookindex.html"
@@ -12,6 +13,19 @@ skippages="bookindex.html"
mkdir "$outdir"
cp "$new"/src/sgml/html/*.css "$new"/src/sgml/html/*.svg "$outdir"
+# The index is useful to allow a static link (not specific to a cirrus run) to the artifacts for the most-recent, successful CI run for a branch
+branchurl=https://api.cirrus-ci.com/v1/artifact/github/$CIRRUS_REPO_FULL_NAME/$CIRRUS_TASK_NAME/html_docs/html_docs/00-index.html?branch=$branch
+
+index="$outdir/00-index.html"
+cat >"$index" <<EOF
+<html>
+<head><title>Index of docs changed since: $BASE_COMMIT</title></head>
+<body>
+<!-- A link to documentation for the most recent successful build: $branchurl -->
+<h1>Index of docs changed since: $BASE_COMMIT</h1>
+<ul>
+EOF
+
changed=`git diff --no-index --name-only "$old"/src/sgml/html "$new"/src/sgml/html` ||
[ $? -eq 1 ]
@@ -23,7 +37,28 @@ do
echo "$f" |grep -Ew "$skippages" >/dev/null &&
continue
- cp -v "$f" "$outdir"
-done
+ cp -v "$f" "$outdir" >&2
+ fn=${f##*/}
+ # ?branch=... is needed when accessing the artifacts for the static link for the branch
+ # It's not needed and ignored if accessing artifacts for *this* CI run
+ echo "<li><a href='$fn?branch=$branch'>$fn</a>"
+done >>"$index"
+
+github=https://github.com/$CIRRUS_REPO_FULL_NAME/commit
+cirrus=https://cirrus-ci.com/build
+
+cat >>"$index" <<EOF
+</ul>
+<hr>
+<code>
+<br>This file was written on: `date --rfc-822 --utc`
+<br>CIRRUS_CHANGE_TITLE: $CIRRUS_CHANGE_TITLE
+<br>CIRRUS_CHANGE_IN_REPO: <a href="$github/$CIRRUS_CHANGE_IN_REPO">$CIRRUS_CHANGE_IN_REPO</a>
+<br>CIRRUS_BUILD_ID: <a href="$cirrus/$CIRRUS_BUILD_ID">$CIRRUS_BUILD_ID</a>
+<br>CIRRUS_LAST_GREEN_CHANGE: <a href="$github/$CIRRUS_LAST_GREEN_CHANGE">$CIRRUS_LAST_GREEN_CHANGE</a>
+<br>CIRRUS_LAST_GREEN_BUILD_ID: <a href="$cirrus/$CIRRUS_LAST_GREEN_BUILD_ID">$CIRRUS_LAST_GREEN_BUILD_ID</a>
+</code>
+</body></html>
+EOF
exit 0
--
2.42.0
[text/x-diff] 0006-WIP-show-changed-docs-with-meson.patch (2.5K, ../../ZnGNeXbET2EL5zkw@pryzbyj2023/7-0006-WIP-show-changed-docs-with-meson.patch)
download | inline diff:
From c68c8e6923328a9a2b0fb461a5ecc089e9d47450 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email protected]>
Date: Wed, 19 Oct 2022 20:33:28 -0500
Subject: [PATCH 6/7] WIP: show changed docs with meson
//-os-only:
---
.cirrus.tasks.yml | 26 ++++++++++++++++++++++----
src/tools/ci/copy-changed-docs | 3 ++-
2 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index 0ce5b172331..c53408197d4 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -783,20 +783,38 @@ task:
# XXX: Only do this if there have been changes in doc/ since last build
always:
docs_build_script: |
- time ./configure \
+ mkdir build-autoconf
+ cd build-autoconf
+ time ../configure \
--cache gcc.cache \
+ --without-icu \
CC="ccache gcc" \
CXX="ccache g++" \
CLANG="ccache clang"
make -s -j${BUILD_JOBS} clean
time make -s -j${BUILD_JOBS} -C doc
- cp -r doc new-docs
+ cp -r doc ../new-docs
# Re-build HTML docs from the base commit.
- git checkout "$BASE_COMMIT" -- doc
+ git checkout "$BASE_COMMIT" -- ../doc
make -s -C doc clean
time make -s -C doc html
- cp -r doc old-docs
+ cp -r doc ../old-docs
+
+ # Exercise HTML and other docs:
+ ninja_docs_build_script: |
+ make maintainer-clean # XXX not needed once compiler-warnings is switched meson
+ mkdir build-ninja
+ cd build-ninja
+ time meson setup
+ time ninja docs
+ cp -r doc ../new-docs
+
+ # Re-build HTML docs from the base commit.
+ git checkout "$BASE_COMMIT" -- ../doc
+ ninja clean
+ time ninja doc/src/sgml/html
+ cp -r doc ../old-docs
copy_changed_docs_script:
- src/tools/ci/copy-changed-docs "old-docs" "new-docs" "html_docs"
diff --git a/src/tools/ci/copy-changed-docs b/src/tools/ci/copy-changed-docs
index 0efad386cca..ee3135c298b 100755
--- a/src/tools/ci/copy-changed-docs
+++ b/src/tools/ci/copy-changed-docs
@@ -11,7 +11,8 @@ branch=$CIRRUS_BRANCH
skippages="bookindex.html"
mkdir "$outdir"
-cp "$new"/src/sgml/html/*.css "$new"/src/sgml/html/*.svg "$outdir"
+cp doc/src/sgml/*.css "$outdir"
+cp doc/src/sgml/images/*.svg "$outdir"
# The index is useful to allow a static link (not specific to a cirrus run) to the artifacts for the most-recent, successful CI run for a branch
branchurl=https://api.cirrus-ci.com/v1/artifact/github/$CIRRUS_REPO_FULL_NAME/$CIRRUS_TASK_NAME/html_docs/html_docs/00-index.html?branch=$branch
--
2.42.0
^ permalink raw reply [nested|flat] 28+ messages in thread
end of thread, other threads:[~2024-06-18 13:36 UTC | newest]
Thread overview: 28+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
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 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]>
2024-06-14 15:22 Re: CI and test improvements Justin Pryzby <[email protected]>
2024-06-14 15:34 ` Re: CI and test improvements Andres Freund <[email protected]>
2024-06-18 13:36 ` Re: CI and test improvements Justin Pryzby <[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