Re: Re: BUG #5065: pg_ctl start fails as administrator, with "could not locate matching postgres executable"

The following bug has been logged online:

Bug reference: 5065
Logged by: Jesse Morris
Email address: jmorris(at)coverity(dot)com
PostgreSQL version: 8.3.7, 8.4.1
Operating system: Windows Server 2003 R2
Description: pg_ctl start fails as administrator, with "could not
locate matching postgres executable"
Details:

I am logged in as domain\jmorris, a member of the local Administrators
group.

I am not running postgres as a service.

I can reproduce this with the zip file from enterprisedb for 8.3.7, or
8.4.1. I also used msys/mingw to build my own (to instrument debugging
output) and that reproduces it as well.

>From cmd.exe:
initdb.exe works fine.
pg_ctl start complains "FATAL: postgres - could not locate matching postgres
executable"

Instrumentation and investigation reveals that the failure is in
find_other_exec (exec.c) as the error text implies, but ultimately in
pipe_read_line; CreatePipe fails with error 5 (Access Denied).

HOWEVER!
If I run pg_ctl from cygwin (or a cmd process that has cygwin or cygstart or
anything cygwinish as a parent) it works as expected.

Almost certainly related: AddUserToDacl (also in exec.c) has comments
explaining that what it does is necessary because otherwise on newer windows
we'll see access denied on CreatePipe. However AddUserToDacl appears to be
working as written; none of the error paths are visited anyway.

I can reproduce this on a number of separate win2k3 R2 SP2 boxes, including
one VM that is basically a fresh OS install.

So: Two bugs:

* pg_ctl's manages to shoot itself in the foot when relinquishing
administrator rights (unless there's cygwin?)

* the error-reporting for this is extremely misleading.

On Sep 18, 7:31 pm, jmor(dot)(dot)(dot)(at)coverity(dot)com ("Jesse Morris") wrote:
> The following bug has been logged online:
>
> Bug reference:      5065
> Logged by:          Jesse Morris
> Email address:      jmor(dot)(dot)(dot)(at)coverity(dot)com
> PostgreSQL version: 8.3.7, 8.4.1
> Operating system:   Windows Server 2003 R2
> Description:        pg_ctl start fails as administrator, with "could not
> locate matching postgres executable"
> Details:
>
> I am logged in as domain\jmorris, a member of the local Administrators
> group.
>
...
>
> From cmd.exe:
> initdb.exe works fine.
> pg_ctl start complains "FATAL: postgres - could not locate matching postgres
> executable"
>
> Instrumentation and investigation reveals that the failure is in
> find_other_exec (exec.c) as the error text implies, but ultimately in
> pipe_read_line; CreatePipe fails with error 5 (Access Denied).  
...

I went back to the version that supposedly initially fixed this issue,
but I couldn't get it to work either. So I think the DACL adjustment
code was always broken. The DACL stuff that both Cygwin and Active
Perl use to simulate *nix file permissions masks this error, so any
test framework that uses them would get false negatives on this bug.
Since these DACLs are inheritable, a workaround is to run pg_ctl as a
child process of Active Perl or Cygwin. The comments indicated
pg_ctl & initdb were already trying to do the same thing themselves
(that is, add the current user to the DACLs) but it didn't actually
work on any of the systems I tried it on.

I think that a number of other people have seen this bug; search for
"FATAL: postgres - could not locate matching postgres executable."
But that message is so misleading is probably why it seems nobody has
properly diagnosed it as a permissions issue before. I didn't do
anything to fix pg_ctl's error reporting. :D

The patch:

------begin patch------

diff -rup unfixed/postgresql-8.4.1/src/bin/initdb/initdb.c fixed/
postgresql-8.4.1/src/bin/initdb/initdb.c
--- unfixed/postgresql-8.4.1/src/bin/initdb/initdb.c 2009-06-11
07:49:07.000000000 -0700
+++ fixed/postgresql-8.4.1/src/bin/initdb/initdb.c 2009-10-15
16:31:12.651226900 -0700
@@ -2392,6 +2392,10 @@ CreateRestrictedProcess(char *cmd, PROCE
fprintf(stderr, "Failed to create restricted token: %lu\n",
GetLastError());
return 0;
}
+
+#ifndef __CYGWIN__
+ AddUserToTokenDacl(restrictedToken);
+#endif

if (!CreateProcessAsUser(restrictedToken,
NULL,
@@ -2409,11 +2413,7 @@ CreateRestrictedProcess(char *cmd, PROCE
fprintf(stderr, "CreateProcessAsUser failed: %lu\n", GetLastError
());
return 0;
}
-
-#ifndef __CYGWIN__
- AddUserToDacl(processInfo->hProcess);
-#endif
-
+
return ResumeThread(processInfo->hThread);
}
#endif
Only in fixed/postgresql-8.4.1/src/bin/initdb: initdb.c.bak
diff -rup unfixed/postgresql-8.4.1/src/bin/pg_ctl/pg_ctl.c fixed/
postgresql-8.4.1/src/bin/pg_ctl/pg_ctl.c
--- unfixed/postgresql-8.4.1/src/bin/pg_ctl/pg_ctl.c 2009-09-01
19:40:59.000000000 -0700
+++ fixed/postgresql-8.4.1/src/bin/pg_ctl/pg_ctl.c 2009-10-15
16:31:00.096971600 -0700
@@ -1389,7 +1389,10 @@ CreateRestrictedProcess(char *cmd, PROCE
write_stderr("Failed to create restricted token: %lu\n",
GetLastError());
return 0;
}
-
+#ifndef __CYGWIN__
+ AddUserToTokenDacl(restrictedToken);
+#endif
+
r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL,
TRUE, CREATE_SUSPENDED, NULL, NULL, &si, processInfo);

Kernel32Handle = LoadLibrary("KERNEL32.DLL");
@@ -1488,9 +1491,6 @@ CreateRestrictedProcess(char *cmd, PROCE
}
}

-#ifndef __CYGWIN__
- AddUserToDacl(processInfo->hProcess);
-#endif

CloseHandle(restrictedToken);

Only in fixed/postgresql-8.4.1/src/bin/pg_ctl: pg_ctl.c.bak
diff -rup unfixed/postgresql-8.4.1/src/include/port.h fixed/
postgresql-8.4.1/src/include/port.h
--- unfixed/postgresql-8.4.1/src/include/port.h 2009-06-11
07:49:08.000000000 -0700
+++ fixed/postgresql-8.4.1/src/include/port.h 2009-10-15
14:02:36.860635900 -0700
@@ -81,7 +81,7 @@ extern int find_other_exec(const char *a

/* Windows security token manipulation (in exec.c) */
#ifdef WIN32
-extern BOOL AddUserToDacl(HANDLE hProcess);
+extern BOOL AddUserToTokenDacl(HANDLE hToken);
#endif

diff -rup unfixed/postgresql-8.4.1/src/port/exec.c fixed/
postgresql-8.4.1/src/port/exec.c
--- unfixed/postgresql-8.4.1/src/port/exec.c 2009-06-11
07:49:15.000000000 -0700
+++ fixed/postgresql-8.4.1/src/port/exec.c 2009-10-15
16:02:04.352805300 -0700
@@ -664,11 +664,10 @@ set_pglocale_pgservice(const char *argv0
#ifdef WIN32

/*
- * AddUserToDacl(HANDLE hProcess)
+ * AddUserToTokenDacl(HANDLE hToken)
*
- * This function adds the current user account to the default DACL
- * which gets attached to the restricted token used when we create
- * a restricted process.
+ * This function adds the current user account to the restricted
+ * token used when we create a restricted process.
*
* This is required because of some security changes in Windows
* that appeared in patches to XP/2K3 and in Vista/2008.
@@ -681,13 +680,13 @@ set_pglocale_pgservice(const char *argv0
* and CreateProcess() calls when running as Administrator.
*
* This function fixes this problem by modifying the DACL of the
- * specified process and explicitly re-adding the current user
account.
- * This is still secure because the Administrator account inherits
it's
- * privileges from the Administrators group - it doesn't have any of
- * it's own.
+ * token the process will use, and explicitly re-adding the current
+ * user account. This is still secure because the Administrator
account
+ * inherits it's privileges from the Administrators group - it
doesn't
+ * have any of its own.
*/
BOOL
-AddUserToDacl(HANDLE hProcess)
+AddUserToTokenDacl(HANDLE hToken)
{
int i;
ACL_SIZE_INFORMATION asi;
@@ -695,7 +694,6 @@ AddUserToDacl(HANDLE hProcess)
DWORD dwNewAclSize;
DWORD dwSize = 0;
DWORD dwTokenInfoLength = 0;
- HANDLE hToken = NULL;
PACL pacl = NULL;
PSID psidUser = NULL;
TOKEN_DEFAULT_DACL tddNew;
@@ -703,13 +701,6 @@ AddUserToDacl(HANDLE hProcess)
TOKEN_INFORMATION_CLASS tic = TokenDefaultDacl;
BOOL ret = FALSE;

- /* Get the token for the process */
- if (!OpenProcessToken(hProcess, TOKEN_QUERY | TOKEN_ADJUST_DEFAULT,
&hToken))
- {
- log_error("could not open process token: %lu", GetLastError());
- goto cleanup;
- }
-
/* Figure out the buffer size for the DACL info */
if (!GetTokenInformation(hToken, tic, (LPVOID) NULL,
dwTokenInfoLength, &dwSize))
{
@@ -785,8 +776,8 @@ AddUserToDacl(HANDLE hProcess)
}

/* Add the new ACE for the current user */
- if (!AddAccessAllowedAce(pacl, ACL_REVISION, GENERIC_ALL, psidUser))
- {
+ if (!AddAccessAllowedAceEx(pacl, ACL_REVISION, OBJECT_INHERIT_ACE,
GENERIC_ALL, psidUser))
+ {
log_error("could not add access allowed ACE: %lu", GetLastError());
goto cleanup;
}
@@ -812,9 +803,6 @@ cleanup:
if (ptdd)
LocalFree((HLOCAL) ptdd);

- if (hToken)
- CloseHandle(hToken);
-
return ret;
}

diff -rup unfixed/postgresql-8.4.1/src/test/regress/pg_regress.c fixed/
postgresql-8.4.1/src/test/regress/pg_regress.c
--- unfixed/postgresql-8.4.1/src/test/regress/pg_regress.c 2009-06-11
07:49:15.000000000 -0700
+++ fixed/postgresql-8.4.1/src/test/regress/pg_regress.c 2009-10-15
14:03:51.609110000 -0700
@@ -1021,6 +1021,10 @@ spawn_process(const char *cmdline)
cmdline2 = malloc(strlen(cmdline) + 8);
sprintf(cmdline2, "cmd /c %s", cmdline);

+#ifndef __CYGWIN__
+ AddUserToTokenDacl(restrictedToken);
+#endif
+
if (!CreateProcessAsUser(restrictedToken,
NULL,
cmdline2,
@@ -1038,10 +1042,6 @@ spawn_process(const char *cmdline)
exit_nicely(2);
}

-#ifndef __CYGWIN__
- AddUserToDacl(pi.hProcess);
-#endif
-
free(cmdline2);

ResumeThread(pi.hThread);
------end patch------

On Fri, Oct 16, 2009 at 1:08 AM, Jesse Morris <jmorris(at)coverity(dot)com> wrote:

> I went back to the version that supposedly initially fixed this issue,
> but I couldn't get it to work either.  So I think the DACL adjustment
> code was always broken.  The DACL stuff that both Cygwin and Active
> Perl use to simulate *nix file permissions masks this error, so any
> test framework that uses them would get false negatives on this bug.
> Since these DACLs are inheritable, a workaround is to run pg_ctl as a
> child process of Active Perl or Cygwin.   The comments indicated
> pg_ctl & initdb were already trying to do the same thing themselves
> (that is, add the current user to the DACLs) but it didn't actually
> work on any of the systems I tried it on.

In fairness it wasn't entirely broken as it fixed the problem for the
majority of people. Clearly there's room for improvement though as we
do still see the problem occasionally.

> I think that a number of other people have seen this bug; search for
> "FATAL: postgres - could not locate matching postgres executable."
> But that message is so misleading is probably why it seems nobody has
> properly diagnosed it as a permissions issue before.  I didn't do
> anything to fix pg_ctl's error reporting.  :D

We figured out that it was a permissions problem after a *lot* of back
and forth with Microsoft's developer support engineers - that's what
led to the AddUserToDacl fix. What didn't then happen was further
investigation into the handful of later reports of similar issues -
unfortunately that can be extremely difficult as few users are
experienced Windows programmers. It's great to see someone that is has
seen the problem and taken the time to figure it out :-).

> The patch:
>
> ------begin patch------

:-(. Unfortunately inlining the patch in the email has munged it
beyond usability. Can you resend it as an attachment please?

Thanks!

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com

> -----Original Message-----
> From: Dave Page [mailto:dpage(at)pgadmin(dot)org]
> Sent: Friday, October 16, 2009 2:14 AM
> To: Jesse Morris
> Cc: pgsql-bugs(at)postgresql(dot)org
> Subject: Re: [BUGS] Re: BUG #5065: pg_ctl start fails as
administrator,
> with "could not locate matching postgres executable"
>
> > The patch:
> >
> > ------begin patch------
>
> :-(. Unfortunately inlining the patch in the email has munged it
> beyond usability. Can you resend it as an attachment please?

Oops! Re-sent, as an attachment.

On Fri, Oct 16, 2009 at 7:03 PM, Jesse Morris <jmorris(at)coverity(dot)com> wrote:
>> -----Original Message-----
>> From: Dave Page [mailto:dpage(at)pgadmin(dot)org]
>> Sent: Friday, October 16, 2009 2:14 AM
>> To: Jesse Morris
>> Cc: pgsql-bugs(at)postgresql(dot)org
>> Subject: Re: [BUGS] Re: BUG #5065: pg_ctl start fails as
> administrator,
>> with "could not locate matching postgres executable"
>>
>> > The patch:
>> >
>> > ------begin patch------
>>
>> :-(. Unfortunately inlining the patch in the email has munged it
>> beyond usability. Can you resend it as an attachment please?
>
> Oops!  Re-sent, as an attachment.

Thanks. I've had a play with this, and it seems to work fine in 8.4.1
- at least, it doesn't seem to cause any regression that I can see
when testing in Vista or XP. I cannot reproduce the problem since I
wrote the original fix though, so I cannot confirm that this fixes any
new cases; we'll have to take your word for that :-)

The code around this has changed a little on -head. I don't have any
more spare cycles at the moment - are you able to produce an updated
patch for 8.5?

Andrew/Magnus; we do still see occasional failures of this nature, so
I believe there is still an issue here. Can we look at getting this
backpatched for 8.3.whatever and 8.4.2, assuming it looks good to you
as well?

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com

Dave Page wrote:
> On Fri, Oct 16, 2009 at 7:03 PM, Jesse Morris <jmorris(at)coverity(dot)com> wrote:
>
>>> -----Original Message-----
>>> From: Dave Page [mailto:dpage(at)pgadmin(dot)org]
>>> Sent: Friday, October 16, 2009 2:14 AM
>>> To: Jesse Morris
>>> Cc: pgsql-bugs(at)postgresql(dot)org
>>> Subject: Re: [BUGS] Re: BUG #5065: pg_ctl start fails as
>>>
>> administrator,
>>
>>> with "could not locate matching postgres executable"
>>>
>>>
>>>> The patch:
>>>>
>>>> ------begin patch------
>>>>
>>> :-(. Unfortunately inlining the patch in the email has munged it
>>> beyond usability. Can you resend it as an attachment please?
>>>
>> Oops! Re-sent, as an attachment.
>>
>
> Thanks. I've had a play with this, and it seems to work fine in 8.4.1
> - at least, it doesn't seem to cause any regression that I can see
> when testing in Vista or XP. I cannot reproduce the problem since I
> wrote the original fix though, so I cannot confirm that this fixes any
> new cases; we'll have to take your word for that :-)
>
> The code around this has changed a little on -head. I don't have any
> more spare cycles at the moment - are you able to produce an updated
> patch for 8.5?
>
> Andrew/Magnus; we do still see occasional failures of this nature, so
> I believe there is still an issue here. Can we look at getting this
> backpatched for 8.3.whatever and 8.4.2, assuming it looks good to you
> as well?
>
>

It looks OK to me (modulo the incorrect changing of "its" to "it's" in a
comment - whoever did that was trying to make it consistent, but
unfortunately made it consistently wrong).

However, I'd like a bit more comment added on just why doing this is
safe. Would it still be safe if someone granted some dangerous privilege
directly to the Administrator user, if that's possible?

cheers

andrew

Andrew,

First the really important stuff: I (correctly) changed the comment
from "it's" to "its" in one place, but didn't notice the other.

As to "is doing this safe?" I'm not sure whether you are asking about my
patch, or about the general approach that I made the slight tweaks to,
but it's probably worth talking about both. I'm going to start with
describing how I understand what's going on. If my understanding of the
windows security model is wrong, then my patch might be flawed. If my
understanding of the windows security model is right, then the general
approach which I made minor changes to might be flawed.

How a thread / process interacts with the system is determined by its
security token. This token belongs to a user owner and a list of
groups. The token also has an list of rights (e.g.,
"SeTakeOwnershipPrivilege"), and a default DACL. The default DACL is
like a umask; new objects the process creates will have that DACL.

Postmaster (that's what I'm going to call postgres.exe) checks that its
token does not belong to the Administrators or Power Users groups, and
refuses to run if it is.

pg_ctl/initdb both run postmaster with a restricted token that disables
the Administrators and Power User groups. Postmaster is satisfied with
this.

However, by default on Win2k3 (and possibly elsewhere?), Administrators'
processes' default DACLs only grants access to {System, Administrators}.
That's the whole list. Postmaster spawns a child postmaster, and they
create a pipe to communicate. But the postmaster is not owned by System
and pg_ctl disabled its Administrators membership, so that pipe is not
accessible: Access denied.

On the Vista and Windows 7 machines I've looked at there's also a Login
SID (S-1-5-5-0-<rand_num>) in both the token's group membership and in
the default DACL. In that case both parent and child process can access
the created pipe and there are no problems. Cygwin, ActivePerl, and
msys all add the user that owns the process to the default DACL, so if
you run pg_ctl with one of those above you in the process tree, it will
work fine.

The code I changed was trying to add the user to the DACL. It doesn't
grant the postmaster processes any additional inherent rights, it just
says "your children processes are all accessible by the creating user
(which also has full access to the database cluster on disk anyway). I
didn't write it though; I just changed how it was doing that. I was
never able to get the original code to work at all, though Dave Page
says he can't even reproduce the failure, so I don't know what we're
doing differently. MSDN says there's a "SeAssignPrimaryTokenPrivilege"
which is required to replace a process-level token, so maybe that's why
it's wasn't working for me - but I wasn't seeing any win32 calls fail,
so I dunno.

Now: To answer your question of "What if the Administrator user is
granted additional permissions?"

The answer is the same with and without my patch. If postgres is run as
a user that has additional permissions, postgres will have additional
permissions. It is unusual to do this though; usually that is all
managed through group membership. "Administrator" is not special in
this regard; I can give the "guest" account full control over everything
if I want.

If you do want to defend against that (personally I'm indifferent), it
wouldn't be difficult to drop unnecessary permissions from the
restricted token that the daemon is already being run with. Postmaster
can verify that anything "dangerous" is not allowed from the backend
startup check. Actually, I'm not sure if *any* of these are necessary:
http://msdn.microsoft.com/en-us/library/bb530716%28VS.85%29.aspx

This wouldn't be a substitute for disabling the Administrators and Power
Users group membership; those still affect a lot of important files.

I personally don't need postgres to be more defensive, I just need it to
start successfully. So even if the above IS a good idea, someone else
will have to implement it.

And by all means, include whatever of this is correct/helpful in code
comments. I don't have to submit a separate patch for that, do I?

-Jesse

> -----Original Message-----
> From: Andrew Dunstan [mailto:andrew(at)dunslane(dot)net]
> Sent: Monday, October 19, 2009 11:03 AM
> To: Dave Page
> Cc: Jesse Morris; pgsql-bugs(at)postgresql(dot)org; Magnus Hagander
> Subject: Re: [BUGS] Re: BUG #5065: pg_ctl start fails as
administrator,
> with "could not locate matching postgres executable"
>
> It looks OK to me (modulo the incorrect changing of "its" to "it's" in
> a comment - whoever did that was trying to make it consistent, but
> unfortunately made it consistently wrong).
>
> However, I'd like a bit more comment added on just why doing this is
> safe. Would it still be safe if someone granted some dangerous
> privilege directly to the Administrator user, if that's possible?
>
> cheers
>
> andrew

On Mon, Oct 19, 2009 at 7:03 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> However, I'd like a bit more comment added on just why doing this is safe.

The patch doesn't change what the code aims to do, only the way it
does it. The existing code does this:

- Creates a restricted security token
- Creates a new (suspended) process using that token
- Adds an ACE for the current user to the DACL for the new process
- Resumes (un-suspends) the process

The patch changes that to:

- Creates a restricted security token
- Adds an ACE for the current user to the DACL for the new token
- Creates a new (suspended) process using that token
- Resumes (un-suspends) the process

The net result /should/ be the same, but the second method is
apparently a little more robust.

> Would it still be safe if someone granted some dangerous privilege directly
> to the Administrator user, if that's possible?

The patch doesn't change that at all, but yes, I believe it is safe
because we drop all privileges when we create the restricted token,
and we then grant access (by adding an ACE) for the user using the
GENERIC_ALL flag, which (AIUI) just gives GENERIC_READ, GENERIC_WRITE
and GENERIC_EXECUTE privileges, and *not* any of the 'standard' or
'specific' rights (which include the more important/dangerous things
like DACL write access).

See:

http://msdn.microsoft.com/en-us/library/aa374892%28VS.85%29.aspx
http://msdn.microsoft.com/en-us/library/aa374951%28VS.85%29.aspx
http://msdn.microsoft.com/en-us/library/aa446583%28VS.85%29.aspx

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
PGDay.EU 2009 Conference: http://2009.pgday.eu/start

Dave Page <dpage(at)pgadmin(dot)org> writes:
> The patch doesn't change what the code aims to do, only the way it
> does it. The existing code does this:
> ...
> The net result /should/ be the same, but the second method is
> apparently a little more robust.

Do we have any idea why? I am always distrustful of random changes made
with no theory as to why they fix a problem. My experience is that such
changes are almost always wrong, once you find out what the problem
*really* is.

regards, tom lane

On Tue, Oct 20, 2009 at 3:48 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dave Page <dpage(at)pgadmin(dot)org> writes:
>> The patch doesn't change what the code aims to do, only the way it
>> does it. The existing code does this:
>> ...
>> The net result /should/ be the same, but the second method is
>> apparently a little more robust.
>
> Do we have any idea why?  I am always distrustful of random changes made
> with no theory as to why they fix a problem.  My experience is that such
> changes are almost always wrong, once you find out what the problem
> *really* is.

Honestly? No. I have a vague hand-wavy idea about there being
something preventing us properly modifying the token of an existing
process in some configurations, but nothing even remotely jello-like,
let alone concrete.

On the other hand, I don't see any obvious way for this to cause a
regression - which was born out by my (limited) testing in which the
original problem remained fixed with the new patch. I'd certainly feel
happier if Magnus took a look as well though.

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
PGDay.EU 2009 Conference: http://2009.pgday.eu/start

Dave Page <dpage(at)pgadmin(dot)org> writes:
> On Tue, Oct 20, 2009 at 3:48 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Do we have any idea why?

> Honestly? No. I have a vague hand-wavy idea about there being
> something preventing us properly modifying the token of an existing
> process in some configurations, but nothing even remotely jello-like,
> let alone concrete.

After re-reading the thread I am struck by Jesse's comment that the
current coding never worked at all for him. It seems like that must
indicate an environment difference or installed-software difference
compared to the setups where it does work. (Antivirus maybe?)

Seems like it would be worth the trouble to identify exactly what the
critical difference is.

regards, tom lane

I've seen it happen on a wide range of win2k3 servers. From a fresh
image with nothing but VMWare tools under Add/Remove Programs, to a
heavily used shared dev server with MSVC, Cygwin, and tons of other
stuff. Some of our beta testers have also seen the issue. In fact I
can't really think of any win2k3 machine where it *had* worked. I
suspect the main reason reports of this failure are relatively rare is
that most people use the one-click installer which sets it up to run as
a non-administrator service account. (That's superior in many ways, but
it is not always an option for me.)

It looks like something that happens with Win2k3 only; XP had the user
instead of Administrators in the DACL, and Vista appears to have the
Session. I guess that's the only place that "AddUserToDacl" was even
necessary in the first place? I dunno.

> -----Original Message-----
> From: Tom Lane [mailto:tgl(at)sss(dot)pgh(dot)pa(dot)us]
> Sent: Tuesday, October 20, 2009 9:45 AM
> To: Dave Page
> Cc: Andrew Dunstan; Jesse Morris; pgsql-bugs(at)postgresql(dot)org; Magnus
> Hagander
> Subject: Re: [BUGS] Re: BUG #5065: pg_ctl start fails as
administrator,
> with "could not locate matching postgres executable"
>
> Dave Page <dpage(at)pgadmin(dot)org> writes:
> > On Tue, Oct 20, 2009 at 3:48 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Do we have any idea why?
>
> > Honestly? No. I have a vague hand-wavy idea about there being
> > something preventing us properly modifying the token of an existing
> > process in some configurations, but nothing even remotely
jello-like,
> > let alone concrete.
>
> After re-reading the thread I am struck by Jesse's comment that the
> current coding never worked at all for him. It seems like that must
> indicate an environment difference or installed-software difference
> compared to the setups where it does work. (Antivirus maybe?)
>
> Seems like it would be worth the trouble to identify exactly what the
> critical difference is.
>
> regards, tom lane

On Tue, Oct 20, 2009 at 5:45 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Seems like it would be worth the trouble to identify exactly what the
> critical difference is.

Given Jesse's description of the systems he's seen this on, I suspect
we'll be very lucky if we pin that down, unless it is something as
simple as win2k3+administrator+command line (which I'll try to test
shortly) - but that doesn't tally with the occasional cases of this I
still hear about (usually from our poker-playing users) who are using
the installer and Vista or XP as a general rule.

The other thing to bear in mind is that the entire OS and much of the
surrounding software (such as AV) is closed source, so we have no idea
what any of it is doing under the hood, or how it may have changed
from version to version.

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
PGDay.EU 2009 Conference: http://2009.pgday.eu/start

On Wed, Oct 21, 2009 at 8:45 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> On Tue, Oct 20, 2009 at 5:45 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> Seems like it would be worth the trouble to identify exactly what the
>> critical difference is.
>
> Given Jesse's description of the systems he's seen this on, I suspect
> we'll be very lucky if we pin that down, unless it is something as
> simple as win2k3+administrator+command line (which I'll try to test
> shortly) - but that doesn't tally with the occasional cases of this I
> still hear about (usually from our poker-playing users) who are using
> the installer and Vista or XP as a general rule.

I tested in 2k3, and was surprised to see that whilst initdb worked
fine in 8.4.1 (that's where the problem traditionally shows up),
starting the server failed:

C:\pgsql>bin\pg_ctl -D data start
server starting

C:\pgsql>FATAL: XX000: C:/pgsql/bin/postgres.exe: could not locate
matching postgres executable
LOCATION: getInstallationPaths, .\src\backend\postmaster\postmaster.c:1070

Jesse's patched version works as it should. This makes it clear that
the re-factoring of the way the security token is created and the
process started is not the really important part of this patch - this
is:

- if (!AddAccessAllowedAce(pacl, ACL_REVISION, GENERIC_ALL, psidUser))
- {
+ if (!AddAccessAllowedAceEx(pacl, ACL_REVISION, OBJECT_INHERIT_ACE,
GENERIC_ALL, psidUser))
+ {

This function call is adding the new access control entry to the DACL,
and in Jesse's modified version it's specifying that the ACE should be
inheritable.

The other refactoring is still important however - without it, I still
see the error. I would guess that you cannot add an inheritable ACE
once the process has been created, but I cannot say for certain.

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
PGDay.EU 2009 Conference: http://2009.pgday.eu/start

Dave Page <dpage(at)pgadmin(dot)org> writes:
> This function call is adding the new access control entry to the DACL,
> and in Jesse's modified version it's specifying that the ACE should be
> inheritable.

Hah, so now we have a theory.

> The other refactoring is still important however - without it, I still
> see the error. I would guess that you cannot add an inheritable ACE
> once the process has been created, but I cannot say for certain.

That would make sense if the state gets copied to the process at time
of creation. Okay, I'm satisfied that this is a believable patch...

regards, tom lane

On Wed, Oct 21, 2009 at 15:07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dave Page <dpage(at)pgadmin(dot)org> writes:
>> This function call is adding the new access control entry to the DACL,
>> and in Jesse's modified version it's specifying that the ACE should be
>> inheritable.
>
> Hah, so now we have a theory.
>
>> The other refactoring is still important however - without it, I still
>> see the error. I would guess that you cannot add an inheritable ACE
>> once the process has been created, but I cannot say for certain.
>
> That would make sense if the state gets copied to the process at time
> of creation.  Okay, I'm satisfied that this is a believable patch...

From a quick look, it looks fine to me. I don't have time to do a
complete check right now, but I'll do that as soon as I can and then
commit it - unless people feel it's more urgent than maybe a week
worst case, in which case someone else has to pick it up :-)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander wrote:

> From a quick look, it looks fine to me. I don't have time to do a
> complete check right now, but I'll do that as soon as I can and then
> commit it - unless people feel it's more urgent than maybe a week
> worst case, in which case someone else has to pick it up :-)

Are we going to do an 8.4.2 release soon?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Magnus Hagander wrote:
>> From a quick look, it looks fine to me. I don't have time to do a
>> complete check right now, but I'll do that as soon as I can and then
>> commit it - unless people feel it's more urgent than maybe a week
>> worst case, in which case someone else has to pick it up :-)

> Are we going to do an 8.4.2 release soon?

There's been no discussion of one, so it's not happening in less than
a week. I'm happy to wait for Magnus on this --- an extra set of
eyeballs can't hurt, especially on something that (I assume) we're
going to back-patch.

regards, tom lane

Magnus Hagander wrote:
> From a quick look, it looks fine to me. I don't have time to do a
> complete check right now, but I'll do that as soon as I can and then
> commit it - unless people feel it's more urgent than maybe a week
> worst case, in which case someone else has to pick it up :-)
>
>
>

I'd rather wait till you can check it.

cheers

andrew

On Wed, Oct 21, 2009 at 15:34, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> Magnus Hagander wrote:
>>
>> From a quick look, it looks fine to me. I don't have time to do a
>> complete check right now, but I'll do that as soon as I can and then
>> commit it - unless people feel it's more urgent than maybe a week
>> worst case, in which case someone else has to pick it up :-)
>>
>>
>>
>
> I'd rather wait till you can check it.

Apologies for the delay.

I've finally looked it over, and it still looks fine. I've cleaned up
some of the whitespace issues in the patch and applied it, including a
backpatch to 8.3 and a forward-patch to HEAD.

Thanks for the patch!

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> I've finally looked it over, and it still looks fine. I've cleaned up
> some of the whitespace issues in the patch and applied it, including a
> backpatch to 8.3 and a forward-patch to HEAD.

Not 8.2?

regards, tom lane

On Saturday, November 14, 2009, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> I've finally looked it over, and it still looks fine. I've cleaned up
>> some of the whitespace issues in the patch and applied it, including a
>> backpatch to 8.3 and a forward-patch to HEAD.
>
> Not 8.2?

Uh. Yeah. Somehow I thought we only supported 7.3 and up. Too used to
there being only two backbranches for win32. Oops.

Sk yeah, I'll go backpath it to 8.2 as well. Probably not tonight but
I'll try to get it done tomorrow.

/Magnus

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Sat, Nov 14, 2009 at 18:12, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Saturday, November 14, 2009, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>> I've finally looked it over, and it still looks fine. I've cleaned up
>>> some of the whitespace issues in the patch and applied it, including a
>>> backpatch to 8.3 and a forward-patch to HEAD.
>>
>> Not 8.2?
>
> Uh. Yeah. Somehow I thought we only supported 7.3 and up. Too used to
> there being only two backbranches for win32. Oops.
>
> Sk yeah, I'll go backpath it to 8.2 as well. Probably not tonight but
> I'll try to get it done tomorrow.

Done.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> On Saturday, November 14, 2009, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Not 8.2?

> Done.

Buildfarm member narwhal seems to think it's not quite right in 8.2.

regards, tom lane

On Mon, Nov 16, 2009 at 06:23, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>> On Saturday, November 14, 2009, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> Not 8.2?
>
>> Done.
>
> Buildfarm member narwhal seems to think it's not quite right in 8.2.

Drat.

So, the new API function requires Windows 2000 or more, and I think
the reason it's failing is that the headers in 8.2 don't specify that
at this point. Which means that 8.2 today theoretically at least runs
on NT4. Which leads to the question - do we want to back out the
patch, have 8.2 keep working on that, and having 8.2 fail in the few
unusual scenarios we have now, or do we also backpatch the requirement
to run on win2k or later?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander wrote:

> So, the new API function requires Windows 2000 or more, and I think
> the reason it's failing is that the headers in 8.2 don't specify that
> at this point. Which means that 8.2 today theoretically at least runs
> on NT4. Which leads to the question - do we want to back out the
> patch, have 8.2 keep working on that, and having 8.2 fail in the few
> unusual scenarios we have now, or do we also backpatch the requirement
> to run on win2k or later?

I think the safest course is to revert the fix and keep 8.2 working on
all platforms on which it used to work.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Magnus Hagander wrote:
>> Which leads to the question - do we want to back out the
>> patch, have 8.2 keep working on that, and having 8.2 fail in the few
>> unusual scenarios we have now, or do we also backpatch the requirement
>> to run on win2k or later?

> I think the safest course is to revert the fix and keep 8.2 working on
> all platforms on which it used to work.

+1 --- since this seems to be a corner case, desupporting old Windows
versions in a minor update doesn't seem like the thing to do.

Sorry for creating work for you, Magnus.

regards, tom lane

2009/11/16 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> Magnus Hagander wrote:
>>> Which leads to the question - do we want to back out the
>>> patch, have 8.2 keep working on that, and having 8.2 fail in the few
>>> unusual scenarios we have now, or do we also backpatch the requirement
>>> to run on win2k or later?
>
>> I think the safest course is to revert the fix and keep 8.2 working on
>> all platforms on which it used to work.
>
> +1 --- since this seems to be a corner case, desupporting old Windows
> versions in a minor update doesn't seem like the thing to do.

Agreed, reverted.

> Sorry for creating work for you, Magnus.

No problem.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/