Re: PostgreSQL Service on Windows does not start. ~ "is not a valid Win32 application"

Rajeev rastogi <rajeev.rastogi@huawei.com> writes:
> One suggestion:
> Instead of using sizeof(cmdLine),
>     a. Can't we use strlen  (hence small 'for' loop).
>     b. Or use memmove to move one byte.

I looked at this patch a bit.  I agree that we need to fix
pgwin32_CommandLine to double-quote the executable name, but it needs a
great deal more work than that :-(.  Whoever wrote this code was
apparently unacquainted with the concept of buffer overrun.  It's not
going to be hard at all to crash pg_ctl with overlength arguments.  I'm
not sure that that amounts to a security bug, but it's certainly bad.

After some thought it seems like the most future-proof fix is to not
use a fixed-length buffer for the command string at all.  The attached
revised patch switches it over to using a PQExpBuffer instead, which is
pretty much free since we're relying on libpq anyway in this program.
(We still use a fixed-length buffer for the program path, which is OK
because that's what find_my_exec and find_other_exec expect.)

In addition, I fixed it to append .exe in both cases not just the one.

I'm not in a position to actually test this, but it does compile
without warnings.

            regards, tom lane

diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 8399cdd..dd80719 100644
*** a/src/bin/pg_ctl/pg_ctl.c
--- b/src/bin/pg_ctl/pg_ctl.c
***************
*** 18,24 ****
--- 18,26 ----
  #endif

  #include "postgres_fe.h"
+
  #include "libpq-fe.h"
+ #include "pqexpbuffer.h"

  #include <fcntl.h>
  #include <locale.h>
*************** pgwin32_IsInstalled(SC_HANDLE hSCM)
*** 1238,1253 ****
  static char *
  pgwin32_CommandLine(bool registration)
  {
!     static char cmdLine[MAXPGPATH];
      int            ret;

- #ifdef __CYGWIN__
-     char        buf[MAXPGPATH];
- #endif
-
      if (registration)
      {
!         ret = find_my_exec(argv0, cmdLine);
          if (ret != 0)
          {
              write_stderr(_("%s: could not find own program executable\n"), progname);
--- 1240,1252 ----
  static char *
  pgwin32_CommandLine(bool registration)
  {
!     PQExpBuffer cmdLine = createPQExpBuffer();
!     char        cmdPath[MAXPGPATH];
      int            ret;

      if (registration)
      {
!         ret = find_my_exec(argv0, cmdPath);
          if (ret != 0)
          {
              write_stderr(_("%s: could not find own program executable\n"), progname);
*************** pgwin32_CommandLine(bool registration)
*** 1257,1263 ****
      else
      {
          ret = find_other_exec(argv0, "postgres", PG_BACKEND_VERSIONSTR,
!                               cmdLine);
          if (ret != 0)
          {
              write_stderr(_("%s: could not find postgres program executable\n"), progname);
--- 1256,1262 ----
      else
      {
          ret = find_other_exec(argv0, "postgres", PG_BACKEND_VERSIONSTR,
!                               cmdPath);
          if (ret != 0)
          {
              write_stderr(_("%s: could not find postgres program executable\n"), progname);
*************** pgwin32_CommandLine(bool registration)
*** 1267,1320 ****

  #ifdef __CYGWIN__
      /* need to convert to windows path */
  #if CYGWIN_VERSION_DLL_MAJOR >= 1007
!     cygwin_conv_path(CCP_POSIX_TO_WIN_A, cmdLine, buf, sizeof(buf));
  #else
!     cygwin_conv_to_full_win32_path(cmdLine, buf);
  #endif
!     strcpy(cmdLine, buf);
  #endif

      if (registration)
!     {
!         if (pg_strcasecmp(cmdLine + strlen(cmdLine) - 4, ".exe") != 0)
!         {
!             /* If commandline does not end in .exe, append it */
!             strcat(cmdLine, ".exe");
!         }
!         strcat(cmdLine, " runservice -N \"");
!         strcat(cmdLine, register_servicename);
!         strcat(cmdLine, "\"");
!     }

      if (pg_config)
!     {
!         strcat(cmdLine, " -D \"");
!         strcat(cmdLine, pg_config);
!         strcat(cmdLine, "\"");
!     }

      if (registration && do_wait)
!         strcat(cmdLine, " -w");

      if (registration && wait_seconds != DEFAULT_WAIT)
!         /* concatenate */
!         sprintf(cmdLine + strlen(cmdLine), " -t %d", wait_seconds);

      if (registration && silent_mode)
!         strcat(cmdLine, " -s");

      if (post_opts)
      {
-         strcat(cmdLine, " ");
-         if (registration)
-             strcat(cmdLine, " -o \"");
-         strcat(cmdLine, post_opts);
          if (registration)
!             strcat(cmdLine, "\"");
      }

!     return cmdLine;
  }

  static void
--- 1266,1319 ----

  #ifdef __CYGWIN__
      /* need to convert to windows path */
+     {
+         char        buf[MAXPGPATH];
+
  #if CYGWIN_VERSION_DLL_MAJOR >= 1007
!         cygwin_conv_path(CCP_POSIX_TO_WIN_A, cmdPath, buf, sizeof(buf));
  #else
!         cygwin_conv_to_full_win32_path(cmdPath, buf);
  #endif
!         strcpy(cmdPath, buf);
!     }
  #endif

+     /* if path does not end in .exe, append it */
+     if (strlen(cmdPath) < 4 ||
+         pg_strcasecmp(cmdPath + strlen(cmdPath) - 4, ".exe") != 0)
+         snprintf(cmdPath + strlen(cmdPath), sizeof(cmdPath) - strlen(cmdPath),
+                  ".exe");
+
+     /* be sure to double-quote the executable's name in the command */
+     appendPQExpBuffer(cmdLine, "\"%s\"", cmdPath);
+
+     /* append assorted switches to the command line, as needed */
+
      if (registration)
!         appendPQExpBuffer(cmdLine, " runservice -N \"%s\"",
!                           register_servicename);

      if (pg_config)
!         appendPQExpBuffer(cmdLine, " -D \"%s\"", pg_config);

      if (registration && do_wait)
!         appendPQExpBuffer(cmdLine, " -w");

      if (registration && wait_seconds != DEFAULT_WAIT)
!         appendPQExpBuffer(cmdLine, " -t %d", wait_seconds);

      if (registration && silent_mode)
!         appendPQExpBuffer(cmdLine, " -s");

      if (post_opts)
      {
          if (registration)
!             appendPQExpBuffer(cmdLine, " -o \"%s\"", post_opts);
!         else
!             appendPQExpBuffer(cmdLine, " %s", post_opts);
      }

!     return cmdLine->data;
  }

  static void
*************** CreateRestrictedProcess(char *cmd, PROCE
*** 1745,1751 ****
       */
      return r;
  }
! #endif

  static void
  do_advice(void)
--- 1744,1750 ----
       */
      return r;
  }
! #endif    /* defined(WIN32) || defined(__CYGWIN__) */

  static void
  do_advice(void)