Make initdb's suggested "pg_ctl start" command line more reliable.

The original coding here was not nearly careful enough about quoting
special characters, and it didn't get corner cases right for constructing
the pg_ctl path either.  Use join_path_components() and appendShellString()
to do it honestly, so that the string will more likely work if blindly
copied-and-pasted.

While at it, teach appendShellString() not to quote strings that clearly
don't need it, so that the output from initdb doesn't become uglier than
it was before in typical cases where quoting is not needed.

Ryan Murphy, reviewed by Michael Paquier and myself

Discussion: <CAHeEsBeAe1FeBypT3E8R1ZVZU0e8xv3A-7BHg6bEOi=jZny2Uw@mail.gmail.com>

diff --git a/src/bin/initdb/Makefile b/src/bin/initdb/Makefile
index 094c8945c9600206ae49727d7d865b14067890fb..531cc979a48bd077e4200414ca84013b20bce7bf 100644 (file)
--- a/src/bin/initdb/Makefile
+++ b/src/bin/initdb/Makefile
@@ -18,6 +18,9 @@ include $(top_builddir)/src/Makefile.global
 
 override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) -I$(top_srcdir)/src/timezone $(CPPFLAGS)
 
+# note: we need libpq only because fe_utils does
+LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
+
 # use system timezone data?
 ifneq (,$(with_system_tzdata))
 override CPPFLAGS += '-DSYSTEMTZDIR="$(with_system_tzdata)"'

diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index a978bbc328ae8695e5666dcf049e337c61ff7924..aad6ba5639f12095354299b63181e714dc255a9b 100644 (file)
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -67,6 +67,7 @@
 #include "getaddrinfo.h"
 #include "getopt_long.h"
 #include "miscadmin.h"
+#include "fe_utils/string_utils.h"
 
 
 /* Define PG_FLUSH_DATA_WORKS if we have an implementation for pg_flush_data */
@@ -331,14 +332,6 @@ do { \
                output_failed = true, output_errno = errno; \
 } while (0)
 
-#ifndef WIN32
-#define QUOTE_PATH     ""
-#define DIR_SEP "/"
-#else
-#define QUOTE_PATH     "\""
-#define DIR_SEP "\\"
-#endif
-
 static char *
 escape_quotes(const char *src)
 {
@@ -3359,7 +3352,8 @@ main(int argc, char *argv[])
        int                     c;
        int                     option_index;
        char       *effective_user;
-       char            bin_dir[MAXPGPATH];
+       PQExpBuffer start_db_cmd;
+       char            pg_ctl_path[MAXPGPATH];
 
        /*
         * Ensure that buffering behavior of stdout and stderr matches what it is
@@ -3587,14 +3581,33 @@ main(int argc, char *argv[])
        if (authwarning != NULL)
                fprintf(stderr, "%s", authwarning);
 
-       /* Get directory specification used to start this executable */
-       strlcpy(bin_dir, argv[0], sizeof(bin_dir));
-       get_parent_directory(bin_dir);
+       /*
+        * Build up a shell command to tell the user how to start the server
+        */
+       start_db_cmd = createPQExpBuffer();
+
+       /* Get directory specification used to start initdb ... */
+       strlcpy(pg_ctl_path, argv[0], sizeof(pg_ctl_path));
+       canonicalize_path(pg_ctl_path);
+       get_parent_directory(pg_ctl_path);
+       /* ... and tag on pg_ctl instead */
+       join_path_components(pg_ctl_path, pg_ctl_path, "pg_ctl");
+
+       /* path to pg_ctl, properly quoted */
+       appendShellString(start_db_cmd, pg_ctl_path);
+
+       /* add -D switch, with properly quoted data directory */
+       appendPQExpBufferStr(start_db_cmd, " -D ");
+       appendShellString(start_db_cmd, pgdata_native);
+
+       /* add suggested -l switch and "start" command */
+       appendPQExpBufferStr(start_db_cmd, " -l logfile start");
 
        printf(_("\nSuccess. You can now start the database server using:\n\n"
-                        "    %s%s%spg_ctl%s -D %s%s%s -l logfile start\n\n"),
-          QUOTE_PATH, bin_dir, (strlen(bin_dir) > 0) ? DIR_SEP : "", QUOTE_PATH,
-                  QUOTE_PATH, pgdata_native, QUOTE_PATH);
+                        "        %s\n\n"),
+                  start_db_cmd->data);
+
+       destroyPQExpBuffer(start_db_cmd);
 
        return 0;
 }

diff --git a/src/fe_utils/string_utils.c b/src/fe_utils/string_utils.c
index 2c566b1ad759a2b665a810d93f15ee2d8231c96a..edbc869e4534c39d248cec8a0f2a5bb6e3ed8d2e 100644 (file)
--- a/src/fe_utils/string_utils.c
+++ b/src/fe_utils/string_utils.c
@@ -418,7 +418,7 @@ appendByteaLiteral(PQExpBuffer buf, const unsigned char *str, size_t length,
 
 /*
  * Append the given string to the shell command being built in the buffer,
- * with suitable shell-style quoting to create exactly one argument.
+ * with shell-style quoting as needed to create exactly one argument.
  *
  * Forbid LF or CR characters, which have scant practical use beyond designing
  * security breaches.  The Windows command shell is unusable as a conduit for
@@ -429,8 +429,22 @@ appendByteaLiteral(PQExpBuffer buf, const unsigned char *str, size_t length,
 void
 appendShellString(PQExpBuffer buf, const char *str)
 {
+#ifdef WIN32
+       int                     backslash_run_length = 0;
+#endif
        const char *p;
 
+       /*
+        * Don't bother with adding quotes if the string is nonempty and clearly
+        * contains only safe characters.
+        */
+       if (*str != '\0' &&
+               strspn(str, "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_./:") == strlen(str))
+       {
+               appendPQExpBufferStr(buf, str);
+               return;
+       }
+
 #ifndef WIN32
        appendPQExpBufferChar(buf, '\'');
        for (p = str; *p; p++)
@@ -450,7 +464,6 @@ appendShellString(PQExpBuffer buf, const char *str)
        }
        appendPQExpBufferChar(buf, '\'');
 #else                                                  /* WIN32 */
-       int                     backslash_run_length = 0;
 
        /*
         * A Windows system() argument experiences two layers of interpretation.