Re: [HACKERS] Fix initdb for path with whitespace and at char
On 05/01/2014 07:55 AM, Amit Kapila wrote: On Wed, Apr 30, 2014 at 4:01 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: I committed the non-invasive fixes to backbranches (and master too, just to keep it in sync), but the attached is what I came up with for master. There are a couple of places in the code where we have #ifdef WIN32 code that uses CreateProcess with CMD /C ... directly. 1. Do we need similar handling for CreatePipe case where it directly uses executable path such as in function pipe_read_line()? Currently the caller of pipe_read_line() calls canonicalize_path() to change win32 specific path, is that sufficient or do we need SYSTEMQUOTE type of handling for it. No, SYSTEMQUOTE style handling is not required with CreateProcess. find_other_exec, which is the only caller of pipe_read_line, adds one pair of double-quotes around the executable's path, which is enough for CreateProcess. 2. system.c #include assert.h Do we really need inclusion of assert.h or this is for future use? You're right, that's not needed. 3. One more observation is that currently install.bat doesn't work for such paths: install.bat e:\PostgreSQL\master\install 1\ins@1 1\ins@1== was unexpected at this time. Yeah, I noticed that. I haven't looked into what it would take to fix that, but for now you can just install to a path that doesn't contain whitespace, and move it from there. install.bat is only used by developers / packagers, so that's not a big deal. 4. Similar to Andrew, I also could not reproduce this problem on my Windows system (Windows 7 64 bit) e:\e:\PostgreSQL\master\install 1\ins@1\bin\initdb.exe -D e: \PostgreSQL\master\Data 1 e:\e:\PostgreSQL\master\install 1\ins@1\bin\pg_ctl.exe start -D e: \PostgreSQL\master\Data 1 Above commands work fine. Hmm, I'm also testing on 64-bit Windows 7, and it failed for me. Note that I already committed the narrow fix for initdb - which I also backpatched - to master, so to reproduce you'll need to revert that locally (commit 503de546). I fixed the issues with malloc that Tom pointed out and committed the wrapper functions to git master. But please let me know if there's still a problem with it. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix initdb for path with whitespace and at char
On Mon, May 5, 2014 at 6:38 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 05/01/2014 07:55 AM, Amit Kapila wrote: 4. Similar to Andrew, I also could not reproduce this problem on my Windows system (Windows 7 64 bit) e:\e:\PostgreSQL\master\install 1\ins@1\bin\initdb.exe -D e: \PostgreSQL\master\Data 1 e:\e:\PostgreSQL\master\install 1\ins@1\bin\pg_ctl.exe start -D e: \PostgreSQL\master\Data 1 Above commands work fine. Hmm, I'm also testing on 64-bit Windows 7, and it failed for me. Note that I already committed the narrow fix for initdb - which I also backpatched - to master, so to reproduce you'll need to revert that locally (commit 503de546). After reverting the specified commit, I could reproduce the issue and verified HEAD(it is fixed). With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix initdb for path with whitespace and at char
On 04/30/2014 07:39 AM, Amit Kapila wrote: On Wed, Apr 30, 2014 at 3:57 AM, Tom Lane t...@sss.pgh.pa.us wrote: Heikki Linnakangas hlinnakan...@vmware.com writes: This looks correct to me. popen() requires SYSTEMQUOTEs on Windows, like system() does. It seems right now SYSTEMQUOTE is used before popen both for Windows and non-Windows, ex. adjust_data_dir() in pg_ctl.c Yeah, but it's defined to an empty string on non-Windows platforms. We might forget to use the wrapper function too, if it has a nonstandard name, no? A better idea would be to redefine popen() and system() on Windows. It looks like we're already using a #define to redefine popen(). Right, that's exactly what I meant by a wrapper function. Won't defining variant of popen just for Windows to add SYSTEMQUOTE effect such (where currently it is used for both win and non-winows) usage? Also, I think we might want to remove use of SYSTEMQUOTE before popen calls where ever it is currently used to avoid usage of the same two times. Yep. I'll write up a patch to do that for git master. For back-branches, I just added the missing SYSTEMQUOTEs. There are a couple of places where changing the behavior might break existing installations. In particular, in the stable branches it's probably best to not add the SYSTEMQUOTEs to things like archive_command, restore_command, and COPY TO/FROM PROGRAM, where the command is specified by the user. Because people might already have added the extra quotes to the command to make it work, and if we suddenly start adding yet another pair of quotes, it will stop working. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix initdb for path with whitespace and at char
I committed the non-invasive fixes to backbranches (and master too, just to keep it in sync), but the attached is what I came up with for master. There are a couple of places in the code where we have #ifdef WIN32 code that uses CreateProcess with CMD /C ... directly. I believe those are currently (ie. before this patch) wrong for cygwin builds. SYSTEMQUOTE is defined as: #if defined(WIN32) !defined(__CYGWIN__) #define SYSTEMQUOTE \ #else #define SYSTEMQUOTE #endif I presume the !CYGWIN part is because cygwin version of system() and popen() don't require the extra quoting, because cygwin does that for us. But when we use CreateProcess directly, e.g like this in pg_ctl.c: snprintf(cmd, MAXPGPATH, CMD /C SYSTEMQUOTE \\%s\ %s%s \%s\ 21\ SYSTEMQUOTE, exec_path, pgdata_opt, post_opts, DEVNULL); if (!CreateRestrictedProcess(cmd, pi, false)) return GetLastError(); we would need the extra quotes, but SYSTEMQUOTE doesn't provide them with cygwin. Andrew: you have a cygwin installation, don't you? Could you test if pg_ctl start works when the binaries are installed to a path that contains both a space and an @ sign, like C:\white space\at@sign\install. I suspect it doesn't, but the attached patch fixes it. - Heikki From b00134385de28361194e7ba0a050343bb581e058 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas heikki.linnakan...@iki.fi Date: Wed, 30 Apr 2014 10:23:14 +0300 Subject: [PATCH] Replace SYSTEMQUOTEs with Windows-specific wrapper functions. It's easy to forget using SYSTEMQUOTEs whe constructing command strings for system() or popen(). We are currently missing it e.g. in COPY TO/FROM PROGRAM calls. Even if we fix all the places missing it now, it is bound to be forgotten again in the future. To eliminate the need for that, add wrapper functions that do the the extra quoting for you, and get rid of SYSTEMQUOTEs in all the callers. diff --git a/configure.in b/configure.in index fc9c52f..52357a6 100644 --- a/configure.in +++ b/configure.in @@ -1353,6 +1353,7 @@ if test $PORTNAME = win32; then AC_REPLACE_FUNCS(gettimeofday) AC_LIBOBJ(kill) AC_LIBOBJ(open) + AC_LIBOBJ(system) AC_LIBOBJ(win32env) AC_LIBOBJ(win32error) AC_LIBOBJ(win32setlocale) diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c index 56e912d..d22b6d3 100644 --- a/contrib/pg_upgrade/check.c +++ b/contrib/pg_upgrade/check.c @@ -970,7 +970,7 @@ get_bin_version(ClusterInfo *cluster) int pre_dot, post_dot; - snprintf(cmd, sizeof(cmd), SYSTEMQUOTE \%s/pg_ctl\ --version SYSTEMQUOTE, cluster-bindir); + snprintf(cmd, sizeof(cmd), \%s/pg_ctl\ --version, cluster-bindir); if ((output = popen(cmd, r)) == NULL || fgets(cmd_output, sizeof(cmd_output), output) == NULL) diff --git a/contrib/pg_upgrade/controldata.c b/contrib/pg_upgrade/controldata.c index fa0a005..476c6be 100644 --- a/contrib/pg_upgrade/controldata.c +++ b/contrib/pg_upgrade/controldata.c @@ -110,7 +110,7 @@ get_control_data(ClusterInfo *cluster, bool live_check) pg_putenv(LC_ALL, NULL); pg_putenv(LC_MESSAGES, C); - snprintf(cmd, sizeof(cmd), SYSTEMQUOTE \%s/%s \%s\ SYSTEMQUOTE, + snprintf(cmd, sizeof(cmd), \%s/%s \%s\, cluster-bindir, live_check ? pg_controldata\ : pg_resetxlog\ -n, cluster-pgdata); diff --git a/contrib/pg_upgrade/exec.c b/contrib/pg_upgrade/exec.c index 7f01301..91e66e6 100644 --- a/contrib/pg_upgrade/exec.c +++ b/contrib/pg_upgrade/exec.c @@ -59,14 +59,14 @@ static DWORD mainThreadId = 0; mainThreadId = GetCurrentThreadId(); #endif - written = strlcpy(cmd, SYSTEMQUOTE, sizeof(cmd)); + written = 0; va_start(ap, fmt); written += vsnprintf(cmd + written, MAXCMDLEN - written, fmt, ap); va_end(ap); if (written = MAXCMDLEN) pg_fatal(command too long\n); written += snprintf(cmd + written, MAXCMDLEN - written, - \%s\ 21 SYSTEMQUOTE, log_file); + \%s\ 21, log_file); if (written = MAXCMDLEN) pg_fatal(command too long\n); diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index b53fa8b..83b7f6e 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -1130,11 +1130,11 @@ test_config_settings(void) test_buffs = MIN_BUFS_FOR_CONNS(test_conns); snprintf(cmd, sizeof(cmd), - SYSTEMQUOTE \%s\ --boot -x0 %s + \%s\ --boot -x0 %s -c max_connections=%d -c shared_buffers=%d -c dynamic_shared_memory_type=none - \%s\ \%s\ 21 SYSTEMQUOTE, + \%s\ \%s\ 21, backend_exec, boot_options, test_conns, test_buffs, DEVNULL, DEVNULL); @@ -1165,11 +1165,11 @@ test_config_settings(void) } snprintf(cmd, sizeof(cmd), - SYSTEMQUOTE \%s\ --boot -x0 %s + \%s\ --boot -x0 %s -c max_connections=%d -c shared_buffers=%d -c dynamic_shared_memory_type=none - \%s\ \%s\ 21 SYSTEMQUOTE, + \%s\ \%s\ 21, backend_exec, boot_options, n_connections, test_buffs, DEVNULL, DEVNULL); @@ -1503,7 +1503,7 @@
Re: [HACKERS] Fix initdb for path with whitespace and at char
On 04/30/2014 06:31 AM, Heikki Linnakangas wrote: Andrew: you have a cygwin installation, don't you? Could you test if pg_ctl start works when the binaries are installed to a path that contains both a space and an @ sign, like C:\white space\at@sign\install. I suspect it doesn't, but the attached patch fixes it. I'll see what I can do. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix initdb for path with whitespace and at char
Heikki Linnakangas hlinnakan...@vmware.com writes: I committed the non-invasive fixes to backbranches (and master too, just to keep it in sync), but the attached is what I came up with for master. The malloc's in the new system.c file should be pg_malloc, or else have custom defenses against out-of-memory (possibly returning ENOMEM to the caller would be best?). Also, it seems like a good idea to save and restore errno across the ending free() calls. I don't know if Windows' version of free() can change errno, but we've definitely found that to be possible on other platforms. Looks good otherwise. 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
Re: [HACKERS] Fix initdb for path with whitespace and at char
On 04/30/2014 11:58 AM, Andrew Dunstan wrote: On 04/30/2014 06:31 AM, Heikki Linnakangas wrote: Andrew: you have a cygwin installation, don't you? Could you test if pg_ctl start works when the binaries are installed to a path that contains both a space and an @ sign, like C:\white space\at@sign\install. I suspect it doesn't, but the attached patch fixes it. I'll see what I can do. I tried git master like this: foo\ bar/a\@b/bin/initdb.exe data foo\ bar/a\@b/bin/pg_ctl.exe -D data/ -w start and didn't encounter a problem. Platform is Windows 8 Pro 64 bit, with latest 32 bit Cygwin. [ ... ] It looks like possibly the only time this will actually matter on Cygwin is when starting a service. Just running pg_ctl start from the command line works fine. But starting the service doesn't. I'll try the patch when I get a chance. cheers andrew cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix initdb for path with whitespace and at char
On 04/30/2014 03:03 PM, Andrew Dunstan wrote: On 04/30/2014 11:58 AM, Andrew Dunstan wrote: On 04/30/2014 06:31 AM, Heikki Linnakangas wrote: Andrew: you have a cygwin installation, don't you? Could you test if pg_ctl start works when the binaries are installed to a path that contains both a space and an @ sign, like C:\white space\at@sign\install. I suspect it doesn't, but the attached patch fixes it. I'll see what I can do. I tried git master like this: foo\ bar/a\@b/bin/initdb.exe data foo\ bar/a\@b/bin/pg_ctl.exe -D data/ -w start and didn't encounter a problem. Platform is Windows 8 Pro 64 bit, with latest 32 bit Cygwin. [ ... ] It looks like possibly the only time this will actually matter on Cygwin is when starting a service. Just running pg_ctl start from the command line works fine. But starting the service doesn't. I'll try the patch when I get a chance. No, there is something horribly wrong with the Cygwin service code. I don't have time now to sort it out. I suspect it's been broken for a very long time. I'm not even sure why we have it. Cygwin contains a wrapper (cygrunsrv) for setting up cygwin executables as services, so this is probably worse than redundant. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix initdb for path with whitespace and at char
On Wed, Apr 30, 2014 at 4:01 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: I committed the non-invasive fixes to backbranches (and master too, just to keep it in sync), but the attached is what I came up with for master. There are a couple of places in the code where we have #ifdef WIN32 code that uses CreateProcess with CMD /C ... directly. 1. Do we need similar handling for CreatePipe case where it directly uses executable path such as in function pipe_read_line()? Currently the caller of pipe_read_line() calls canonicalize_path() to change win32 specific path, is that sufficient or do we need SYSTEMQUOTE type of handling for it. 2. system.c #include assert.h Do we really need inclusion of assert.h or this is for future use? 3. One more observation is that currently install.bat doesn't work for such paths: install.bat e:\PostgreSQL\master\install 1\ins@1 1\ins@1== was unexpected at this time. 4. Similar to Andrew, I also could not reproduce this problem on my Windows system (Windows 7 64 bit) e:\e:\PostgreSQL\master\install 1\ins@1\bin\initdb.exe -D e: \PostgreSQL\master\Data 1 e:\e:\PostgreSQL\master\install 1\ins@1\bin\pg_ctl.exe start -D e: \PostgreSQL\master\Data 1 Above commands work fine. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Fix initdb for path with whitespace and at char
Hi,
On win32, initdb fails if it's path includes a space and at ('@')
character. E.g.
C:\C:\Program Files\user@company\Postgres\9.3\bin\initdb.exe -D c:\baz
'C:\Program' is not recognized as an internal or external command,
operable program or batch file.
Here's a patch that fixes initdb by enclosing the command string in
extra double quotes being passed to popen() (similar to system() call).
Thanks,
Nikhil
0001-Fix-initdb-for-path-with-whitespace-and-at-char.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix initdb for path with whitespace and at char
On 04/29/2014 09:14 PM, Nikhil Deshpande wrote:
On win32, initdb fails if it's path includes a space and at ('@')
character. E.g.
C:\C:\Program Files\user@company\Postgres\9.3\bin\initdb.exe -D c:\baz
'C:\Program' is not recognized as an internal or external command,
operable program or batch file.
Here's a patch that fixes initdb by enclosing the command string in
extra double quotes being passed to popen() (similar to system() call).
This looks correct to me. popen() requires SYSTEMQUOTEs on Windows, like
system() does. We already use SYSTEMQUOTEs in some popen() calls, like
in pg_ctl, but initdb is missing them. get_bin_version function in
pg_upgrade is also missing it, as is the popen() call in COPY TO/FROM
PROGRAM command.
Is there any situation where would *not* want to wrap the command in
SYSTEMQUOTEs? If there isn't, ISTM it would be better to create a
wrapper function, pgwin32_popen(), which adds the quotes instead of
sprinkling them into all callers. Even if we go around and fix all of
the callers now, we're bound to forget it again in the future.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix initdb for path with whitespace and at char
Heikki Linnakangas hlinnakan...@vmware.com writes: This looks correct to me. popen() requires SYSTEMQUOTEs on Windows, like system() does. We already use SYSTEMQUOTEs in some popen() calls, like in pg_ctl, but initdb is missing them. get_bin_version function in pg_upgrade is also missing it, as is the popen() call in COPY TO/FROM PROGRAM command. Yuck. Is there any situation where would *not* want to wrap the command in SYSTEMQUOTEs? If there isn't, ISTM it would be better to create a wrapper function, pgwin32_popen(), which adds the quotes instead of sprinkling them into all callers. Even if we go around and fix all of the callers now, we're bound to forget it again in the future. We might forget to use the wrapper function too, if it has a nonstandard name, no? A better idea would be to redefine popen() and system() on Windows. It looks like we're already using a #define to redefine popen(). This approach would let us get rid of nonstandard notation for this problem, instead of adding more. 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
Re: [HACKERS] Fix initdb for path with whitespace and at char
On Wed, Apr 30, 2014 at 3:57 AM, Tom Lane t...@sss.pgh.pa.us wrote: Heikki Linnakangas hlinnakan...@vmware.com writes: This looks correct to me. popen() requires SYSTEMQUOTEs on Windows, like system() does. It seems right now SYSTEMQUOTE is used before popen both for Windows and non-Windows, ex. adjust_data_dir() in pg_ctl.c We might forget to use the wrapper function too, if it has a nonstandard name, no? A better idea would be to redefine popen() and system() on Windows. It looks like we're already using a #define to redefine popen(). Won't defining variant of popen just for Windows to add SYSTEMQUOTE effect such (where currently it is used for both win and non-winows) usage? Also, I think we might want to remove use of SYSTEMQUOTE before popen calls where ever it is currently used to avoid usage of the same two times. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix initdb for path with whitespace and at char
Amit Kapila amit.kapil...@gmail.com writes: On Wed, Apr 30, 2014 at 3:57 AM, Tom Lane t...@sss.pgh.pa.us wrote: We might forget to use the wrapper function too, if it has a nonstandard name, no? A better idea would be to redefine popen() and system() on Windows. It looks like we're already using a #define to redefine popen(). Won't defining variant of popen just for Windows to add SYSTEMQUOTE effect such (where currently it is used for both win and non-winows) usage? Also, I think we might want to remove use of SYSTEMQUOTE before popen calls where ever it is currently used to avoid usage of the same two times. Well, yeah: the point would be to remove SYSTEMQUOTE altogether, probably. Certainly the idea I'm suggesting is that we don't need any Windows-specific notation at the call sites. 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
