Patch: initdb: "'" for QUOTE_PATH (non-windows)

On Mon, Aug 15, 2016 at 12:12 AM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:

This is to fix an issue that came up for me when running initdb.

At the end of a successful initdb it says:

Success. You can now start the database server using:
pg_ctl -D /some/path/to/data -l logfile start

but this command did not work for me because my data directory
contained a space. The src/bin/initdb/initdb.c source code
did already have a QUOTE_PATH constant, but it was the empty
string for non-windows cases.

Therefore, added quotes via existing QUOTE_PATH constant:

pg_ctl -D '/some/path/to/data' -l logfile start

You may want to register this patch to the next commit fest:
https://commitfest.postgresql.org/10/
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Ok, I'll do that!
Thanks Michael!
Ryan

On Monday, August 15, 2016, Michael Paquier <michael.paquier@gmail.com>
wrote:

I've submitted my patch to Commitfest 2016-09.

https://commitfest.postgresql.org/10/718/

My username on postgresql.org is murftown

On Tue, Aug 16, 2016 at 1:02 AM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:

Hi,

On 2016-08-14 10:12:45 -0500, Ryan Murphy wrote:

Hello Postgres Team!
but this command did not work for me because my data directory
contained a space. The src/bin/initdb/initdb.c source code
did already have a QUOTE_PATH constant, but it was the empty
string for non-windows cases.

Therefore, added quotes via existing QUOTE_PATH constant:

pg_ctl -D '/some/path/to/data' -l logfile start

From 275d045bc41b136df8c413eedba12fbd21609de1 Mon Sep 17 00:00:00 2001
From: ryanfmurphy <ryanfmurphy@gmail.com>
Date: Sun, 14 Aug 2016 08:56:50 -0500
Subject: [PATCH] initdb: "'" for QUOTE_PATH (non-windows)

fix issue when running initdb

at the end of a successful initdb it says:

Success. You can now start the database server using:
pg_ctl -D /some/path/to/data -l logfile start

but this command will not work if the data directory contains a space
therefore, added quotes via existing QUOTE_PATH constant:

pg_ctl -D '/some/path/to/data' -l logfile start
---
src/bin/initdb/initdb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 73cb7ee..6dc1e23 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -332,7 +332,7 @@ do { \
} while (0)

#ifndef WIN32
-#define QUOTE_PATH ""
+#define QUOTE_PATH "'"
#define DIR_SEP "/"
#else
#define QUOTE_PATH "\""

ISTM that the correct fix would be to actually introduce something like
quote_path_for_shell() which either adds proper quotes, or fails if
that'd be hard (e.g. if the path contains quotes, and we're on
windows).

--
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, Aug 17, 2016 at 8:05 AM, Andres Freund <andres@anarazel.de> wrote:

ISTM that the correct fix would be to actually introduce something like
quote_path_for_shell() which either adds proper quotes, or fails if
that'd be hard (e.g. if the path contains quotes, and we're on
windows).

You are looking for appendShellString in fe_utils/string_utils.c.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

That makes sense, Michael and Andres.

I started to make a solution that uses a PQExpBuffer, appendShellString,
etc. I think it will work just fine, but I think I need to alter the
Makefile as well, to get initdb.c to be compiled using
-L../../../src/fe_utils -lpgfeutils. Otherwise I am having issues linking:

gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv
-Wno-unused-command-line-argument -O2 initdb.o findtimezone.o localtime.o
encnames.o -L../../../src/port -L../../../src/common
-Wl,-dead_strip_dylibs -lpgcommon -lpgport -lz -lreadline -lm -o initdb
Undefined symbols for architecture x86_64:
"_appendPQExpBufferStr", referenced from:
_main in initdb.o
"_appendShellString", referenced from:
_main in initdb.o
"_createPQExpBuffer", referenced from:
_main in initdb.o
"_destroyPQExpBuffer", referenced from:
_main in initdb.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see
invocation)

On Tue, Aug 16, 2016 at 10:00 PM, Michael Paquier <michael.paquier@gmail.com

I have created a better patch (attached) that correctly escapes the shell
arguments using PQExpBufferStr and the appendShellString function, as per
Michael and Andres' suggestions.

Further suggestions welcome of course.

Ryan

On Wed, Aug 17, 2016 at 8:28 AM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:

On Thu, Aug 18, 2016 at 12:21 AM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:

I have created a better patch (attached) that correctly escapes the shell
arguments using PQExpBufferStr and the appendShellString function, as per
Michael and Andres' suggestions.

Further suggestions welcome of course.

As far as I know, it is perfectly possible to have LF/CR in a path
name (that's bad practice btw...), and your patch would make initdb
fail in such cases. Do we want to authorize that? If we bypass the
error checks in appendShellString with an extra option, and have
initdb use that, the generated command would be actually correct.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

That's a fair point Michael. I would be willing to make such a change, but
since c doesn't have optional function arguments I'm not sure the least
intrusive way to do that. Do you have a suggestion?

On Wednesday, August 17, 2016, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Thu, Aug 18, 2016 at 9:30 AM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:
(please avoid top-posting)

As far as I know, it is perfectly possible to have LF/CR in a path
name (that's bad practice btw...), and your patch would make initdb
fail in such cases. Do we want to authorize that? If we bypass the
error checks in appendShellString with an extra option, and have
initdb use that, the generated command would be actually correct.

That's a fair point Michael. I would be willing to make such a change, but
since c doesn't have optional function arguments I'm not sure the least
intrusive way to do that. Do you have a suggestion?

You could just add a boolean flag to appendShellString called for
example no_error that the code path of initdb sets to true, and the
other ones to false.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2016-08-18 09:14:44 +0900, Michael Paquier wrote:

On Thu, Aug 18, 2016 at 12:21 AM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:

I have created a better patch (attached) that correctly escapes the shell
arguments using PQExpBufferStr and the appendShellString function, as per
Michael and Andres' suggestions.

Further suggestions welcome of course.

As far as I know, it is perfectly possible to have LF/CR in a path
name (that's bad practice btw...), and your patch would make initdb
fail in such cases. Do we want to authorize that?

I think that's actually a good thing to forbid.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I think that's actually a good thing to forbid.

I think I agree Andres, there are already comments in the appendShellString
function to this effect - they say that CR/LF chars in a file name are
mostly used for malicious hacking attempts anyways - I know I've hardly
ever needed a newline in a file name.

Did you see anything else in my code that you have recommendations about?
I made sure to free the PQExpBufferStr vars that I allocated.

Best,
Ryan

On Wed, Aug 17, 2016 at 7:41 PM, Andres Freund <andres@anarazel.de> wrote:

On Thu, Aug 18, 2016 at 11:35 AM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:

Be careful of top-posting, this is not this ML style:
http://www.idallen.com/topposting.html

I think that's actually a good thing to forbid.

I think I agree Andres, there are already comments in the appendShellString
function to this effect - they say that CR/LF chars in a file name are
mostly used for malicious hacking attempts anyways - I know I've hardly ever
needed a newline in a file name.

Did you see anything else in my code that you have recommendations about? I
made sure to free the PQExpBufferStr vars that I allocated.

+        { /* pg_ctl command w path, properly quoted */
+            PQExpBuffer pg_ctl_path = createPQExpBuffer();
+            printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
+                bin_dir,
+                (strlen(bin_dir) > 0) ? DIR_SEP : ""
+            );
+            appendShellString(start_db_cmd, pg_ctl_path->data);
+            destroyPQExpBuffer(pg_ctl_path);
+        }

This is not really project-style to have an independent block. Usually
those are controlled by for, while or if. And you could use the same
PQExpBuffer for everything.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On August 17, 2016 8:15:56 PM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:

+        { /* pg_ctl command w path, properly quoted */
+            PQExpBuffer pg_ctl_path = createPQExpBuffer();
+            printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
+                bin_dir,
+                (strlen(bin_dir) > 0) ? DIR_SEP : ""
+            );
+            appendShellString(start_db_cmd, pg_ctl_path->data);
+            destroyPQExpBuffer(pg_ctl_path);
+        }

This is not really project-style to have an independent block. Usually
those are controlled by for, while or if.

Besides the comment positioning I'd not say that that is against the usual style, there's a number of such blocks already. Don't think it's necessarily needed here though...
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

--
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, Aug 17, 2016 at 11:18 PM, Andres Freund <andres@anarazel.de> wrote:

On August 17, 2016 8:15:56 PM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:

+        { /* pg_ctl command w path, properly quoted */
+            PQExpBuffer pg_ctl_path = createPQExpBuffer();
+            printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
+                bin_dir,
+                (strlen(bin_dir) > 0) ? DIR_SEP : ""
+            );
+            appendShellString(start_db_cmd, pg_ctl_path->data);
+            destroyPQExpBuffer(pg_ctl_path);
+        }

This is not really project-style to have an independent block. Usually
those are controlled by for, while or if.

Besides the comment positioning I'd not say that that is against the usual style, there's a number of such blocks already. Don't think it's necessarily needed here though...

Really? I'd remove such blocks before committing anything, or ask for
them to be removed, unless there were some special reason for having
them.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2016-08-18 16:11:27 -0400, Robert Haas wrote:

On Wed, Aug 17, 2016 at 11:18 PM, Andres Freund <andres@anarazel.de> wrote:

On August 17, 2016 8:15:56 PM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:

+        { /* pg_ctl command w path, properly quoted */
+            PQExpBuffer pg_ctl_path = createPQExpBuffer();
+            printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
+                bin_dir,
+                (strlen(bin_dir) > 0) ? DIR_SEP : ""
+            );
+            appendShellString(start_db_cmd, pg_ctl_path->data);
+            destroyPQExpBuffer(pg_ctl_path);
+        }

This is not really project-style to have an independent block. Usually
those are controlled by for, while or if.

Besides the comment positioning I'd not say that that is against the usual style, there's a number of such blocks already. Don't think it's necessarily needed here though...

Really? I'd remove such blocks before committing anything, or ask for
them to be removed, unless there were some special reason for having
them.

Well, reducing the scope of variables *can* be such a reason, no? As I
said, I don't see any reason here, but in general, it's imo a reasonable
tool on one's belt.

--
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, Aug 18, 2016 at 4:15 PM, Andres Freund <andres@anarazel.de> wrote:

On 2016-08-18 16:11:27 -0400, Robert Haas wrote:

On Wed, Aug 17, 2016 at 11:18 PM, Andres Freund <andres@anarazel.de> wrote:

On August 17, 2016 8:15:56 PM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:

+        { /* pg_ctl command w path, properly quoted */
+            PQExpBuffer pg_ctl_path = createPQExpBuffer();
+            printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
+                bin_dir,
+                (strlen(bin_dir) > 0) ? DIR_SEP : ""
+            );
+            appendShellString(start_db_cmd, pg_ctl_path->data);
+            destroyPQExpBuffer(pg_ctl_path);
+        }

This is not really project-style to have an independent block. Usually
those are controlled by for, while or if.

Besides the comment positioning I'd not say that that is against the usual style, there's a number of such blocks already. Don't think it's necessarily needed here though...

Really? I'd remove such blocks before committing anything, or ask for
them to be removed, unless there were some special reason for having
them.

Well, reducing the scope of variables *can* be such a reason, no? As I
said, I don't see any reason here, but in general, it's imo a reasonable
tool on one's belt.

I think it's worth reducing the scope of variables when that's as
simple as putting them inside a block that you have to create anyway,
but I'm skeptical about the idea that one would create a block just to
reduce the scope of the variables. I don't think that's our usual
practice, and I would expect the compiler to detect where the variable
is referenced first and last anyway.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
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, Aug 18, 2016 at 3:22 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Aug 18, 2016 at 4:15 PM, Andres Freund <andres@anarazel.de> wrote:

On 2016-08-18 16:11:27 -0400, Robert Haas wrote:

On Wed, Aug 17, 2016 at 11:18 PM, Andres Freund <andres@anarazel.de>

wrote:

On August 17, 2016 8:15:56 PM PDT, Michael Paquier <

michael.paquier@gmail.com> wrote:

+        { /* pg_ctl command w path, properly quoted */
+            PQExpBuffer pg_ctl_path = createPQExpBuffer();
+            printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
+                bin_dir,
+                (strlen(bin_dir) > 0) ? DIR_SEP : ""
+            );
+            appendShellString(start_db_cmd, pg_ctl_path->data);
+            destroyPQExpBuffer(pg_ctl_path);
+        }

This is not really project-style to have an independent block. Usually
those are controlled by for, while or if.

Besides the comment positioning I'd not say that that is against the

usual style, there's a number of such blocks already. Don't think it's
necessarily needed here though...

Really? I'd remove such blocks before committing anything, or ask for
them to be removed, unless there were some special reason for having
them.

Well, reducing the scope of variables *can* be such a reason, no? As I
said, I don't see any reason here, but in general, it's imo a reasonable
tool on one's belt.

I think it's worth reducing the scope of variables when that's as
simple as putting them inside a block that you have to create anyway,
but I'm skeptical about the idea that one would create a block just to
reduce the scope of the variables. I don't think that's our usual
practice, and I would expect the compiler to detect where the variable
is referenced first and last anyway.

I'm can change my patch to take out that block.

I enjoy adding the blocks for explicit variable scoping and for quick
navigation in vim (the % key navigates between matching {}'s). But I want
to fit in with the style conventions of the project.

Before I change and resubmit my patch, are there any other changes, style
or otherwise, that you all would recommend?

Ryan Murphy <ryanfmurphy@gmail.com> writes:

On Thu, Aug 18, 2016 at 3:22 PM, Robert Haas <robertmhaas@gmail.com> wrote:

+        { /* pg_ctl command w path, properly quoted */
+            PQExpBuffer pg_ctl_path = createPQExpBuffer();
+            printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",

I think it's worth reducing the scope of variables when that's as
simple as putting them inside a block that you have to create anyway,
but I'm skeptical about the idea that one would create a block just to
reduce the scope of the variables. I don't think that's our usual
practice, and I would expect the compiler to detect where the variable
is referenced first and last anyway.

I enjoy adding the blocks for explicit variable scoping and for quick
navigation in vim (the % key navigates between matching {}'s). But I want
to fit in with the style conventions of the project.

Another point here is that code like this will look quite a bit different
after pgindent gets done with it --- that comment will not stay where
you put it, for example. Some of our worst formatting messes come from
code wherein somebody adhered to their own favorite layout style without
any thought for how it would play with pgindent.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers