Thread: Re: [COMMITTERS] pgsql: Make initdb's suggested "pg_ctl start" command line more reliabl

On 8/20/16 3:05 PM, Tom Lane wrote:
> Make initdb's suggested "pg_ctl start" command line more reliable.
> 
> The original coding here was not nearly careful enough about quoting
> special characters, and it didn't get corner cases right for constructing
> the pg_ctl path either.  Use join_path_components() and appendShellString()
> to do it honestly, so that the string will more likely work if blindly
> copied-and-pasted.
> 
> While at it, teach appendShellString() not to quote strings that clearly
> don't need it, so that the output from initdb doesn't become uglier than
> it was before in typical cases where quoting is not needed.

A couple of problems with this:

The not-quoting-if-not-needed doesn't appear to do anything useful for me:
   'pg-install/bin/pg_ctl' -D 'pg-install/var/data' -l logfile start

The indentation of that line was changed from 4 to 10.  I don't think
that was a good change.

As just mentioned elsewhere, this accidentally introduces a failure if
the PostgreSQL installation path contains LF/CR, because of the use of
appendShellString().

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 8/20/16 3:05 PM, Tom Lane wrote:
>> Make initdb's suggested "pg_ctl start" command line more reliable.

> A couple of problems with this:

> The not-quoting-if-not-needed doesn't appear to do anything useful for me:
>     'pg-install/bin/pg_ctl' -D 'pg-install/var/data' -l logfile start

Dash is considered a character that needs quoting.  It might be possible
to avoid that if we could be certain that appendShellString's output would
never be placed in a spot where it could be taken for a switch, but that
seems like a large assumption to me.  Or maybe we could treat it as
forcing quotes only if it starts the string; but I don't personally care
enough to write the code for that.  Feel free if you want to.

> The indentation of that line was changed from 4 to 10.  I don't think
> that was a good change.

Hmm, not intentional ... [ looks closely... ]  Looks like a tab got in
there somehow.  Will fix.

> As just mentioned elsewhere, this accidentally introduces a failure if
> the PostgreSQL installation path contains LF/CR, because of the use of
> appendShellString().

I think that's intentional, not accidental.  What actual use case is
there for allowing such paths?
        regards, tom lane

On Tue, Sep 6, 2016 at 2:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The not-quoting-if-not-needed doesn't appear to do anything useful for me:
>>     'pg-install/bin/pg_ctl' -D 'pg-install/var/data' -l logfile start
>
> Dash is considered a character that needs quoting.  It might be possible
> to avoid that if we could be certain that appendShellString's output would
> never be placed in a spot where it could be taken for a switch, but that
> seems like a large assumption to me.  Or maybe we could treat it as
> forcing quotes only if it starts the string; but I don't personally care
> enough to write the code for that.  Feel free if you want to.


Wouldn't it be taken for a switch even with quoting?

Quoting "-D" seems to work fine, which would suggest the above is true.

On 2016-09-06 13:08:51 -0400, Tom Lane wrote:
> Dash is considered a character that needs quoting.  It might be possible
> to avoid that if we could be certain that appendShellString's output would
> never be placed in a spot where it could be taken for a switch, but that
> seems like a large assumption to me.  Or maybe we could treat it as
> forcing quotes only if it starts the string; but I don't personally care
> enough to write the code for that.  Feel free if you want to.

But quotes are interpreted by the shell, not by the called program. So I
don't think putting --whatnot in quotes changes anything here?

Claudio Freire <klaussfreire@gmail.com> writes:
> On Tue, Sep 6, 2016 at 2:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Dash is considered a character that needs quoting.  It might be possible
>> to avoid that if we could be certain that appendShellString's output would
>> never be placed in a spot where it could be taken for a switch, but that
>> seems like a large assumption to me.

> Wouldn't it be taken for a switch even with quoting?
> Quoting "-D" seems to work fine, which would suggest the above is true.

[ thinks about that... ]  Oh, you're right, brain fade on my part.  The
shell doesn't care whether words are switches or not.  So actually the
risk-factor for us is whether we have designed any command-line syntaxes
in a way that would allow a path starting with a dash to cause bad things
to happen.  I have a feeling the answer is "yes", even without considering
the prospect that GNU getopt will arbitrarily rearrange the command words
on us depending on what it thinks is a switch.  (Maybe leading-dash is
another one of the things we'd better make a policy against.)

But meanwhile, yes, the argument for treating it as quotable in
appendShellString seems completely bogus.  I'll go change that.
        regards, tom lane

On 9/6/16 1:08 PM, Tom Lane wrote:
>> As just mentioned elsewhere, this accidentally introduces a failure if
>> > the PostgreSQL installation path contains LF/CR, because of the use of
>> > appendShellString().
> I think that's intentional, not accidental.  What actual use case is
> there for allowing such paths?

There probably isn't one.  But we ought to be introducing this change in
a more intentional and consistent way.

For example, pg_basebackup has no such restriction.  So using
pg_basebackup, then promote, then pg_upgrade will (probably) fail now
for some paths.

More generally, I'm concerned that appendShellString() looks pretty
attractive for future use.  It's not inconceivable that someone will
want to use it for say calling pg_dump from pg_dumpall or pg_upgrade at
some point, and then maybe we'll accidentally disallow LF/CR in
tablespace names, say.

Also, if we're concerned about the potential for confusion that these
characters can cause, maybe we should be disallowing more control
characters in similar places.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> More generally, I'm concerned that appendShellString() looks pretty
> attractive for future use.  It's not inconceivable that someone will
> want to use it for say calling pg_dump from pg_dumpall or pg_upgrade at
> some point, and then maybe we'll accidentally disallow LF/CR in
> tablespace names, say.

That's fair, but I'm not sure how you propose to avoid it, given that
we don't have a method for safely quoting those characters on Windows.

I would certainly far rather find a way to do that and then remove the
prohibition.  But lacking such a solution, we're going to be forced into
messy compromises.
        regards, tom lane