Re: fixing pg_ctl with relative paths

There have been some complaints[1][2] in the past about pg_ctl not
playing nice with relative path specifications for the datadir. Here's
a concise illustration:

$ mkdir /tmp/mydata/ && initdb /tmp/mydata/
$ cd /tmp/
$ pg_ctl -D ./mydata/ start
$ cd /
$ pg_ctl -D /tmp/mydata/ restart

IMO it's pretty hard to defend the behavior of the last step, where
pg_ctl knows exactly which datadir the user has specified, and
succeeds in stopping the server but not starting it.

Digging into this gripe, a related problem I noticed is that `pg_ctl
... restart` doesn't always preserve the "-D $DATADIR" argument as the
following comment suggests it should[4]:

* We could pass PGDATA just in an environment
* variable but we do -D too for clearer postmaster
* 'ps' display

Specifically, if one passes in additional -o options, e.g.

$ pg_ctl -D /tmp/mydata/ -o "-N 10" restart

after which postmaster.opts will be missing the "-D ..." argument
which is otherwise recorded, and the `ps` output is similarly
abridged.

Anyway, Tom suggested[3] two possible ways of fixing the original
gripe, and I went with his latter suggestion,

| for pg_ctl restart to override that
| option with its freshly-derived idea of where the data directory is

mainly so we don't need to worry about the impact of changing the
appearance of postmaster.opts, plus it seems like this code fits
better in pg_ctl.c rather than the postmaster (where the
postmaster.opts file is actually written). The fix seems to be pretty
simple, namely stripping post_opts of the old "-D ... " portion and
having the new specification, if any, be used instead. I believe the
attached patch should fix these two infelicities.

Full disclosure: the strip_datadirs() function makes no attempt to
properly handle pathnames containing quotes. There seems to be some,
uh, confusion in the existing code about how these should be handled
already. For instance,

$ mkdir "/tmp/here's a \" quote"
$ initdb "/tmp/here's a \" quote"

How to successfully start, restart, and stop the server with pg_ctl is
left as an exercise for the reader. So I tried to avoid that can of
worms with this patch, though I'm happy to robustify strip_datadirs()
if we'd like to properly support such pathnames, and there's consensus
on how to standardize the escaping.

Josh

[1] http://www.postgresql.org/message-id/CAA-aLv72O+NegjAipHORmbqSMZTkZayaTxrd+C9v60YbmMmZUQ@mail.gmail.com
[2] http://www.postgresql.org/message-id/CAK3UJRGABxWSOCXnAsSYw5BfR4D9ageXF+6GtsRVm-LtfWfW=g@mail.gmail.com
[3] http://www.postgresql.org/message-id/27233.1350234453@sss.pgh.pa.us
[4] Note, ps output and postmaster.opts will not include the datadir
specification if you rely solely on the PGDATA environment variable
for pg_ctl

On January 23, 2013 9:13 AM Josh Kupershmidt wrote:
>There have been some complaints[1][2] in the past about pg_ctl not playing
nice with relative path specifications for the datadir. Here's a concise
illustration:
>
> $ mkdir /tmp/mydata/ && initdb /tmp/mydata/
> $ cd /tmp/
> $ pg_ctl -D ./mydata/ start
> $ cd /
> $ pg_ctl -D /tmp/mydata/ restart
>
>IMO it's pretty hard to defend the behavior of the last step, where pg_ctl
knows exactly which datadir the user has specified, and succeeds in stopping
the >server but not starting it.
>
>Digging into this gripe, a related problem I noticed is that `pg_ctl ...
restart` doesn't always preserve the "-D $DATADIR" argument as the following
comment >suggests it should[4]:
>
> * We could pass PGDATA just in an environment
> * variable but we do -D too for clearer postmaster
> * 'ps' display
>
>Specifically, if one passes in additional -o options, e.g.
>
> $ pg_ctl -D /tmp/mydata/ -o "-N 10" restart
>
>after which postmaster.opts will be missing the "-D ..." argument which is
otherwise recorded, and the `ps` output is similarly abridged.
>
>Anyway, Tom suggested[3] two possible ways of fixing the original gripe,
and I went with his latter suggestion,
>
>| for pg_ctl restart to override that
>| option with its freshly-derived idea of where the data directory is
>
>mainly so we don't need to worry about the impact of changing the
appearance of postmaster.opts, plus it seems like this code fits better in
pg_ctl.c rather than >the postmaster (where the postmaster.opts file is
actually written). The fix seems to be pretty simple, namely stripping
post_opts of the old "-D ... " portion >and having the new specification, if
any, be used instead. I believe the attached patch should fix these two
infelicities.
>
>Full disclosure: the strip_datadirs() function makes no attempt to properly
handle pathnames containing quotes. There seems to be some, uh, confusion in
the >existing code about how these should be handled already. For instance,
>
> $ mkdir "/tmp/here's a \" quote"
> $ initdb "/tmp/here's a \" quote"
>
>How to successfully start, restart, and stop the server with pg_ctl is left
as an exercise for the reader. So I tried to avoid that can of worms with
this patch, >though I'm happy to robustify strip_datadirs() if we'd like to
properly support such pathnames, and there's consensus on how to standardize
the escaping.
>
>[1]
http://www.postgresql.org/message-id/CAA-aLv72O+NegjAipHORmbqSMZTkZayaTxrd+C
9v60YbmMmZUQ(at)mail(dot)gmail(dot)com
>[2]
http://www.postgresql.org/message-id/CAK3UJRGABxWSOCXnAsSYw5BfR4D9ageXF+6Gts
RVm-LtfWfW=g(at)mail(dot)gmail(dot)com
>[3] http://www.postgresql.org/message-id/27233.1350234453@sss.pgh.pa.us
>[4] Note, ps output and postmaster.opts will not include the datadir
specification if you rely solely on the PGDATA environment variable for
pg_ctl

Please find the review of the patch.

Basic stuff:
------------
- Patch applies OK
- Compiles cleanly with no warnings
- Regression tests pass.

What it does:
-------------
The restart functionality of pg_ctl has problems with relative paths. This
patch removes the
problems arising during restart. This patch strips the data directory which
is present in the
postmaster options and keep the rest of the options already provided incase
if user not provided
any options during restart.

Code Review:
------------
+if (orig_post_opts) {
+ post_opts = strip_datadirs(orig_post_opts);
+}

No need of "{}" as the only one statement block is present in the if block.

+ free(tmp);

The above statement can be moved inside the if (*(trailing_quote + 1) !=
'\0') {
where it's exact usage is present.

Testing:
--------
I tested this feature with different postmaster options and database folder
names, found no problem.

The database folder with quotes present in it is any way having problems
with pg_ctl.
I feel the strip_datadirs() function header explanation is providing good
understanding.
Overall the patch is good. It makes the pg_ctl restart functionality works
well.

Regards,
Hari babu

On Tue, Jun 25, 2013 at 2:28 AM, Hari Babu <haribabu(dot)kommi(at)huawei(dot)com> wrote:
> Please find the review of the patch.

Thank you for reviewing!

> Code Review:
> ------------
> +if (orig_post_opts) {
> + post_opts = strip_datadirs(orig_post_opts);
> +}
>
> No need of "{}" as the only one statement block is present in the if block.

OK.

> + free(tmp);
>
> The above statement can be moved inside the if (*(trailing_quote + 1) !=
> '\0') {
> where it's exact usage is present.

Right.

> Testing:
> --------
> I tested this feature with different postmaster options and database folder
> names, found no problem.
>
>
> The database folder with quotes present in it is any way having problems
> with pg_ctl.
> I feel the strip_datadirs() function header explanation is providing good
> understanding.
> Overall the patch is good. It makes the pg_ctl restart functionality works
> well.

Thanks for the feedback. Attached is a rebased version of the patch
with the two small issues noted fixed.

Josh

On June 26, 2013 5:02 AM Josh Kupershmidt wrote:
>Thanks for the feedback. Attached is a rebased version of the patch with
the two small issues noted fixed.

Patch is good, I marked the patch as ready for committer.

Regards,
Hari babu

On Wed, Jun 26, 2013 at 2:36 PM, Hari Babu <haribabu(dot)kommi(at)huawei(dot)com> wrote:
> On June 26, 2013 5:02 AM Josh Kupershmidt wrote:
>>Thanks for the feedback. Attached is a rebased version of the patch with
> the two small issues noted fixed.

The following description in the document of pg_ctl needs to be modified?

restart might fail if relative paths specified were specified on
the command-line during server start.

+#define DATADIR_SPEC "\"-D\" \""
+
+ datadir = strstr(post_opts, DATADIR_SPEC);

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.

Just idea to work around that problem is to just append the specified -D option
and value to post_opts. IOW, -D option and value appear twice in post_opts.
In this case, posteriorly-located ones are used in the end. Thought?

Regards,

--
Fujii Masao

On Wed, Jun 26, 2013 at 12:22 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Wed, Jun 26, 2013 at 2:36 PM, Hari Babu <haribabu(dot)kommi(at)huawei(dot)com> wrote:
>> On June 26, 2013 5:02 AM Josh Kupershmidt wrote:
>>>Thanks for the feedback. Attached is a rebased version of the patch with
>> the two small issues noted fixed.
>
> The following description in the document of pg_ctl needs to be modified?
>
> restart might fail if relative paths specified were specified on
> the command-line during server start.

Right, that caveat could go away.

> +#define DATADIR_SPEC "\"-D\" \""
> +
> + datadir = strstr(post_opts, DATADIR_SPEC);
>
> 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?

> Just idea to work around that problem is to just append the specified -D option
> and value to post_opts. IOW, -D option and value appear twice in post_opts.
> In this case, posteriorly-located ones are used in the end. Thought?

Hrm, I think we'd have to be careful that postmaster.opts doesn't
accumulate an additional -D specification with every restart.

Josh

On Thu, Jun 27, 2013 at 10:36 AM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> On Wed, Jun 26, 2013 at 12:22 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Wed, Jun 26, 2013 at 2:36 PM, Hari Babu <haribabu(dot)kommi(at)huawei(dot)com> wrote:
>>> On June 26, 2013 5:02 AM Josh Kupershmidt wrote:
>>>>Thanks for the feedback. Attached is a rebased version of the patch with
>>> the two small issues noted fixed.
>>
>> The following description in the document of pg_ctl needs to be modified?
>>
>> restart might fail if relative paths specified were specified on
>> the command-line during server start.
>
> Right, that caveat could go away.
>
>> +#define DATADIR_SPEC "\"-D\" \""
>> +
>> + datadir = strstr(post_opts, DATADIR_SPEC);
>>
>> 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 :)

Another corner case is, for example, pg_ctl -D test1 -o "-D test2", ....
that is, multiple -D specifications appear in the command-line.

Can we overlook these cases?

>> Just idea to work around that problem is to just append the specified -D option
>> and value to post_opts. IOW, -D option and value appear twice in post_opts.
>> In this case, posteriorly-located ones are used in the end. Thought?
>
> Hrm, I think we'd have to be careful that postmaster.opts doesn't
> accumulate an additional -D specification with every restart.

Yes. Oh, I was thinking that postmaster writes only -D specification which
postmaster actually uses, in the opts file. So that accumulation would not
happen, I thought. But that's not true. Postmaster writes all the specified
arguments in the opts file.

Regards,

--
Fujii Masao

On Fri, Jun 28, 2013 at 12:47 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> Another corner case is, for example, pg_ctl -D test1 -o "-D test2", ....
> that is, multiple -D specifications appear in the command-line.

The patch handles this case properly. Sorry for noise..

Regards,

--
Fujii Masao

On Thu, Jun 27, 2013 at 11:47 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Thu, Jun 27, 2013 at 10:36 AM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
>> On Wed, Jun 26, 2013 at 12:22 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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.

Josh

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(dot)fujii(at)gmail(dot)com> wrote:
> > On Thu, Jun 27, 2013 at 10:36 AM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> >> On Wed, Jun 26, 2013 at 12:22 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

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

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

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