fixing pg_ctl with relative paths
There have been some complaints[1]/messages/by-id/CAA-aLv72O+NegjAipHORmbqSMZTkZayaTxrd+C9v60YbmMmZUQ@mail.gmail.com[2]/messages/by-id/CAK3UJRGABxWSOCXnAsSYw5BfR4D9ageXF+6GtsRVm-LtfWfW=g@mail.gmail.com 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]Note, ps output and postmaster.opts will not include the datadir specification if you rely solely on the PGDATA environment variable for pg_ctl:
* 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]/messages/by-id/27233.1350234453@sss.pgh.pa.us 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]: /messages/by-id/CAA-aLv72O+NegjAipHORmbqSMZTkZayaTxrd+C9v60YbmMmZUQ@mail.gmail.com
[2]: /messages/by-id/CAK3UJRGABxWSOCXnAsSYw5BfR4D9ageXF+6GtsRVm-LtfWfW=g@mail.gmail.com
[3]: /messages/by-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
specification if you rely solely on the PGDATA environment variable
for pg_ctl
Attachments:
pgctl_paths.v01.diffapplication/octet-stream; name=pgctl_paths.v01.diffDownload
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
new file mode 100644
index e412d71..1aea485
*** a/src/bin/pg_ctl/pg_ctl.c
--- b/src/bin/pg_ctl/pg_ctl.c
*************** static void do_promote(void);
*** 134,139 ****
--- 134,140 ----
static void do_kill(pgpid_t pid);
static void print_msg(const char *msg);
static void adjust_data_dir(void);
+ static char *strip_datadirs(char *orig_post_opts);
#if defined(WIN32) || defined(__CYGWIN__)
static bool pgwin32_IsInstalled(SC_HANDLE);
*************** read_post_opts(void)
*** 719,724 ****
--- 720,726 ----
int len;
char *optline;
char *arg1;
+ char *orig_post_opts = NULL;
optline = optlines[0];
/* trim off line endings */
*************** read_post_opts(void)
*** 733,742 ****
{
*arg1 = '\0'; /* terminate so we get only program
* name */
! post_opts = arg1 + 1; /* point past whitespace */
}
if (exec_path == NULL)
exec_path = optline;
}
}
}
--- 735,748 ----
{
*arg1 = '\0'; /* terminate so we get only program
* name */
! orig_post_opts = arg1 + 1; /* point past whitespace */
}
if (exec_path == NULL)
exec_path = optline;
+
+ if (orig_post_opts) {
+ post_opts = strip_datadirs(orig_post_opts);
+ }
}
}
}
*************** do_start(void)
*** 819,826 ****
read_post_opts();
! /* No -D or -D already added during server start */
! if (ctl_command == RESTART_COMMAND || pgdata_opt == NULL)
pgdata_opt = "";
if (exec_path == NULL)
--- 825,831 ----
read_post_opts();
! if (pgdata_opt == NULL)
pgdata_opt = "";
if (exec_path == NULL)
*************** adjust_data_dir(void)
*** 1994,1999 ****
--- 1999,2065 ----
}
+ /*
+ * Remove any "-D" "/path/to/datadir" specifications. We don't want to
+ * preserve these during a restart, since the user will be providing
+ * a possibly-conflicting datadir which should take precedence over
+ * the old value(s). Get rid of all such datadir specifications in
+ * the string, since a user might have manually launched the postmaster
+ * with redundant specifications, e.g.
+ * postgres -D /some/dir/ -D /some/dir/ ...
+ *
+ * Note, the simple parsing here won't cope with path specifications
+ * containing embedded quotes, such as
+ * "-D" "/foo \" bar/"
+ * but such paths seem to be currently unsupported for other reasons
+ * anyway, since `pg_ctl start` / `pg_ctl stop` have inconsistent
+ * handling of such paths.
+ */
+ static char *
+ strip_datadirs(char *orig_post_opts)
+ {
+ char *datadir;
+ char *trailing_quote;
+ char *tmp = NULL;
+ char *post_opts = pg_strdup(orig_post_opts);
+
+ #define DATADIR_SPEC "\"-D\" \""
+
+ datadir = strstr(post_opts, DATADIR_SPEC);
+
+ while (datadir != NULL) {
+ trailing_quote = strchr(datadir + sizeof(DATADIR_SPEC), '"');
+ if (trailing_quote) {
+ *datadir = '\0';
+
+ /*
+ * If there are any options after the -D ...
+ * specification, preserve them by concatenating.
+ * post_opts must have enough space for strcat(),
+ * since it is the same size as post_opts, but with
+ * an '\0' inserted at *datadir.
+ */
+ if (*(trailing_quote + 1) != '\0') {
+ tmp = pg_strdup(trailing_quote + 1);
+ strcat(post_opts, tmp);
+ }
+
+ datadir = strstr(post_opts, DATADIR_SPEC);
+ }
+ else {
+ write_stderr(_("%s: unable to parse post_opts: %s"),
+ progname, orig_post_opts);
+ post_opts = orig_post_opts;
+ break;
+ }
+ }
+
+ free(tmp);
+ return post_opts;
+ }
+
+
+
int
main(int argc, char **argv)
{
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/ restartIMO 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' displaySpecifically, 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 ismainly 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]
/messages/by-id/CAA-aLv72O+NegjAipHORmbqSMZTkZayaTxrd+C
9v60YbmMmZUQ@mail.gmail.com
[2]
/messages/by-id/CAK3UJRGABxWSOCXnAsSYw5BfR4D9ageXF+6Gts
RVm-LtfWfW=g@mail.gmail.com
[3] /messages/by-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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jun 25, 2013 at 2:28 AM, Hari Babu <haribabu.kommi@huawei.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
Attachments:
pgctl_paths.v02.diffapplication/octet-stream; name=pgctl_paths.v02.diffDownload
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
new file mode 100644
index 9045e00..76acd3b
*** a/src/bin/pg_ctl/pg_ctl.c
--- b/src/bin/pg_ctl/pg_ctl.c
*************** static void do_promote(void);
*** 131,136 ****
--- 131,137 ----
static void do_kill(pgpid_t pid);
static void print_msg(const char *msg);
static void adjust_data_dir(void);
+ static char *strip_datadirs(char *orig_post_opts);
#if defined(WIN32) || defined(__CYGWIN__)
static bool pgwin32_IsInstalled(SC_HANDLE);
*************** read_post_opts(void)
*** 681,686 ****
--- 682,688 ----
int len;
char *optline;
char *arg1;
+ char *orig_post_opts = NULL;
optline = optlines[0];
/* trim off line endings */
*************** read_post_opts(void)
*** 695,704 ****
{
*arg1 = '\0'; /* terminate so we get only program
* name */
! post_opts = arg1 + 1; /* point past whitespace */
}
if (exec_path == NULL)
exec_path = optline;
}
}
}
--- 697,709 ----
{
*arg1 = '\0'; /* terminate so we get only program
* name */
! orig_post_opts = arg1 + 1; /* point past whitespace */
}
if (exec_path == NULL)
exec_path = optline;
+
+ if (orig_post_opts)
+ post_opts = strip_datadirs(orig_post_opts);
}
}
}
*************** do_start(void)
*** 781,788 ****
read_post_opts();
! /* No -D or -D already added during server start */
! if (ctl_command == RESTART_COMMAND || pgdata_opt == NULL)
pgdata_opt = "";
if (exec_path == NULL)
--- 786,792 ----
read_post_opts();
! if (pgdata_opt == NULL)
pgdata_opt = "";
if (exec_path == NULL)
*************** adjust_data_dir(void)
*** 1964,1969 ****
--- 1968,2034 ----
}
+ /*
+ * Remove any "-D" "/path/to/datadir" specifications. We don't want to
+ * preserve these during a restart, since the user will be providing
+ * a possibly-conflicting datadir which should take precedence over
+ * the old value(s). Get rid of all such datadir specifications in
+ * the string, since a user might have manually launched the postmaster
+ * with redundant specifications, e.g.
+ * postgres -D /some/dir/ -D /some/dir/ ...
+ *
+ * Note, the simple parsing here won't cope with path specifications
+ * containing embedded quotes, such as
+ * "-D" "/foo \" bar/"
+ * but such paths seem to be currently unsupported for other reasons
+ * anyway, since `pg_ctl start` / `pg_ctl stop` have inconsistent
+ * handling of such paths.
+ */
+ static char *
+ strip_datadirs(char *orig_post_opts)
+ {
+ char *datadir;
+ char *trailing_quote;
+ char *tmp = NULL;
+ char *post_opts = pg_strdup(orig_post_opts);
+
+ #define DATADIR_SPEC "\"-D\" \""
+
+ datadir = strstr(post_opts, DATADIR_SPEC);
+
+ while (datadir != NULL) {
+ trailing_quote = strchr(datadir + sizeof(DATADIR_SPEC), '"');
+ if (trailing_quote) {
+ *datadir = '\0';
+
+ /*
+ * If there are any options after the -D ...
+ * specification, preserve them by concatenating.
+ * post_opts must have enough space for strcat(),
+ * since it is the same size as post_opts, but with
+ * an '\0' inserted at *datadir.
+ */
+ if (*(trailing_quote + 1) != '\0') {
+ tmp = pg_strdup(trailing_quote + 1);
+ strcat(post_opts, tmp);
+ free(tmp);
+ }
+
+ datadir = strstr(post_opts, DATADIR_SPEC);
+ }
+ else {
+ write_stderr(_("%s: unable to parse post_opts: %s"),
+ progname, orig_post_opts);
+ post_opts = orig_post_opts;
+ break;
+ }
+ }
+
+ return post_opts;
+ }
+
+
+
int
main(int argc, char **argv)
{
Import Notes
Reply to msg id not found: 51c938b0.a7fe420a.14a6.ffffc75fSMTPIN_ADDED_BROKEN@mx.google.com
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jun 26, 2013 at 2:36 PM, Hari Babu <haribabu.kommi@huawei.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: 51ca7df7.c1dc0e0a.7e9c.01eeSMTPIN_ADDED_BROKEN@mx.google.com
On Wed, Jun 26, 2013 at 12:22 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Jun 26, 2013 at 2:36 PM, Hari Babu <haribabu.kommi@huawei.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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:
On Wed, Jun 26, 2013 at 2:36 PM, Hari Babu <haribabu.kommi@huawei.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jun 28, 2013 at 12:47 AM, Fujii Masao <masao.fujii@gmail.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
Josh
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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/ restartthat 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
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/ restartthat 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!
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/ restartthat 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-hackersHi, 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
Attachments:
PoC_prioritize_pgctl_D_on_restart.patchtext/x-patch; charset=us-asciiDownload
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)
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