Обсуждение: 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@coverity.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=A0pm, jmor...@coverity.com ("Jesse Morris") wrote: > The following bug has been logged online: > > Bug reference: =A0 =A0 =A05065 > Logged by: =A0 =A0 =A0 =A0 =A0Jesse Morris > Email address: =A0 =A0 =A0jmor...@coverity.com > PostgreSQL version: 8.3.7, 8.4.1 > Operating system: =A0 Windows Server 2003 R2 > Description: =A0 =A0 =A0 =A0pg_ctl start fails as administrator, with "co= uld 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 postg= res > 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). =A0 ... 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 =3D CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL, TRUE, CREATE_SUSPENDED, NULL, NULL, &si, processInfo); Kernel32Handle =3D 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 =3D 0; DWORD dwTokenInfoLength =3D 0; - HANDLE hToken =3D NULL; PACL pacl =3D NULL; PSID psidUser =3D NULL; TOKEN_DEFAULT_DACL tddNew; @@ -703,13 +701,6 @@ AddUserToDacl(HANDLE hProcess) TOKEN_INFORMATION_CLASS tic =3D TokenDefaultDacl; BOOL ret =3D 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 =3D 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@coverity.com> wrote: > I went back to the version that supposedly initially fixed this issue, > but I couldn't get it to work either. =A0So I think the DACL adjustment > code was always broken. =A0The 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. =A0 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. =A0I didn't do > anything to fix pg_ctl's error reporting. =A0: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! --=20 Dave Page EnterpriseDB UK: http://www.enterprisedb.com
> -----Original Message----- > From: Dave Page [mailto:dpage@pgadmin.org] > Sent: Friday, October 16, 2009 2:14 AM > To: Jesse Morris > Cc: pgsql-bugs@postgresql.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@coverity.com> wrote: >> -----Original Message----- >> From: Dave Page [mailto:dpage@pgadmin.org] >> Sent: Friday, October 16, 2009 2:14 AM >> To: Jesse Morris >> Cc: pgsql-bugs@postgresql.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! =A0Re-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? --=20 Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page wrote: > On Fri, Oct 16, 2009 at 7:03 PM, Jesse Morris <jmorris@coverity.com> wrote: > >>> -----Original Message----- >>> From: Dave Page [mailto:dpage@pgadmin.org] >>> Sent: Friday, October 16, 2009 2:14 AM >>> To: Jesse Morris >>> Cc: pgsql-bugs@postgresql.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.=20=20 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.=20=20 =20 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.=20=20 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.=20 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.=20=20 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.=20=20 Now: To answer your question of "What if the Administrator user is granted additional permissions?"=20=20 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.=20 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.=20=20= =20 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.=20=20 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@dunslane.net] > Sent: Monday, October 19, 2009 11:03 AM > To: Dave Page > Cc: Jesse Morris; pgsql-bugs@postgresql.org; Magnus Hagander > Subject: Re: [BUGS] Re: BUG #5065: pg_ctl start fails as administrator, > with "could not locate matching postgres executable" >=20 > 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). >=20 > 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? >=20 > cheers >=20 > andrew
On Mon, Oct 19, 2009 at 7:03 PM, Andrew Dunstan <andrew@dunslane.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@pgadmin.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@sss.pgh.pa.us> wrote: > Dave Page <dpage@pgadmin.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? =A0I am always distrustful of random changes made > with no theory as to why they fix a problem. =A0My 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. --=20 Dave Page EnterpriseDB UK: http://www.enterprisedb.com PGDay.EU 2009 Conference: http://2009.pgday.eu/start
Dave Page <dpage@pgadmin.org> writes: > On Tue, Oct 20, 2009 at 3:48 PM, Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us] > Sent: Tuesday, October 20, 2009 9:45 AM > To: Dave Page > Cc: Andrew Dunstan; Jesse Morris; pgsql-bugs@postgresql.org; Magnus > Hagander > Subject: Re: [BUGS] Re: BUG #5065: pg_ctl start fails as administrator, > with "could not locate matching postgres executable" >=20 > Dave Page <dpage@pgadmin.org> writes: > > On Tue, Oct 20, 2009 at 3:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Do we have any idea why? >=20 > > 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. >=20 > 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?) >=20 > Seems like it would be worth the trouble to identify exactly what the > critical difference is. >=20 > regards, tom lane
On Tue, Oct 20, 2009 at 5:45 PM, Tom Lane <tgl@sss.pgh.pa.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@pgadmin.org> wrote: > On Tue, Oct 20, 2009 at 5:45 PM, Tom Lane <tgl@sss.pgh.pa.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@pgadmin.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@sss.pgh.pa.us> wrote: > Dave Page <dpage@pgadmin.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. =A0Okay, I'm satisfied that this is a believable patch... =46rom 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 :-) --=20 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 :-) > > > I'd rather wait till you can check it. cheers andrew
On Wed, Oct 21, 2009 at 15:34, Andrew Dunstan <andrew@dunslane.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@hagander.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@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.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@hagander.net> wrote: > On Saturday, November 14, 2009, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Magnus Hagander <magnus@hagander.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@hagander.net> writes: >> On Saturday, November 14, 2009, Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >>> On Saturday, November 14, 2009, Tom Lane <tgl@sss.pgh.pa.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@commandprompt.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@sss.pgh.pa.us>: > Alvaro Herrera <alvherre@commandprompt.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/