Re: Re: fixing pg_ctl with relative paths

2020-08-01 Thread Chapman Flack
On 07/01/13 20:10, Josh Kupershmidt wrote:
> 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.

It was timely to see this thread recently revived, as I had only just
recently needed to contend with the same escaping issue while writing a
PostgresNode-like test harness for PL/Java, where I discovered I have
to pass -o values pre-transformed to pg_ctl, and even have to do that
platform-sensitively, to pre-transform them according to the rules for
Bourne shell or those for cmd.exe.

I looked at the history of that code in pg_ctl and saw that it went
all the way back, so I assumed that any proposal to fix it would probably
be met with "it has always been that way and anybody calling it with
arbitrary arguments must be pre-transforming them anyway and it would be
bad to break that." (And anyway, my test harness thing is now yet one more
thing that depends on the current behavior.)

But would it be worthwhile to perhaps make a start, add an option
(non-default at first) that changes to an implementation that passes
values transparently and isn't injection-prone?

(I use "injection-prone" not because I'd be especially concerned about
command injection by anybody who can already run pg_ctl, just because
it's an economical way to describe what pg_ctl currently does.)

Regards,
-Chap




Re: fixing pg_ctl with relative paths

2020-07-31 Thread Kyotaro Horiguchi
At Fri, 31 Jul 2020 09:42:42 +0800 (CST), ZHAOWANCHENG   
wrote in 
> At 2014-01-28 21:11:54, "Bruce Momjian"  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  
> >> wrote:
> >> > On Thu, Jun 27, 2013 at 10:36 AM, Josh Kupershmidt  
> >> > wrote:
> >> >> On Wed, Jun 26, 2013 at 12:22 PM, Fujii Masao  
> >> >> 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  http://momjian.us
> >  EnterpriseDB http://enterprisedb.com
> >
> >  + Everyone has their own god. +
> >
> >
> >-- 
> >Sent via pgsql-hackers mailing list (pgsql-hack...@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 

Re: fixing pg_ctl with relative paths

2020-07-30 Thread ZHAOWANCHENG






At 2014-01-28 21:11:54, "Bruce Momjian"  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  wrote:
>> > On Thu, Jun 27, 2013 at 10:36 AM, Josh Kupershmidt  
>> > wrote:
>> >> On Wed, Jun 26, 2013 at 12:22 PM, Fujii Masao  
>> >> 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  http://momjian.us
>  EnterpriseDB http://enterprisedb.com
>
>  + Everyone has their own god. +
>
>
>-- 
>Sent via pgsql-hackers mailing list (pgsql-hack...@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!