Re: BUG #5065: pg_ctl start fails as administrator, with "could not locate matching postgres executable" - Mailing list pgsql-bugs

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------