Setting restrictedtoken in pg_regress

Started by Daniel Gustafssonover 3 years ago6 messages

daniel@yesql.se

over 3 years ago

1 attachment(s)

In pg_regress we set restrictedToken when calling CreateRestrictedProcess, but
we never seem to use that anywhere. Not being well versed in Windows I might
be missing something, but is it needed or is it a copy/pasteo from fa1e5afa8a2
which does that in restricted_token.c? If not needed, removing it makes the
code more readable IMO.

--
Daniel Gustafsson https://vmware.com/

Attachments:

pg_regress_restrictedtoken.diffapplication/octet-stream; name=pg_regress_restrictedtoken.diff; x-unix-mode=0644Download

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index a803355f8e..3fc9e13b29 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -1104,7 +1104,6 @@ spawn_process(const char *cmdline)
 #else
 	PROCESS_INFORMATION pi;
 	char	   *cmdline2;
-	HANDLE		restrictedToken;
 	const char *comspec;
 
 	/* Find CMD.EXE location using COMSPEC, if it's set */
@@ -1115,8 +1114,7 @@ spawn_process(const char *cmdline)
 	memset(&pi, 0, sizeof(pi));
 	cmdline2 = psprintf("\"%s\" /c \"%s\"", comspec, cmdline);
 
-	if ((restrictedToken =
-		 CreateRestrictedProcess(cmdline2, &pi)) == 0)
+	if (CreateRestrictedProcess(cmdline2, &pi) == 0)
 		exit(2);
 
 	CloseHandle(pi.hThread);

Nathan Bossart

nathandbossart@gmail.com

over 2 years ago

In reply to: Daniel Gustafsson (#1)

Re: Setting restrictedtoken in pg_regress

On Tue, Aug 30, 2022 at 03:02:54PM +0200, Daniel Gustafsson wrote:

In pg_regress we set restrictedToken when calling CreateRestrictedProcess, but
we never seem to use that anywhere. Not being well versed in Windows I might
be missing something, but is it needed or is it a copy/pasteo from fa1e5afa8a2
which does that in restricted_token.c? If not needed, removing it makes the
code more readable IMO.

Looks reasonable to me.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Michael Paquier

michael@paquier.xyz

over 2 years ago

In reply to: Nathan Bossart (#2)

Re: Setting restrictedtoken in pg_regress

On Mon, Jun 12, 2023 at 04:12:22PM -0700, Nathan Bossart wrote:

On Tue, Aug 30, 2022 at 03:02:54PM +0200, Daniel Gustafsson wrote:

In pg_regress we set restrictedToken when calling CreateRestrictedProcess, but
we never seem to use that anywhere. Not being well versed in Windows I might
be missing something, but is it needed or is it a copy/pasteo from fa1e5afa8a2
which does that in restricted_token.c? If not needed, removing it makes the
code more readable IMO.

Looks reasonable to me.

Indeed, looks like a copy-pasto to me.

I am actually a bit confused with the return value of
CreateRestrictedProcess() on failures in restricted_token.c. Wouldn't
it be cleaner to return INVALID_HANDLE_VALUE rather than 0 in these
cases?
--
Michael

Nathan Bossart

nathandbossart@gmail.com

over 2 years ago

In reply to: Michael Paquier (#3)

Re: Setting restrictedtoken in pg_regress

On Tue, Jun 13, 2023 at 08:29:19AM +0900, Michael Paquier wrote:

I am actually a bit confused with the return value of
CreateRestrictedProcess() on failures in restricted_token.c. Wouldn't
it be cleaner to return INVALID_HANDLE_VALUE rather than 0 in these
cases?

My suspicion is that this was chosen to align with CreateProcess and to
allow things like

if (!CreateRestrictedProcess(...))

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Andrew Dunstan

andrew@dunslane.net

over 2 years ago

In reply to: Nathan Bossart (#4)

Re: Setting restrictedtoken in pg_regress

On 2023-06-12 Mo 19:43, Nathan Bossart wrote:

On Tue, Jun 13, 2023 at 08:29:19AM +0900, Michael Paquier wrote:

I am actually a bit confused with the return value of
CreateRestrictedProcess() on failures in restricted_token.c. Wouldn't
it be cleaner to return INVALID_HANDLE_VALUE rather than 0 in these
cases?

My suspicion is that this was chosen to align with CreateProcess and to
allow things like

if (!CreateRestrictedProcess(...))

Probably, it's been a while. I doubt it's worth changing at this point,
and we could just change pg_regress.c to use a boolean test like the above.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Daniel Gustafsson

daniel@yesql.se

over 2 years ago

In reply to: Andrew Dunstan (#5)

Re: Setting restrictedtoken in pg_regress

On 14 Jun 2023, at 13:02, Andrew Dunstan <andrew@dunslane.net> wrote:
On 2023-06-12 Mo 19:43, Nathan Bossart wrote:

On Tue, Jun 13, 2023 at 08:29:19AM +0900, Michael Paquier wrote:

I am actually a bit confused with the return value of
CreateRestrictedProcess() on failures in restricted_token.c. Wouldn't
it be cleaner to return INVALID_HANDLE_VALUE rather than 0 in these
cases?

My suspicion is that this was chosen to align with CreateProcess and to
allow things like

if (!CreateRestrictedProcess(...))

Probably, it's been a while. I doubt it's worth changing at this point, and we could just change pg_regress.c to use a boolean test like the above.

Done that way and pushed, thanks!

--
Daniel Gustafsson