pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedCI/windows docker vs "am a service" autodetection on windows
14+ messages / 5 participants
[nested] [flat]
Commits from this thread:
2021-09-07 76e38b37a5 windows: Only consider us to be running as service if stderr is invalid.
* CI/windows docker vs "am a service" autodetection on windows
@ 2021-03-04 19:08 Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 14+ messages in thread
From: Andres Freund @ 2021-03-04 19:08 UTC (permalink / raw)
To: pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>; Magnus Hagander <magnus@hagander.net>; Andrew Dunstan <andrew@dunslane.net>; Heikki Linnakangas <heikki.linnakangas@iki.fi>
Hi,
When one tests postgres in a some of the popular CI systems (all that
use docker for windows), some of the tests fail in weird ways. Like
https://www.postgresql.org/message-id/20210303052011.ybplxw6q4tafwogk%40alap3.anarazel.de
> t/003_recovery_targets.pl ............ 7/9
> # Failed test 'multiple conflicting settings'
> # at t/003_recovery_targets.pl line 151.
>
> # Failed test 'recovery end before target reached is a fatal error'
> # at t/003_recovery_targets.pl line 177.
> t/003_recovery_targets.pl ............ 9/9 # Looks like you failed 2 tests of 9.
> t/003_recovery_targets.pl ............ Dubious, test returned 2 (wstat 512, 0x200)
> Failed 2/9 subtests
>
> I think it's pretty dangerous if we have a substantial number of tests
> that aren't run on windows - I think a lot of us just assume that the
> BF would catch windows specific problems...
A lot of debugging later I figured out that the problem is that postgres
decides not to write anything to stderr, but send everything to the
windows event log instead. This includes error messages when starting
postgres with wrong parameters or such...
The reason for that elog.c and pg_ctl.c use
src/port/win32security.c:pgwin32_is_service() to detect whether they're
running as a service:
static void
send_message_to_server_log(ErrorData *edata)
...
/*
* In a win32 service environment, there is no usable stderr. Capture
* anything going there and write it to the eventlog instead.
*
* If stderr redirection is active, it was OK to write to stderr above
* because that's really a pipe to the syslogger process.
*/
else if (pgwin32_is_service())
write_eventlog(edata->elevel, buf.data, buf.len);
..
void
write_stderr(const char *fmt,...)
...
/*
* On Win32, we print to stderr if running on a console, or write to
* eventlog if running as a service
*/
if (pgwin32_is_service()) /* Running as a service */
{
write_eventlog(ERROR, errbuf, strlen(errbuf));
but pgwin32_is_service() doesn't actually reliably detect if running as
a service - it's a heuristic that also triggers when running postgres
within a windows docker container (presumably because that itself is run
from within a service?).
ISTM that that's a problem, and is likely to become more of a problem
going forward (assuming that docker on windows will become more
popular).
My opinion is that the whole attempt at guessing whether we are running
as a service is a bad idea. This isn't the first time to be a problem,
see e.g. [1].
Why don't we instead have pgwin32_doRegister() include a parameter that
indicates we're running as a service and remove all the heuristics?
I tried to look around to see if there's a simple way to drop the
problematic memberships that trigger pgwin32_is_service() - but there
seem to be no commandline tools doing so (but there are C APIs).
Does anybody have an alternative way of fixing this?
Greetings,
Andres Freund
[1]
commit ff30aec759bdc4de78912d91f650ec8fd95ff6bc
Author: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: 2017-03-17 11:14:01 +0200
Fix and simplify check for whether we're running as Windows service.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: CI/windows docker vs "am a service" autodetection on windows
@ 2021-03-04 19:33 Andrew Dunstan <andrew@dunslane.net>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Dunstan @ 2021-03-04 19:33 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>; Magnus Hagander <magnus@hagander.net>; Heikki Linnakangas <heikki.linnakangas@iki.fi>
On 3/4/21 2:08 PM, Andres Freund wrote:
> [...] pgwin32_is_service() doesn't actually reliably detect if running as
> a service - it's a heuristic that also triggers when running postgres
> within a windows docker container (presumably because that itself is run
> from within a service?).
>
>
> ISTM that that's a problem, and is likely to become more of a problem
> going forward (assuming that docker on windows will become more
> popular).
>
>
> My opinion is that the whole attempt at guessing whether we are running
> as a service is a bad idea. This isn't the first time to be a problem,
> see e.g. [1].
>
> Why don't we instead have pgwin32_doRegister() include a parameter that
> indicates we're running as a service and remove all the heuristics?
I assume you mean a postmaster parameter, that would be set via pg_ctl?
Seems reasonable.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: CI/windows docker vs "am a service" autodetection on windows
@ 2021-03-04 20:08 Magnus Hagander <magnus@hagander.net>
parent: Andrew Dunstan <andrew@dunslane.net>
0 siblings, 1 reply; 14+ messages in thread
From: Magnus Hagander @ 2021-03-04 20:08 UTC (permalink / raw)
To: Andrew Dunstan <andrew@dunslane.net>; +Cc: Andres Freund <andres@anarazel.de>; pgsql-hackers; Thomas Munro <thomas.munro@gmail.com>; Heikki Linnakangas <heikki.linnakangas@iki.fi>
On Thu, Mar 4, 2021 at 8:33 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 3/4/21 2:08 PM, Andres Freund wrote:
> > [...] pgwin32_is_service() doesn't actually reliably detect if running as
> > a service - it's a heuristic that also triggers when running postgres
> > within a windows docker container (presumably because that itself is run
> > from within a service?).
> >
> >
> > ISTM that that's a problem, and is likely to become more of a problem
> > going forward (assuming that docker on windows will become more
> > popular).
> >
> >
> > My opinion is that the whole attempt at guessing whether we are running
> > as a service is a bad idea. This isn't the first time to be a problem,
> > see e.g. [1].
> >
> > Why don't we instead have pgwin32_doRegister() include a parameter that
> > indicates we're running as a service and remove all the heuristics?
>
>
>
> I assume you mean a postmaster parameter, that would be set via pg_ctl?
> Seems reasonable.
The problem with doing it at register time is that everybody who
builds an installer for PostgreSQL will then have to do it in their
own registration (I'm pretty sure most of them don't use pg_ctl
register).
The same thing in pgwin32_doRunAsService() might help with that. But
then we'd have to figure out what to do if pg_ctl fails prior to
reaching that point... There aren't that many such paths, but there
are some.
Just throwing out ideas without spending time thinking about it, maybe
log to *both* in the case when we pick by it by autodetection?
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: CI/windows docker vs "am a service" autodetection on windows
@ 2021-03-04 20:30 Andres Freund <andres@anarazel.de>
parent: Magnus Hagander <magnus@hagander.net>
0 siblings, 1 reply; 14+ messages in thread
From: Andres Freund @ 2021-03-04 20:30 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; Dave Page <dpage@pgadmin.org>; +Cc: Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers; Thomas Munro <thomas.munro@gmail.com>; Heikki Linnakangas <heikki.linnakangas@iki.fi>
Hi,
On 2021-03-04 21:08:30 +0100, Magnus Hagander wrote:
> The problem with doing it at register time is that everybody who
> builds an installer for PostgreSQL will then have to do it in their
> own registration (I'm pretty sure most of them don't use pg_ctl
> register).
Well, hm, maybe they should change that?
> The same thing in pgwin32_doRunAsService() might help with that.
What do you mean by this?
> But then we'd have to figure out what to do if pg_ctl fails prior to
> reaching that point... There aren't that many such paths, but there
> are some.
>
> Just throwing out ideas without spending time thinking about it, maybe
> log to *both* in the case when we pick by it by autodetection?
I think that's a good answer for pg_ctl - not so sure about postgres
itself, at least once it's up and running. I don't know what lead to all
of this autodetection stuff, but is there the possibility of blocking on
whatever stderr is set too as a service?
Perhaps we could make the service detection more reliable by checking
whether stderr is actually something useful?
There does seem to be isatty(), so we could improve the case of
pg_ctl/postgres run interactively without breaking a sweat. And there is
fstat() too, so if stderr in a service is something distinguishable...
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: CI/windows docker vs "am a service" autodetection on windows
@ 2021-03-04 20:36 Magnus Hagander <magnus@hagander.net>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 14+ messages in thread
From: Magnus Hagander @ 2021-03-04 20:36 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Dave Page <dpage@pgadmin.org>; Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers; Thomas Munro <thomas.munro@gmail.com>; Heikki Linnakangas <heikki.linnakangas@iki.fi>
On Thu, Mar 4, 2021 at 9:30 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2021-03-04 21:08:30 +0100, Magnus Hagander wrote:
> > The problem with doing it at register time is that everybody who
> > builds an installer for PostgreSQL will then have to do it in their
> > own registration (I'm pretty sure most of them don't use pg_ctl
> > register).
>
> Well, hm, maybe they should change that?
>
>
> > The same thing in pgwin32_doRunAsService() might help with that.
>
> What do you mean by this?
I mean controlling this flag by entry into pgwin32_doRunAsService().
So that when we start *postgres* we pass a parameter along saying that
it's a service and should use eventlog for the early exit.
pgwin32_doRunAsService() will (of course) only be called when started
with runservice. That would, I think, sort out the problem for the
postgres processes, and leave us with just pg_ctl to figure out.
> > But then we'd have to figure out what to do if pg_ctl fails prior to
> > reaching that point... There aren't that many such paths, but there
> > are some.
> >
> > Just throwing out ideas without spending time thinking about it, maybe
> > log to *both* in the case when we pick by it by autodetection?
>
> I think that's a good answer for pg_ctl - not so sure about postgres
> itself, at least once it's up and running. I don't know what lead to all
> of this autodetection stuff, but is there the possibility of blocking on
> whatever stderr is set too as a service?
>
> Perhaps we could make the service detection more reliable by checking
> whether stderr is actually something useful?
So IIRC, and mind that this is like 15 years ago, there is something
that looks like stderr, but the contents are thrown away. It probably
exists specifically so that programs won't crash when run as a
service...
> There does seem to be isatty(), so we could improve the case of
> pg_ctl/postgres run interactively without breaking a sweat. And there is
> fstat() too, so if stderr in a service is something distinguishable...
We seem to have used that at some point, but commit
a967613911f7ef7b6387b9e8718f0ab8f0c4d9c8 got rid of it... But maybe
apply it in a combination.
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: CI/windows docker vs "am a service" autodetection on windows
@ 2021-03-04 20:48 Andres Freund <andres@anarazel.de>
parent: Magnus Hagander <magnus@hagander.net>
0 siblings, 2 replies; 14+ messages in thread
From: Andres Freund @ 2021-03-04 20:48 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; Alvaro Herrera <alvherre@alvh.no-ip.org>; Michael Paquier <michael@paquier.xyz>; +Cc: Dave Page <dpage@pgadmin.org>; Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers; Thomas Munro <thomas.munro@gmail.com>; Heikki Linnakangas <heikki.linnakangas@iki.fi>
Hi,
On 2021-03-04 21:36:23 +0100, Magnus Hagander wrote:
> > I think that's a good answer for pg_ctl - not so sure about postgres
> > itself, at least once it's up and running. I don't know what lead to all
> > of this autodetection stuff, but is there the possibility of blocking on
> > whatever stderr is set too as a service?
> >
> > Perhaps we could make the service detection more reliable by checking
> > whether stderr is actually something useful?
>
> So IIRC, and mind that this is like 15 years ago, there is something
> that looks like stderr, but the contents are thrown away. It probably
> exists specifically so that programs won't crash when run as a
> service...
Yea, that'd make sense.
I wish we had tests for the service stuff, but that's from long before
there were tap tests...
> > There does seem to be isatty(), so we could improve the case of
> > pg_ctl/postgres run interactively without breaking a sweat. And there is
> > fstat() too, so if stderr in a service is something distinguishable...
>
> We seem to have used that at some point, but commit
> a967613911f7ef7b6387b9e8718f0ab8f0c4d9c8 got rid of it...
Hm. The bug #13592 referenced in that commit appears to be about
something else. Looks to be #13594
https://postgr.es/m/20150828104658.2089.83265%40wrigleys.postgresql.org
> But maybe apply it in a combination.
Yea, that's what I was thinking.
Gah, I don't really want to know anything about windows, I just want to
hack on aio with proper working CI.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: CI/windows docker vs "am a service" autodetection on windows
@ 2021-03-04 21:04 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 0 replies; 14+ messages in thread
From: Alvaro Herrera @ 2021-03-04 21:04 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Magnus Hagander <magnus@hagander.net>; Michael Paquier <michael@paquier.xyz>; Dave Page <dpage@pgadmin.org>; Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers; Thomas Munro <thomas.munro@gmail.com>; Heikki Linnakangas <heikki.linnakangas@iki.fi>
On 2021-Mar-04, Andres Freund wrote:
> > > There does seem to be isatty(), so we could improve the case of
> > > pg_ctl/postgres run interactively without breaking a sweat. And there is
> > > fstat() too, so if stderr in a service is something distinguishable...
> >
> > We seem to have used that at some point, but commit
> > a967613911f7ef7b6387b9e8718f0ab8f0c4d9c8 got rid of it...
>
> Hm. The bug #13592 referenced in that commit appears to be about
> something else. Looks to be #13594
> https://postgr.es/m/20150828104658.2089.83265%40wrigleys.postgresql.org
Yeah, that's a typo in the commit message.
> > But maybe apply it in a combination.
>
> Yea, that's what I was thinking.
That makes sense. At the time we were not thinking (*I* was not
thinking, for sure) that you could have a not-a-service process that
runs inside a service. The fixed bug was in the same direction that you
want to fix now, just differently: the bare "isatty" test was
considering too many things as under a service, and replaced it with the
pgwin32_is_service which considers a different set of too many things as
under a service. I agree with the idea that *both* tests have to pass
in order to consider it as under a service.
--
Álvaro Herrera 39°49'30"S 73°17'W
"No hay hombre que no aspire a la plenitud, es decir,
la suma de experiencias de que un hombre es capaz"
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: CI/windows docker vs "am a service" autodetection on windows
@ 2021-03-05 18:57 Andres Freund <andres@anarazel.de>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 14+ messages in thread
From: Andres Freund @ 2021-03-05 18:57 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; Alvaro Herrera <alvherre@alvh.no-ip.org>; Michael Paquier <michael@paquier.xyz>; +Cc: Dave Page <dpage@pgadmin.org>; Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers; Thomas Munro <thomas.munro@gmail.com>; Heikki Linnakangas <heikki.linnakangas@iki.fi>
Hi,
On 2021-03-04 12:48:59 -0800, Andres Freund wrote:
> On 2021-03-04 21:36:23 +0100, Magnus Hagander wrote:
> > > I think that's a good answer for pg_ctl - not so sure about postgres
> > > itself, at least once it's up and running. I don't know what lead to all
> > > of this autodetection stuff, but is there the possibility of blocking on
> > > whatever stderr is set too as a service?
> > >
> > > Perhaps we could make the service detection more reliable by checking
> > > whether stderr is actually something useful?
> >
> > So IIRC, and mind that this is like 15 years ago, there is something
> > that looks like stderr, but the contents are thrown away. It probably
> > exists specifically so that programs won't crash when run as a
> > service...
>
> Yea, that'd make sense.
>
> I wish we had tests for the service stuff, but that's from long before
> there were tap tests...
After fighting with a windows VM for a bit (ugh), it turns out that yes,
there is stderr, but that fileno(stderr) returns -2, and
GetStdHandle(STD_ERROR_HANDLE) returns NULL (not INVALID_HANDLE_VALUE).
The complexity however is that while that's true for pg_ctl within
pgwin32_ServiceMain:
checking what stderr=00007FF8687DFCB0 is (handle: 0, fileno=-2)
but not for postmaster or backends
WARNING: 01000: checking what stderr=00007FF880F5FCB0 is (handle: 92, fileno=2)
which makes sense in a way, because we don't tell CreateProcessAsUser()
that it should pass stdin/out/err down (which then seems to magically
get access to the "topmost" console applications output - damn, this
stuff is weird).
You'd earlier mentioned that other distributions may not use pg_ctl
register - but I assume they use pg_ctl runservice? Or do they actually
re-implement all those magic incantations in pg_ctl.c?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: CI/windows docker vs "am a service" autodetection on windows
@ 2021-03-05 20:55 Andres Freund <andres@anarazel.de>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 14+ messages in thread
From: Andres Freund @ 2021-03-05 20:55 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; Alvaro Herrera <alvherre@alvh.no-ip.org>; Michael Paquier <michael@paquier.xyz>; +Cc: Dave Page <dpage@pgadmin.org>; Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers; Thomas Munro <thomas.munro@gmail.com>; Heikki Linnakangas <heikki.linnakangas@iki.fi>
Hi,
On 2021-03-05 10:57:52 -0800, Andres Freund wrote:
> On 2021-03-04 12:48:59 -0800, Andres Freund wrote:
> > On 2021-03-04 21:36:23 +0100, Magnus Hagander wrote:
> > > > I think that's a good answer for pg_ctl - not so sure about postgres
> > > > itself, at least once it's up and running. I don't know what lead to all
> > > > of this autodetection stuff, but is there the possibility of blocking on
> > > > whatever stderr is set too as a service?
> > > >
> > > > Perhaps we could make the service detection more reliable by checking
> > > > whether stderr is actually something useful?
> > >
> > > So IIRC, and mind that this is like 15 years ago, there is something
> > > that looks like stderr, but the contents are thrown away. It probably
> > > exists specifically so that programs won't crash when run as a
> > > service...
> >
> > Yea, that'd make sense.
> >
> > I wish we had tests for the service stuff, but that's from long before
> > there were tap tests...
>
> After fighting with a windows VM for a bit (ugh), it turns out that yes,
> there is stderr, but that fileno(stderr) returns -2, and
> GetStdHandle(STD_ERROR_HANDLE) returns NULL (not INVALID_HANDLE_VALUE).
>
> The complexity however is that while that's true for pg_ctl within
> pgwin32_ServiceMain:
> checking what stderr=00007FF8687DFCB0 is (handle: 0, fileno=-2)
> but not for postmaster or backends
> WARNING: 01000: checking what stderr=00007FF880F5FCB0 is (handle: 92, fileno=2)
>
> which makes sense in a way, because we don't tell CreateProcessAsUser()
> that it should pass stdin/out/err down (which then seems to magically
> get access to the "topmost" console applications output - damn, this
> stuff is weird).
That part is not too hard to address - it seems we only need to do that
in pg_ctl pgwin32_doRunAsService(). It seems that the
stdin/stderr/stdout being set to invalid will then be passed down to
postmaster children.
https://docs.microsoft.com/en-us/windows/console/getstdhandle
"If an application does not have associated standard handles, such as a
service running on an interactive desktop, and has not redirected them,
the return value is NULL."
There does seem to be some difference between what services get as std*
- GetStdHandle() returns NULL, and what explicitly passing down invalid
handles to postmaster does - GetStdHandle() returns
INVALID_HANDLE_VALUE. But passing down NULL rather than
INVALID_HANDLE_VALUE to postmaster seems to lead to postmaster
re-opening console buffers.
Patch attached.
I'd like to commit something to address this issue to master soon - it
allows us to run a lot more tests in cirrus-ci... But probably not
backpatch it [yet] - there've not really been field complaints, and who
knows if there end up being some unintentional side-effects...
> You'd earlier mentioned that other distributions may not use pg_ctl
> register - but I assume they use pg_ctl runservice? Or do they actually
> re-implement all those magic incantations in pg_ctl.c?
It seems that we, in addition to the above patch, should add a guc that
pg_ctl runservice passes down to postgres. And then rip out the call to
pgwin32_is_service() from the backend. That doesn't require other
distributions to use pg_ctl register, just pg_ctl runservice - which I
think they need to do anyway, unless they want to duplicate all the
logic around pgwin32_SetServiceStatus()?
Greetings,
Andres Freund
Attachments:
v2-0001-windows-Only-consider-us-to-be-running-as-service.patch (text/x-diff, 4.3K) download
From 86fb1f3da2db85d513f491d88b638ea7cc1327b8 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 2 Mar 2021 21:24:11 -0800
Subject: [PATCH v2] windows: Only consider us to be running as service if
stderr is invalid.
Previously pgwin32_is_service() would falsely return true when
postgres is started from somewhere within a service, but not as a
service. That is e.g. always the case with windows docker containers,
which some CI services use to run windows tests in.
In addition to this change, it likely would be a good idea to have
pg_ctl runservice pass down a flag indicating that postgres is running
as a service.
Author: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20210305185752.3up5eq2eanb7ofmb@alap3.anarazel.de
---
src/port/win32security.c | 13 +++++++++++--
src/bin/pg_ctl/pg_ctl.c | 33 +++++++++++++++++++++++++++++++++
2 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/src/port/win32security.c b/src/port/win32security.c
index 4a673fde19a..b57ce61d752 100644
--- a/src/port/win32security.c
+++ b/src/port/win32security.c
@@ -95,8 +95,9 @@ pgwin32_is_admin(void)
* We consider ourselves running as a service if one of the following is
* true:
*
- * 1) We are running as LocalSystem (only used by services)
- * 2) Our token contains SECURITY_SERVICE_RID (automatically added to the
+ * 1) Standard error is not valid (always the case for services)
+ * 2) We are running as LocalSystem (only used by services)
+ * 3) Our token contains SECURITY_SERVICE_RID (automatically added to the
* process token by the SCM when starting a service)
*
* The check for LocalSystem is needed, because surprisingly, if a service
@@ -121,11 +122,19 @@ pgwin32_is_service(void)
PSID ServiceSid;
PSID LocalSystemSid;
SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};
+ HANDLE stderr_handle;
/* Only check the first time */
if (_is_service != -1)
return _is_service;
+ stderr_handle = GetStdHandle(STD_ERROR_HANDLE);
+ if (stderr_handle != INVALID_HANDLE_VALUE && stderr_handle != NULL)
+ {
+ _is_service = 0;
+ return _is_service;
+ }
+
/* First check for LocalSystem */
if (!AllocateAndInitializeSid(&NtAuthority, 1,
SECURITY_LOCAL_SYSTEM_RID, 0, 0, 0, 0, 0, 0, 0,
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 7985da0a943..c99e3c507de 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -1737,6 +1737,31 @@ typedef BOOL (WINAPI * __SetInformationJobObject) (HANDLE, JOBOBJECTINFOCLASS, L
typedef BOOL (WINAPI * __AssignProcessToJobObject) (HANDLE, HANDLE);
typedef BOOL (WINAPI * __QueryInformationJobObject) (HANDLE, JOBOBJECTINFOCLASS, LPVOID, DWORD, LPDWORD);
+/*
+ * Set up STARTUPINFO for the new process to inherit this process' handles.
+ *
+ * Process started as services appear to have "empty" handles (GetStdHandle()
+ * returns NULL) rather than invalid ones. But passing down NULL ourselves
+ * doesn't work, it's interpreted as STARTUPINFO->hStd* not being set. But we
+ * can pass down INVALID_HANDLE_VALUE - which makes GetStdHandle() in the new
+ * process (and its child processes!) return INVALID_HANDLE_VALUE. Which
+ * achieves the goal of postmaster running in a similar environment as pg_ctl.
+ */
+static void
+InheritStdHandles(STARTUPINFO* si)
+{
+ si->dwFlags |= STARTF_USESTDHANDLES;
+ si->hStdInput = GetStdHandle(STD_INPUT_HANDLE);
+ if (si->hStdInput == NULL)
+ si->hStdInput = INVALID_HANDLE_VALUE;
+ si->hStdOutput = GetStdHandle(STD_OUTPUT_HANDLE);
+ if (si->hStdOutput == NULL)
+ si->hStdOutput = INVALID_HANDLE_VALUE;
+ si->hStdError = GetStdHandle(STD_ERROR_HANDLE);
+ if (si->hStdError == NULL)
+ si->hStdError = INVALID_HANDLE_VALUE;
+}
+
/*
* Create a restricted token, a job object sandbox, and execute the specified
* process with it.
@@ -1774,6 +1799,14 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_ser
ZeroMemory(&si, sizeof(si));
si.cb = sizeof(si);
+ /*
+ * Set stdin/stdout/stderr handles to be inherited in the child
+ * process. That allows postmaster and the processes it starts to perform
+ * additional checks to see if running in a service (otherwise they get
+ * the default console handles - which point to "somewhere").
+ */
+ InheritStdHandles(&si);
+
Advapi32Handle = LoadLibrary("ADVAPI32.DLL");
if (Advapi32Handle != NULL)
{
--
2.29.2.540.g3cf59784d4
* Re: CI/windows docker vs "am a service" autodetection on windows
@ 2021-08-13 13:02 Andres Freund <andres@anarazel.de>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 2 replies; 14+ messages in thread
From: Andres Freund @ 2021-08-13 13:02 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; Alvaro Herrera <alvherre@alvh.no-ip.org>; Michael Paquier <michael@paquier.xyz>; +Cc: Dave Page <dpage@pgadmin.org>; Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers; Thomas Munro <thomas.munro@gmail.com>; Heikki Linnakangas <heikki.linnakangas@iki.fi>
Hi,
Magnus, Michael, Anyone - I'd appreciate a look.
On 2021-03-05 12:55:37 -0800, Andres Freund wrote:
> > After fighting with a windows VM for a bit (ugh), it turns out that yes,
> > there is stderr, but that fileno(stderr) returns -2, and
> > GetStdHandle(STD_ERROR_HANDLE) returns NULL (not INVALID_HANDLE_VALUE).
> >
> > The complexity however is that while that's true for pg_ctl within
> > pgwin32_ServiceMain:
> > checking what stderr=00007FF8687DFCB0 is (handle: 0, fileno=-2)
> > but not for postmaster or backends
> > WARNING: 01000: checking what stderr=00007FF880F5FCB0 is (handle: 92, fileno=2)
> >
> > which makes sense in a way, because we don't tell CreateProcessAsUser()
> > that it should pass stdin/out/err down (which then seems to magically
> > get access to the "topmost" console applications output - damn, this
> > stuff is weird).
>
> That part is not too hard to address - it seems we only need to do that
> in pg_ctl pgwin32_doRunAsService(). It seems that the
> stdin/stderr/stdout being set to invalid will then be passed down to
> postmaster children.
>
> https://docs.microsoft.com/en-us/windows/console/getstdhandle
> "If an application does not have associated standard handles, such as a
> service running on an interactive desktop, and has not redirected them,
> the return value is NULL."
>
> There does seem to be some difference between what services get as std*
> - GetStdHandle() returns NULL, and what explicitly passing down invalid
> handles to postmaster does - GetStdHandle() returns
> INVALID_HANDLE_VALUE. But passing down NULL rather than
> INVALID_HANDLE_VALUE to postmaster seems to lead to postmaster
> re-opening console buffers.
>
> Patch attached.
> I'd like to commit something to address this issue to master soon - it
> allows us to run a lot more tests in cirrus-ci... But probably not
> backpatch it [yet] - there've not really been field complaints, and who
> knows if there end up being some unintentional side-effects...
Because it'd allow us to run more tests as part of cfbot and other CI
efforts, I'd like to push this forward. So I'm planning to commit this
to master soon-ish, unless somebody wants to take this over? I'm really
not a windows person...
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: CI/windows docker vs "am a service" autodetection on windows
@ 2021-08-15 14:25 Ranier Vilela <ranier.vf@gmail.com>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 14+ messages in thread
From: Ranier Vilela @ 2021-08-15 14:25 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Magnus Hagander <magnus@hagander.net>; Alvaro Herrera <alvherre@alvh.no-ip.org>; Michael Paquier <michael@paquier.xyz>; Dave Page <dpage@pgadmin.org>; Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers; Thomas Munro <thomas.munro@gmail.com>; Heikki Linnakangas <heikki.linnakangas@iki.fi>
Em sex., 13 de ago. de 2021 às 10:03, Andres Freund <andres@anarazel.de>
escreveu:
> Hi,
>
> Magnus, Michael, Anyone - I'd appreciate a look.
>
> On 2021-03-05 12:55:37 -0800, Andres Freund wrote:
> > > After fighting with a windows VM for a bit (ugh), it turns out that
> yes,
> > > there is stderr, but that fileno(stderr) returns -2, and
> > > GetStdHandle(STD_ERROR_HANDLE) returns NULL (not INVALID_HANDLE_VALUE).
> > >
> > > The complexity however is that while that's true for pg_ctl within
> > > pgwin32_ServiceMain:
> > > checking what stderr=00007FF8687DFCB0 is (handle: 0, fileno=-2)
> > > but not for postmaster or backends
> > > WARNING: 01000: checking what stderr=00007FF880F5FCB0 is (handle: 92,
> fileno=2)
> > >
> > > which makes sense in a way, because we don't tell CreateProcessAsUser()
> > > that it should pass stdin/out/err down (which then seems to magically
> > > get access to the "topmost" console applications output - damn, this
> > > stuff is weird).
> >
> > That part is not too hard to address - it seems we only need to do that
> > in pg_ctl pgwin32_doRunAsService(). It seems that the
> > stdin/stderr/stdout being set to invalid will then be passed down to
> > postmaster children.
> >
> > https://docs.microsoft.com/en-us/windows/console/getstdhandle
> > "If an application does not have associated standard handles, such as a
> > service running on an interactive desktop, and has not redirected them,
> > the return value is NULL."
> >
> > There does seem to be some difference between what services get as std*
> > - GetStdHandle() returns NULL, and what explicitly passing down invalid
> > handles to postmaster does - GetStdHandle() returns
> > INVALID_HANDLE_VALUE. But passing down NULL rather than
> > INVALID_HANDLE_VALUE to postmaster seems to lead to postmaster
> > re-opening console buffers.
> >
> > Patch attached.
>
> > I'd like to commit something to address this issue to master soon - it
> > allows us to run a lot more tests in cirrus-ci... But probably not
> > backpatch it [yet] - there've not really been field complaints, and who
> > knows if there end up being some unintentional side-effects...
>
> Because it'd allow us to run more tests as part of cfbot and other CI
> efforts, I'd like to push this forward. So I'm planning to commit this
> to master soon-ish, unless somebody wants to take this over? I'm really
> not a windows person...
>
Hi Andres,
I found this function on the web, from OpenSSL, but I haven't tested it.
I think that there is one more way to test if a service is running
(SECURITY_INTERACTIVE_RID).
Can you test on a Windows VM?
If this works I can elaborate a bit.
Attached.
regards,
Ranier Vilela
Attachments:
pgwin32_is_service.c (text/plain, 3.7K) download
int
pgwin32_is_service(void)
{
static int _is_service = -1;
HANDLE hProcessToken = NULL;
DWORD groupLength = 64;
PTOKEN_GROUPS groupInfo;
SID_IDENTIFIER_AUTHORITY siaNt = SECURITY_NT_AUTHORITY;
PSID InteractiveSid = NULL;
PSID ServiceSid = NULL;
DWORD i;
/* Only check the first time */
if (_is_service != -1)
return _is_service;
_is_service = 0;
groupInfo = (PTOKEN_GROUPS) LocalAlloc(0, groupLength);
if (groupInfo == NULL)
{
fprintf(stderr, "could not get SID for local system account\n");
goto clean;
}
if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &hProcessToken))
{
fprintf(stderr, "could not get SID for local system account\n");
goto clean;
}
if (!GetTokenInformation(hProcessToken, TokenGroups, groupInfo,
groupLength, &groupLength))
{
if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)
{
fprintf(stderr, "could not get SID for local system account\n");
goto clean;
}
LocalFree(groupInfo);
groupInfo = NULL;
groupInfo = (PTOKEN_GROUPS) LocalAlloc(0, groupLength);
if (groupInfo == NULL)
{
fprintf(stderr, "could not get SID for local system account\n");
goto clean;
}
if (!GetTokenInformation(hProcessToken, TokenGroups, groupInfo,
groupLength, &groupLength))
{
fprintf(stderr, "could not get SID for local system account\n");
goto clean;
}
}
//
// We now know the groups associated with this token. We want to look to see if
// the interactive group is active in the token, and if so, we know that
// this is an interactive process.
//
// We also look for the "service" SID, and if it's present, we know we're a service.
//
// The service SID will be present iff the service is running in a
// user account (and was invoked by the service controller).
//
if (!AllocateAndInitializeSid(&siaNt, 1, SECURITY_INTERACTIVE_RID, 0, 0,
0, 0, 0, 0, 0, &InteractiveSid))
{
fprintf(stderr, "could not get SID for local system account\n");
goto clean;
}
if (!AllocateAndInitializeSid(&siaNt, 1, SECURITY_SERVICE_RID, 0, 0, 0,
0, 0, 0, 0, &ServiceSid))
{
fprintf(stderr, "could not get SID for local system account\n");
goto clean;
}
for (i = 0; i < groupInfo->GroupCount; i += 1)
{
SID_AND_ATTRIBUTES sanda = groupInfo->Groups[i];
PSID Sid = sanda.Sid;
//
// Check to see if the group we're looking at is one of
// the 2 groups we're interested in.
//
if (EqualSid(Sid, InteractiveSid))
{
//
// This process has the Interactive SID in its
// token. This means that the process is running as
// an EXE.
//
goto clean;
}
else if (EqualSid(Sid, ServiceSid))
{
//
// This process has the Service SID in its
// token. This means that the process is running as
// a service running in a user account.
//
_is_service = 1;
goto clean;
}
}
//
// Neither Interactive or Service was present in the current users token,
// This implies that the process is running as a service, most likely
// running as LocalSystem.
//
_is_service = 1;
clean:
if (InteractiveSid)
FreeSid(InteractiveSid);
if (ServiceSid)
FreeSid(ServiceSid);
if (groupInfo)
LocalFree(groupInfo);
if (hProcessToken)
CloseHandle(hProcessToken);
return _is_service;
}
* Re: CI/windows docker vs "am a service" autodetection on windows
@ 2021-08-16 13:34 Magnus Hagander <magnus@hagander.net>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 14+ messages in thread
From: Magnus Hagander @ 2021-08-16 13:34 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; Michael Paquier <michael@paquier.xyz>; Dave Page <dpage@pgadmin.org>; Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers; Thomas Munro <thomas.munro@gmail.com>; Heikki Linnakangas <heikki.linnakangas@iki.fi>
On Fri, Aug 13, 2021 at 3:03 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> Magnus, Michael, Anyone - I'd appreciate a look.
>
> On 2021-03-05 12:55:37 -0800, Andres Freund wrote:
> > > After fighting with a windows VM for a bit (ugh), it turns out that yes,
> > > there is stderr, but that fileno(stderr) returns -2, and
> > > GetStdHandle(STD_ERROR_HANDLE) returns NULL (not INVALID_HANDLE_VALUE).
> > >
> > > The complexity however is that while that's true for pg_ctl within
> > > pgwin32_ServiceMain:
> > > checking what stderr=00007FF8687DFCB0 is (handle: 0, fileno=-2)
> > > but not for postmaster or backends
> > > WARNING: 01000: checking what stderr=00007FF880F5FCB0 is (handle: 92, fileno=2)
> > >
> > > which makes sense in a way, because we don't tell CreateProcessAsUser()
> > > that it should pass stdin/out/err down (which then seems to magically
> > > get access to the "topmost" console applications output - damn, this
> > > stuff is weird).
> >
> > That part is not too hard to address - it seems we only need to do that
> > in pg_ctl pgwin32_doRunAsService(). It seems that the
> > stdin/stderr/stdout being set to invalid will then be passed down to
> > postmaster children.
> >
> > https://docs.microsoft.com/en-us/windows/console/getstdhandle
> > "If an application does not have associated standard handles, such as a
> > service running on an interactive desktop, and has not redirected them,
> > the return value is NULL."
> >
> > There does seem to be some difference between what services get as std*
> > - GetStdHandle() returns NULL, and what explicitly passing down invalid
> > handles to postmaster does - GetStdHandle() returns
> > INVALID_HANDLE_VALUE. But passing down NULL rather than
> > INVALID_HANDLE_VALUE to postmaster seems to lead to postmaster
> > re-opening console buffers.
> >
> > Patch attached.
>
> > I'd like to commit something to address this issue to master soon - it
> > allows us to run a lot more tests in cirrus-ci... But probably not
> > backpatch it [yet] - there've not really been field complaints, and who
> > knows if there end up being some unintentional side-effects...
>
> Because it'd allow us to run more tests as part of cfbot and other CI
> efforts, I'd like to push this forward. So I'm planning to commit this
> to master soon-ish, unless somebody wants to take this over? I'm really
> not a windows person...
It certainly sounds reasonable. It does make me wonder why we didn't
use that GetStdHandle in the first place -- mostly in that "did we try
that and it didn't work", but that was long enough ago that I really
can't remember, and I am unable to find any references in my mail
history either. So it may very well just be that we missed it. But
give the number of times we've had issues around this, it makes me
wonder. It could of course also be something that didn't use to be
reliable but us now -- the world of Windows has changed a lot since
that was written.
It wouldn't surprise me if it does break some *other* weird
cornercase, but based on the docs page you linked to it doesn't look
like it would break any of the normal/standard usecases. But I'm also
very much not a Windows person these days, and most of my knowledge on
the API side is quite outdated by now -- so I can only base that on
reading the same manual page you did...
Gaining better testability definitely seems worth it, so I think an
approach of "push to master and see what explodes" is reasonable :)
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: CI/windows docker vs "am a service" autodetection on windows
@ 2021-09-07 18:49 Andres Freund <andres@anarazel.de>
parent: Ranier Vilela <ranier.vf@gmail.com>
0 siblings, 0 replies; 14+ messages in thread
From: Andres Freund @ 2021-09-07 18:49 UTC (permalink / raw)
To: Ranier Vilela <ranier.vf@gmail.com>; +Cc: Magnus Hagander <magnus@hagander.net>; Alvaro Herrera <alvherre@alvh.no-ip.org>; Michael Paquier <michael@paquier.xyz>; Dave Page <dpage@pgadmin.org>; Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers; Thomas Munro <thomas.munro@gmail.com>; Heikki Linnakangas <heikki.linnakangas@iki.fi>
Hi,
On 2021-08-15 11:25:01 -0300, Ranier Vilela wrote:
> I found this function on the web, from OpenSSL, but I haven't tested it.
> I think that there is one more way to test if a service is running
> (SECURITY_INTERACTIVE_RID).
I don't think that really addresses the issue. If a service starts postgres
somewhere within, it won't have SECURITY_INTERACTIVE_RID, but postgres isn't
quite running as a service nevertheless. I think that's the situation in the
CI case that triggered me to look into this.
Regards,
Andres
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: CI/windows docker vs "am a service" autodetection on windows
@ 2021-09-07 19:03 Andres Freund <andres@anarazel.de>
parent: Magnus Hagander <magnus@hagander.net>
0 siblings, 0 replies; 14+ messages in thread
From: Andres Freund @ 2021-09-07 19:03 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; Michael Paquier <michael@paquier.xyz>; Dave Page <dpage@pgadmin.org>; Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers; Thomas Munro <thomas.munro@gmail.com>; Heikki Linnakangas <heikki.linnakangas@iki.fi>
Hi,
On 2021-08-16 15:34:51 +0200, Magnus Hagander wrote:
> It wouldn't surprise me if it does break some *other* weird
> cornercase, but based on the docs page you linked to it doesn't look
> like it would break any of the normal/standard usecases.
Yea, me neither...
I do suspect that it'd have been better to have a --windows-service flag to
postgres. But we can't easily change the past...
> Gaining better testability definitely seems worth it, so I think an
> approach of "push to master and see what explodes" is reasonable :)
Done.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2021-09-07 19:03 UTC | newest]
Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-03-04 19:08 CI/windows docker vs "am a service" autodetection on windows Andres Freund <andres@anarazel.de>
2021-03-04 19:33 ` Andrew Dunstan <andrew@dunslane.net>
2021-03-04 20:08 ` Magnus Hagander <magnus@hagander.net>
2021-03-04 20:30 ` Andres Freund <andres@anarazel.de>
2021-03-04 20:36 ` Magnus Hagander <magnus@hagander.net>
2021-03-04 20:48 ` Andres Freund <andres@anarazel.de>
2021-03-04 21:04 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-03-05 18:57 ` Andres Freund <andres@anarazel.de>
2021-03-05 20:55 ` Andres Freund <andres@anarazel.de>
2021-08-13 13:02 ` Andres Freund <andres@anarazel.de>
2021-08-15 14:25 ` Ranier Vilela <ranier.vf@gmail.com>
2021-09-07 18:49 ` Andres Freund <andres@anarazel.de>
2021-08-16 13:34 ` Magnus Hagander <magnus@hagander.net>
2021-09-07 19:03 ` Andres Freund <andres@anarazel.de>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox