Re: fixing pg_ctl with relative paths - Mailing list pgsql-hackers

At Fri, 31 Jul 2020 09:42:42 +0800 (CST), ZHAOWANCHENG  <zhaowcheng@163.com> wrote in 
> At 2014-01-28 21:11:54, "Bruce Momjian" <bruce@momjian.us> wrote:
> >On Mon, Jul  1, 2013 at 08:10:14PM -0400, Josh Kupershmidt wrote:
> >> On Thu, Jun 27, 2013 at 11:47 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> >> > On Thu, Jun 27, 2013 at 10:36 AM, Josh Kupershmidt <schmiddy@gmail.com> wrote:
> >> >> On Wed, Jun 26, 2013 at 12:22 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> >> >>> Though this is a corner case, the patch doesn't seem to handle properly the case
> >> >>> where "-D" appears as other option value, e.g., -k option value, in
> >> >>> postmaster.opts
> >> >>> file.
> >> >>
> >> >> Could I see a command-line example of what you mean?
> >> >
> >> > postmaster -k "-D", for example. Of course, it's really a corner case :)
> >> 
> >> Oh, I see. I was able to trip up strip_datadirs() with something like
> >> 
> >> $ PGDATA="/my/data/" postmaster -k "-D" -S 100 &
> >> $ pg_ctl -D /my/data/ restart
> >> 
> >> that example causes pg_ctl to fail to start the server after stopping
> >> it, although perhaps you could even trick the server into starting
> >> with the wrong options. Of course, similar problems exists today in
> >> other cases, such as with the relative paths issue this patch is
> >> trying to address, or a datadir containing embedded quotes.
> >> 
> >> I am eager to see the relative paths issue fixed, but maybe we need to
> >> bite the bullet and sort out the escaping of command-line options in
> >> the rest of pg_ctl first, so that a DataDir like "/tmp/here's a \"
> >> quote" can consistently be used by pg_ctl {start|stop|restart} before
> >> we can fix this wart.
> >
> >Where are we on this patch?
> >
> >-- 
> >  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
> >  EnterpriseDB                             http://enterprisedb.com
> >
> >  + Everyone has their own god. +
> >
> >
> >-- 
> >Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> >To make changes to your subscription:
> >http://www.postgresql.org/mailpref/pgsql-hackers
> 
> >
> 
> 
> Hi, I encountered the same problem.
> I want to know is there a final conclusion?
> thank you very much!

It seems to me agrouding on parsing issue. We haven't find a way to
parse the content of postmaster.opt properly.

1. For escaped option arguments, we can't find where directory name ends.

  "-D" "/tmp/here's a \" quote"

2. We need to distinguish option names and arguments.

  "-k" "-D"       # "-D" is an arguemnt, not a option name.

3. This is not mentioned here, but getopt accepts "merged" (I'm not
 sure what to call it.) short options.

  "-iD" "/hoge"   # equivalent to "-i" "-D" "hoge"

We need to either let pg_ctl reparse the commandline the same way with
postmaster or let postmaster normalize and/or markup the content of
postmaster.opts so that pg_ctl can read it desired way. That can be as
attached, but the change seems a bit too big..



regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 5b5fc97c72..0650cc10e8 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -560,6 +560,45 @@ int            postmaster_alive_fds[2] = {-1, -1};
 HANDLE        PostmasterHandle;
 #endif
 
+char   *norm_args = NULL;  /* normalized arguments */
+char   *norm_args_tail = NULL;
+int        norm_args_len = 0;
+
+static void
+add_norm_argument(int opt, char *value)
+{
+    int valuelen = 0;
+
+    if (norm_args_len == 0)
+    {
+        norm_args_len = 128;
+        norm_args = malloc(norm_args_len);
+        norm_args_tail = norm_args;
+    }
+
+    if (opt == 0)
+    {
+        *norm_args_tail++ = '\0';   /* terminator */
+        return;
+    }
+
+    if (value)
+        valuelen = strlen(value) + 3;  /* _\"val\"*/
+
+    /* expand buffer as needed */
+    while (norm_args_tail - norm_args + 4 /* \"-x\" */ + valuelen + 1
+           > norm_args_len)
+        norm_args_len *= 2;
+    norm_args = realloc(norm_args, norm_args_len);
+
+    *norm_args_tail++ = '\1';        /* delimiter */
+
+    if (value != NULL)
+        norm_args_tail += sprintf(norm_args_tail, "\"-%c\" \"%s\"", opt, value);
+    else
+        norm_args_tail += sprintf(norm_args_tail, "\"-%c\"", opt);
+}
+
 /*
  * Postmaster main entry point
  */
@@ -680,6 +719,8 @@ PostmasterMain(int argc, char *argv[])
      */
     while ((opt = getopt(argc, argv, "B:bc:C:D:d:EeFf:h:ijk:lN:nOo:Pp:r:S:sTt:W:-:")) != -1)
     {
+        add_norm_argument(opt, optarg);
+
         switch (opt)
         {
             case 'B':
@@ -850,6 +891,9 @@ PostmasterMain(int argc, char *argv[])
         }
     }
 
+    /* terminate normalized arguemnt list */
+    add_norm_argument(0, NULL);
+
     /*
      * Postmaster accepts no non-option switch arguments.
      */
@@ -5666,7 +5710,6 @@ static bool
 CreateOptsFile(int argc, char *argv[], char *fullprogname)
 {
     FILE       *fp;
-    int            i;
 
 #define OPTS_FILE    "postmaster.opts"
 
@@ -5677,8 +5720,8 @@ CreateOptsFile(int argc, char *argv[], char *fullprogname)
     }
 
     fprintf(fp, "%s", fullprogname);
-    for (i = 1; i < argc; i++)
-        fprintf(fp, " \"%s\"", argv[i]);
+    if (norm_args)
+        fprintf(fp, "%s", norm_args);
     fputs("\n", fp);
 
     if (fclose(fp))
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 1cdc3ebaa3..b4ccaf224f 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -755,10 +755,37 @@ read_post_opts(void)
                  * Are we at the first option, as defined by space and
                  * double-quote?
                  */
-                if ((arg1 = strstr(optline, " \"")) != NULL)
+                if ((arg1 = strstr(optline, "\1\"")) != NULL)
                 {
+                    char *pto;
+                    char *pfrom;
+
                     *arg1 = '\0';    /* terminate so we get only program name */
                     post_opts = pg_strdup(arg1 + 1);    /* point past whitespace */
+
+                    pto = pfrom = post_opts;
+                    while (*pfrom)
+                    {
+                        if (*pfrom != '\1')
+                        {
+                            *pto++ = *pfrom++;
+                            continue;
+                        }
+
+                        pfrom++;
+
+                        /* Remove -D optsion if we have a replacment */
+                        if (pgdata_opt && strncmp(pfrom, "\"-D\"", 4) == 0)
+                        {
+                            /* Skip -D option */
+                            while (*pfrom && *pfrom != '\1') pfrom++;
+                            continue;
+                        }
+
+                        /* replace '\1' with a space */
+                        *pto++ = ' ';
+                    }
+                    *pto = 0;
                 }
                 if (exec_path == NULL)
                     exec_path = pg_strdup(optline);
@@ -870,8 +897,8 @@ do_start(void)
 
     read_post_opts();
 
-    /* No -D or -D already added during server start */
-    if (ctl_command == RESTART_COMMAND || pgdata_opt == NULL)
+    /* Use "" for printf safety */
+    if (pgdata_opt == NULL)
         pgdata_opt = "";
 
     if (exec_path == NULL)