Обсуждение: 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

Heikki Linnakangas <hlinnakangas@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

On Wed, Apr 30, 2014 at 3:57 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Heikki Linnakangas <hlinnakangas@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

Amit Kapila <amit.kapila16@gmail.com> writes:
> On Wed, Apr 30, 2014 at 3:57 AM, Tom Lane <tgl@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

On 04/30/2014 07:39 AM, Amit Kapila wrote:
> On Wed, Apr 30, 2014 at 3:57 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Heikki Linnakangas <hlinnakangas@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

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\" 2>&1\"" 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

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

Heikki Linnakangas <hlinnakangas@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

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
>
>
>

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

On Wed, Apr 30, 2014 at 4:01 PM, Heikki Linnakangas
<hlinnakangas@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

On 05/01/2014 07:55 AM, Amit Kapila wrote:
> On Wed, Apr 30, 2014 at 4:01 PM, Heikki Linnakangas
> <hlinnakangas@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

On Mon, May 5, 2014 at 6:38 PM, Heikki Linnakangas
<hlinnakangas@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