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.
26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ messages in thread

* Re: Support prepared statement invalidation when result types change
@ 2023-08-28 16:47 Jelte Fennema <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Jelte Fennema @ 2023-08-28 16:47 UTC (permalink / raw)
  To: Konstantin Knizhnik <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>

On Mon, 28 Aug 2023 at 15:05, Konstantin Knizhnik <[email protected]> wrote:
> The following assignment of format is not corrects:
>
> It has to be copied as below:
>
>      portal->formats = (int16 *)
>          MemoryContextAlloc(portal->portalContext,
>                             natts * sizeof(int16));
>          memcpy(portal->formats, formats, natts * sizeof(int16));

I attached a new version of the patch where I now did this. But I also
moved the code around quite a bit, since all this tupDesc/format
delaying is only needed for exec_simple_query. The original changes
actually broke some prepared statements that were using protocol level
Bind messages.


Attachments:

  [application/octet-stream] v2-0001-Support-prepared-statement-invalidation-when-resu.patch (7.3K, ../../CAGECzQSNeE5O4jbcyPNU0TVuEHhANQBp0q3iWOxAWjNVi2qnAw@mail.gmail.com/2-v2-0001-Support-prepared-statement-invalidation-when-resu.patch)
  download | inline diff:
From 43da6d5013224ec263490c3d21f471d02204494e Mon Sep 17 00:00:00 2001
From: Jelte Fennema <[email protected]>
Date: Fri, 25 Aug 2023 17:09:38 +0200
Subject: [PATCH v2 1/2] Support prepared statement invalidation when result
 types change

The cached plan for a prepared statements can get invalidated when DDL
changes the tables used in the query, or when search_path changes. When
this happens the prepared statement can still be executed, but it will
be replanned in the new context. This means that the prepared statement
will do something different e.g. in case of search_path changes it will
select data from a completely different table. This won't throw an
error, because it is considered the responsibility of the operator and
query writers that the query will still do the intended thing.

However, we would throw an error if the the result of the query is of a
different type than it was before. This was not documented anywhere and
can thus be a surprising error to hit. But it's actually not needed for
this to be an error, as long as we send the correct RowDescription there
does not have to be a problem for clients when the result types or
column counts change.

This patch starts to allow a prepared statement to continue to work even
when the result type changes.

Without this change all clients that automatically prepare queries as a
performance optimization will need to handle or avoid the error somehow,
often resulting in deallocating and re-preparing queries when its
usually not necessary. With this change connection poolers can also
safely prepare the same query only once on a connection and share this
one prepared query across clients that prepared that exact same query.
---
 src/backend/tcop/postgres.c             | 26 ++++++++++++++++++++++++-
 src/backend/tcop/pquery.c               | 16 +++++++++++++++
 src/backend/utils/cache/plancache.c     |  5 -----
 src/test/regress/expected/plancache.out | 24 +++++++++++++++++------
 src/test/regress/sql/plancache.sql      |  7 +++----
 5 files changed, 62 insertions(+), 16 deletions(-)

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index e4756f8be21..1b160d140aa 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1254,7 +1254,31 @@ exec_simple_query(const char *query_string)
 					format = 1; /* BINARY */
 			}
 		}
-		PortalSetResultFormat(portal, 1, &format);
+
+		if (portal->strategy == PORTAL_UTIL_SELECT)
+		{
+			/*
+			 * For SELECT-like queries we clear the tupDesc now, and it will
+			 * be filled in later by FillPortalStore, because the tupDesc
+			 * might change due to replanning when ExecuteQuery calls
+			 * GetCachedPlan. So we should only fetch the tupDesc after the
+			 * query is actually executed. This also means that we cannot set
+			 * the result format for the output tuple yet, so we temporarily
+			 * store the desired format in portal->formats. Then after
+			 * creating the actual tupDesc we call PortalSetResultFormat,
+			 * using this format.
+			 */
+			FreeTupleDesc(portal->tupDesc);
+			portal->tupDesc = NULL;
+
+			portal->formats = (int16 *) MemoryContextAlloc(portal->portalContext,
+														   sizeof(int16));
+			portal->formats[0] = format;
+		}
+		else
+		{
+			PortalSetResultFormat(portal, 1, &format);
+		}
 
 		/*
 		 * Now we can create the destination receiver object.
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 5565f200c3d..4f7633faf46 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -1030,6 +1030,22 @@ FillPortalStore(Portal portal, bool isTopLevel)
 		case PORTAL_UTIL_SELECT:
 			PortalRunUtility(portal, linitial_node(PlannedStmt, portal->stmts),
 							 isTopLevel, true, treceiver, &qc);
+
+			if (!portal->tupDesc)
+			{
+				/*
+				 * Now fill in the tupDesc and formats fields of the portal.
+				 * We delayed this because for EXECUTE queries we only know
+				 * the tuple after they were executed, because its original
+				 * plan might get replaced with a new one that returns
+				 * different columns, due to the table having changed since
+				 * the last PREPARE/EXECUTE command.
+				 */
+				PlannedStmt *pstmt = PortalGetPrimaryStmt(portal);
+
+				portal->tupDesc = UtilityTupleDescriptor(pstmt->utilityStmt);
+				PortalSetResultFormat(portal, 1, portal->formats);
+			}
 			break;
 
 		default:
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 7d4168f82f5..a6fbe235381 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -717,11 +717,6 @@ RevalidateCachedQuery(CachedPlanSource *plansource,
 	else if (resultDesc == NULL || plansource->resultDesc == NULL ||
 			 !equalTupleDescs(resultDesc, plansource->resultDesc))
 	{
-		/* can we give a better error message? */
-		if (plansource->fixed_result)
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("cached plan must not change result type")));
 		oldcxt = MemoryContextSwitchTo(plansource->context);
 		if (resultDesc)
 			resultDesc = CreateTupleDescCopy(resultDesc);
diff --git a/src/test/regress/expected/plancache.out b/src/test/regress/expected/plancache.out
index 4e59188196c..77ae17b074a 100644
--- a/src/test/regress/expected/plancache.out
+++ b/src/test/regress/expected/plancache.out
@@ -49,14 +49,26 @@ EXECUTE prepstmt2(123);
  123 | 4567890123456789
 (2 rows)
 
--- prepared statements should prevent change in output tupdesc,
--- since clients probably aren't expecting that to change on the fly
-ALTER TABLE pcachetest ADD COLUMN q3 bigint;
+-- prepared statements should work even if the output tupdesc changes
+ALTER TABLE pcachetest ADD COLUMN q3 bigint DEFAULT 20;
 EXECUTE prepstmt;
-ERROR:  cached plan must not change result type
+        q1        |        q2         | q3 
+------------------+-------------------+----
+ 4567890123456789 | -4567890123456789 | 20
+ 4567890123456789 |               123 | 20
+              123 |               456 | 20
+              123 |  4567890123456789 | 20
+ 4567890123456789 |  4567890123456789 | 20
+(5 rows)
+
 EXECUTE prepstmt2(123);
-ERROR:  cached plan must not change result type
--- but we're nice guys and will let you undo your mistake
+ q1  |        q2        | q3 
+-----+------------------+----
+ 123 |              456 | 20
+ 123 | 4567890123456789 | 20
+(2 rows)
+
+-- changing it back should also work fine
 ALTER TABLE pcachetest DROP COLUMN q3;
 EXECUTE prepstmt;
         q1        |        q2         
diff --git a/src/test/regress/sql/plancache.sql b/src/test/regress/sql/plancache.sql
index 4b2f11dcc64..cab0c42a142 100644
--- a/src/test/regress/sql/plancache.sql
+++ b/src/test/regress/sql/plancache.sql
@@ -27,14 +27,13 @@ CREATE TEMP TABLE pcachetest AS SELECT * FROM int8_tbl ORDER BY 2;
 EXECUTE prepstmt;
 EXECUTE prepstmt2(123);
 
--- prepared statements should prevent change in output tupdesc,
--- since clients probably aren't expecting that to change on the fly
-ALTER TABLE pcachetest ADD COLUMN q3 bigint;
+-- prepared statements should work even if the output tupdesc changes
+ALTER TABLE pcachetest ADD COLUMN q3 bigint DEFAULT 20;
 
 EXECUTE prepstmt;
 EXECUTE prepstmt2(123);
 
--- but we're nice guys and will let you undo your mistake
+-- changing it back should also work fine
 ALTER TABLE pcachetest DROP COLUMN q3;
 
 EXECUTE prepstmt;

base-commit: 36e4419d1f1ef06bba58a28a870aaaa8de73bb46
-- 
2.34.1



  [application/octet-stream] v2-0002-Completely-remove-fixed_result-from-CachedPlanSou.patch (7.1K, ../../CAGECzQSNeE5O4jbcyPNU0TVuEHhANQBp0q3iWOxAWjNVi2qnAw@mail.gmail.com/3-v2-0002-Completely-remove-fixed_result-from-CachedPlanSou.patch)
  download | inline diff:
From 9a8d33be7d77b1f4deca430e47eeb9ac22abcce1 Mon Sep 17 00:00:00 2001
From: Jelte Fennema <[email protected]>
Date: Fri, 25 Aug 2023 19:48:24 +0200
Subject: [PATCH v2 2/2] Completely remove fixed_result from CachedPlanSource

Because of the previous patch, no fixed_result plans exist anymore. This
removes all references. It's done in a separate commit to ease
reviewing, if this gets merged we might want to squash them together.
---
 src/backend/commands/prepare.c      | 12 +-----------
 src/backend/executor/spi.c          |  6 ++----
 src/backend/tcop/postgres.c         |  6 +-----
 src/backend/utils/cache/plancache.c |  7 +------
 src/include/utils/plancache.h       |  4 +---
 5 files changed, 6 insertions(+), 29 deletions(-)

diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 18f70319fc4..da3115beb99 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -127,8 +127,7 @@ PrepareQuery(ParseState *pstate, PrepareStmt *stmt,
 					   nargs,
 					   NULL,
 					   NULL,
-					   CURSOR_OPT_PARALLEL_OK,	/* allow parallel mode */
-					   true);	/* fixed result */
+					   CURSOR_OPT_PARALLEL_OK); /* allow parallel mode */
 
 	/*
 	 * Save the results.
@@ -165,10 +164,6 @@ ExecuteQuery(ParseState *pstate,
 	/* Look it up in the hash table */
 	entry = FetchPreparedStatement(stmt->name, true);
 
-	/* Shouldn't find a non-fixed-result cached plan */
-	if (!entry->plansource->fixed_result)
-		elog(ERROR, "EXECUTE does not support variable-result cached plans");
-
 	/* Evaluate parameters, if any */
 	if (entry->plansource->num_params > 0)
 	{
@@ -469,7 +464,6 @@ FetchPreparedStatementResultDesc(PreparedStatement *stmt)
 	 * Since we don't allow prepared statements' result tupdescs to change,
 	 * there's no need to worry about revalidating the cached plan here.
 	 */
-	Assert(stmt->plansource->fixed_result);
 	if (stmt->plansource->resultDesc)
 		return CreateTupleDescCopy(stmt->plansource->resultDesc);
 	else
@@ -591,10 +585,6 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
 	/* Look it up in the hash table */
 	entry = FetchPreparedStatement(execstmt->name, true);
 
-	/* Shouldn't find a non-fixed-result cached plan */
-	if (!entry->plansource->fixed_result)
-		elog(ERROR, "EXPLAIN EXECUTE does not support variable-result cached plans");
-
 	query_string = entry->plansource->query_string;
 
 	/* Evaluate parameters, if any */
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 33975687b38..52128290741 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2285,8 +2285,7 @@ _SPI_prepare_plan(const char *src, SPIPlanPtr plan)
 						   plan->nargs,
 						   plan->parserSetup,
 						   plan->parserSetupArg,
-						   plan->cursor_options,
-						   false);	/* not fixed result */
+						   plan->cursor_options);	/* not fixed result */
 
 		plancache_list = lappend(plancache_list, plansource);
 	}
@@ -2522,8 +2521,7 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
 							   plan->nargs,
 							   plan->parserSetup,
 							   plan->parserSetupArg,
-							   plan->cursor_options,
-							   false);	/* not fixed result */
+							   plan->cursor_options);
 		}
 
 		/*
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 1b160d140aa..c9d2da7e4a7 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1578,8 +1578,7 @@ exec_parse_message(const char *query_string,	/* string to execute */
 					   numParams,
 					   NULL,
 					   NULL,
-					   CURSOR_OPT_PARALLEL_OK,	/* allow parallel mode */
-					   true);	/* fixed result */
+					   CURSOR_OPT_PARALLEL_OK); /* allow parallel mode */
 
 	/* If we got a cancel signal during analysis, quit */
 	CHECK_FOR_INTERRUPTS();
@@ -2653,9 +2652,6 @@ exec_describe_statement_message(const char *stmt_name)
 					 errmsg("unnamed prepared statement does not exist")));
 	}
 
-	/* Prepared statements shouldn't have changeable result descs */
-	Assert(psrc->fixed_result);
-
 	/*
 	 * If we are in aborted transaction state, we can't run
 	 * SendRowDescriptionMessage(), because that needs catalog accesses.
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index a6fbe235381..1106215b781 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -203,7 +203,6 @@ CreateCachedPlan(RawStmt *raw_parse_tree,
 	plansource->parserSetup = NULL;
 	plansource->parserSetupArg = NULL;
 	plansource->cursor_options = 0;
-	plansource->fixed_result = false;
 	plansource->resultDesc = NULL;
 	plansource->context = source_context;
 	plansource->query_list = NIL;
@@ -271,7 +270,6 @@ CreateOneShotCachedPlan(RawStmt *raw_parse_tree,
 	plansource->parserSetup = NULL;
 	plansource->parserSetupArg = NULL;
 	plansource->cursor_options = 0;
-	plansource->fixed_result = false;
 	plansource->resultDesc = NULL;
 	plansource->context = CurrentMemoryContext;
 	plansource->query_list = NIL;
@@ -346,8 +344,7 @@ CompleteCachedPlan(CachedPlanSource *plansource,
 				   int num_params,
 				   ParserSetupHook parserSetup,
 				   void *parserSetupArg,
-				   int cursor_options,
-				   bool fixed_result)
+				   int cursor_options)
 {
 	MemoryContext source_context = plansource->context;
 	MemoryContext oldcxt = CurrentMemoryContext;
@@ -430,7 +427,6 @@ CompleteCachedPlan(CachedPlanSource *plansource,
 	plansource->parserSetup = parserSetup;
 	plansource->parserSetupArg = parserSetupArg;
 	plansource->cursor_options = cursor_options;
-	plansource->fixed_result = fixed_result;
 	plansource->resultDesc = PlanCacheComputeResultDesc(querytree_list);
 
 	MemoryContextSwitchTo(oldcxt);
@@ -1547,7 +1543,6 @@ CopyCachedPlan(CachedPlanSource *plansource)
 	newsource->parserSetup = plansource->parserSetup;
 	newsource->parserSetupArg = plansource->parserSetupArg;
 	newsource->cursor_options = plansource->cursor_options;
-	newsource->fixed_result = plansource->fixed_result;
 	if (plansource->resultDesc)
 		newsource->resultDesc = CreateTupleDescCopy(plansource->resultDesc);
 	else
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index 916e59d9fef..36fd0104aec 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -104,7 +104,6 @@ typedef struct CachedPlanSource
 	ParserSetupHook parserSetup;	/* alternative parameter spec method */
 	void	   *parserSetupArg;
 	int			cursor_options; /* cursor options used for planning */
-	bool		fixed_result;	/* disallow change in result tupdesc? */
 	TupleDesc	resultDesc;		/* result type; NULL = doesn't return tuples */
 	MemoryContext context;		/* memory context holding all above */
 	/* These fields describe the current analyzed-and-rewritten query tree: */
@@ -201,8 +200,7 @@ extern void CompleteCachedPlan(CachedPlanSource *plansource,
 							   int num_params,
 							   ParserSetupHook parserSetup,
 							   void *parserSetupArg,
-							   int cursor_options,
-							   bool fixed_result);
+							   int cursor_options);
 
 extern void SaveCachedPlan(CachedPlanSource *plansource);
 extern void DropCachedPlan(CachedPlanSource *plansource);
-- 
2.34.1



^ permalink  raw  reply  [nested|flat] 26+ messages in thread

end of thread, other threads:[~2023-08-28 16:47 UTC | newest]

Thread overview: 26+ 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 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]>
2023-08-28 16:47 Re: Support prepared statement invalidation when result types change Jelte Fennema <[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