Обсуждение: Non-portable shell code in pg_upgrade tap tests

Collegues,

I've discovered that in the branch REL_11_STABLE there is shell script
src/bin/pg_upgrade/test.sh which doesn't work under Solaris 10.
(it uses $(command) syntax with is not compatible with original
Solaris /bin/sh)

I was quite surprised that this problem goes unnoticed on big buildfarm,
but after some investigation found out that both Solaris machines in
that buildfarm don't configure postgres with --enable-tap-tests.

Offending code is:

# make sure all directories and files have group permissions, on Unix hosts
# Windows hosts don't support Unix-y permissions.
case $testhost in
    MINGW*) ;;
    *)  if [ $(find ${PGDATA} -type f ! -perm 640 | wc -l) -ne 0 ]; then
            echo "files in PGDATA with permission != 640";
            exit 1;
        fi ;;
esac

case $testhost in
    MINGW*) ;;
    *)  if [ $(find ${PGDATA} -type d ! -perm 750 | wc -l) -ne 0 ]; then
            echo "directories in PGDATA with permission != 750";
            exit 1;
        fi ;;
esac


It is quite easy to replace $() syntax with backticks. Backticks are
not nestable and considered unsafe by modern shell scripting style
guides, but they do work with older shells.

--

Victor Wagner <vitus@wagner.pp.ru> writes:
> I've discovered that in the branch REL_11_STABLE there is shell script
> src/bin/pg_upgrade/test.sh which doesn't work under Solaris 10.
> (it uses $(command) syntax with is not compatible with original
> Solaris /bin/sh)

OK ...

> It is quite easy to replace $() syntax with backticks. Backticks are
> not nestable and considered unsafe by modern shell scripting style
> guides, but they do work with older shells.

Please send a patch.  Most of us do not have access to old shells
we could test this on.  (The oldest machine I have, and it's old
enough to vote, does run that script ... I doubt very many other
developers have anything older.)

            regards, tom lane

On Fri, 20 Jul 2018 10:25:47 -0400
Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Victor Wagner <vitus@wagner.pp.ru> writes:
> > I've discovered that in the branch REL_11_STABLE there is shell
> > script src/bin/pg_upgrade/test.sh which doesn't work under Solaris
> > 10. (it uses $(command) syntax with is not compatible with original
> > Solaris /bin/sh)  

> 
> Please send a patch.  Most of us do not have access to old shells

Here it goes. Previous letter was written before fixed tests were
completed, because this old machine is slow.



> we could test this on.  (The oldest machine I have, and it's old
> enough to vote, does run that script ... I doubt very many other
> developers have anything older.)
> 
>             regards, tom lane

Victor Wagner <vitus@wagner.pp.ru> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Please send a patch.  Most of us do not have access to old shells

> Here it goes. Previous letter was written before fixed tests were
> completed, because this old machine is slow.

Thanks.  Will check on my own dinosaurs, and push if I don't find
a problem.

            regards, tom lane

Moin,

On Fri, July 20, 2018 10:55 am, Victor Wagner wrote:
> On Fri, 20 Jul 2018 10:25:47 -0400
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> Victor Wagner <vitus@wagner.pp.ru> writes:
>> > I've discovered that in the branch REL_11_STABLE there is shell
>> > script src/bin/pg_upgrade/test.sh which doesn't work under Solaris
>> > 10. (it uses $(command) syntax with is not compatible with original
>> > Solaris /bin/sh)
>
>>
>> Please send a patch.  Most of us do not have access to old shells
>
> Here it goes. Previous letter was written before fixed tests were
> completed, because this old machine is slow.


+    *)    if [ `find ${PGDATA} -type f ! -perm 640 | wc -l` -ne 0 ]; then

Shouldn't ${PGDATA} in the above as argument to find be quoted, otherwise
the shell would get confused if it contains spaces or other special
characters?

Regards,

Tels

"Tels" <nospam-pg-abuse@bloodgate.com> writes:
> +    *)    if [ `find ${PGDATA} -type f ! -perm 640 | wc -l` -ne 0 ]; then

> Shouldn't ${PGDATA} in the above as argument to find be quoted, otherwise
> the shell would get confused if it contains spaces or other special
> characters?

Hmm.  Yeah, probably.  I don't think this script is meant to be run with
arbitrary values of PGDATA, but most of the other uses are quoted, so
for consistency's sake this should be too.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> "Tels" <nospam-pg-abuse@bloodgate.com> writes:
>> +    *)    if [ `find ${PGDATA} -type f ! -perm 640 | wc -l` -ne 0 ]; then
>
>> Shouldn't ${PGDATA} in the above as argument to find be quoted, otherwise
>> the shell would get confused if it contains spaces or other special
>> characters?
>
> Hmm.  Yeah, probably.  I don't think this script is meant to be run with
> arbitrary values of PGDATA, but most of the other uses are quoted, so
> for consistency's sake this should be too.

PGDATA is built from `pwd`, so it breaks if the build directory has a
space in it. Because it's testing for the absence of files, it doesn't
actually break the test, but would fail to detect the bugs it's trying
to.

+ find /home/ilmari/src/post gresql/src/bin/pg_upgrade/tmp_check/data -type f ! -perm 640
+ wc -l
find: ‘/home/ilmari/src/post’: No such file or directory
find: ‘gresql/src/bin/pg_upgrade/tmp_check/data’: No such file or directory
+ [ 0 -ne 0 ]
+ find /home/ilmari/src/post gresql/src/bin/pg_upgrade/tmp_check/data -type d ! -perm 750
+ wc -l
find: ‘/home/ilmari/src/post’: No such file or directory
find: ‘gresql/src/bin/pg_upgrade/tmp_check/data’: No such file or directory
+ [ 0 -ne 0 ]

Attached is a patch fixing this.  I checked the rest of the script, and
this seems to be the only place lacking quoting.

- ilmari
-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."                              -- Skud's Meta-Law

From cabd43aa1988fb9f33743981266c9bf2278681a2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Sat, 21 Jul 2018 17:42:39 +0100
Subject: [PATCH] Quote ${PGDATA} in pg_upgrade/test.sh

---
 src/bin/pg_upgrade/test.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
index 775dd5729d..0e285c5c17 100644
--- a/src/bin/pg_upgrade/test.sh
+++ b/src/bin/pg_upgrade/test.sh
@@ -234,7 +234,7 @@ pg_upgrade $PG_UPGRADE_OPTS -d "${PGDATA}.old" -D "${PGDATA}" -b "$oldbindir" -B
 # Windows hosts don't support Unix-y permissions.
 case $testhost in
     MINGW*) ;;
-    *)    if [ `find ${PGDATA} -type f ! -perm 640 | wc -l` -ne 0 ]; then
+    *)    if [ `find "${PGDATA}" -type f ! -perm 640 | wc -l` -ne 0 ]; then
             echo "files in PGDATA with permission != 640";
             exit 1;
         fi ;;
@@ -242,7 +242,7 @@ esac
 
 case $testhost in
     MINGW*) ;;
-    *)    if [ `find ${PGDATA} -type d ! -perm 750 | wc -l` -ne 0 ]; then
+    *)    if [ `find "${PGDATA}" -type d ! -perm 750 | wc -l` -ne 0 ]; then
             echo "directories in PGDATA with permission != 750";
             exit 1;
         fi ;;
-- 
2.18.0

ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> Hmm.  Yeah, probably.  I don't think this script is meant to be run with
>> arbitrary values of PGDATA, but most of the other uses are quoted, so
>> for consistency's sake this should be too.

> Attached is a patch fixing this.  I checked the rest of the script, and
> this seems to be the only place lacking quoting.

Done already, but thanks.

            regards, tom lane

Moin,

On Sat, July 21, 2018 12:47 pm, Dagfinn Ilmari Mannsåker wrote:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>
>> "Tels" <nospam-pg-abuse@bloodgate.com> writes:
>>> +    *)    if [ `find ${PGDATA} -type f ! -perm 640 | wc -l` -ne 0 ]; then
>>
>>> Shouldn't ${PGDATA} in the above as argument to find be quoted,
>>> otherwise
>>> the shell would get confused if it contains spaces or other special
>>> characters?
>>
>> Hmm.  Yeah, probably.  I don't think this script is meant to be run with
>> arbitrary values of PGDATA, but most of the other uses are quoted, so
>> for consistency's sake this should be too.
>
> PGDATA is built from `pwd`, so it breaks if the build directory has a
> space in it. Because it's testing for the absence of files, it doesn't
> actually break the test, but would fail to detect the bugs it's trying
> to.

Thanx for the fix. But perhaps I should have been a bit more specific in
my first message, spaces are not the only character this can break at.

Looking at your new patch, I notice you used "" for quoting, not ''. (Not
sure which variant Tom used when pushing a patch).

I'm not a shell expert, but I think '' are safer, as "" still has some
interpolation from the shell (at least on the systems I use regulary):

 te@pc:~$ mkdir 'test$test'
 te@pc:~$ touch 'test$test/test'
 te@pc:~$ find 'test$test/test' -type f
 test$test/test
 te@pc:~$ find "test$test/test" -type f
 find: ‘test/test’: No such file or directory

So I've grown the habit to always use '' instead of "". Not sure if this
is specific to bash vs. sh or PC vs Mac, tho.

Best wishes,

Tels

"Tels" <nospam-pg-abuse@bloodgate.com> writes:
> Looking at your new patch, I notice you used "" for quoting, not ''. (Not
> sure which variant Tom used when pushing a patch).
> I'm not a shell expert, but I think '' are safer, as "" still has some
> interpolation from the shell (at least on the systems I use regulary):

We can't do that here because '' would suppress interpolation of the
variable's value, which is sort of the point.

AFAIK, the locution "$foo" is safe regardless of what is in $foo,
as long as only one pass of shell evaluation is involved.  The shell
will treat the value of $foo as one not-further-interpreted command
argument.

            regards, tom lane

On Fri, Jul 20, 2018 at 11:09:13AM -0400, Tom Lane wrote:
> Victor Wagner <vitus@wagner.pp.ru> writes:
> > Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Please send a patch.  Most of us do not have access to old shells
> 
> > Here it goes. Previous letter was written before fixed tests were
> > completed, because this old machine is slow.
> 
> Thanks.  Will check on my own dinosaurs, and push if I don't find
> a problem.

I'd say the right way to fix this is the one specified in
https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/The-Make-Macro-SHELL.html,
in particular:

  Using @SHELL@ means that your makefile will benefit from the same improved
  shell, such as bash or ksh, that was discovered during configure, so that
  you aren't fighting two different sets of shell bugs between the two
  contexts.

The Solaris 10 /bin/sh can't even run most of "configure", but Solaris 10 also
provides /bin/ksh and /usr/xpg4/bin/sh with the usual modern features.
("configure" works on Solaris 10 by finding a good shell and re-execing
itself.)

Maintaining shell scripts to an even lower common denominator than Autoconf
would be a good deal less reliable.

Noah Misch <noah@leadboat.com> writes:
> I'd say the right way to fix this is the one specified in
> https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/The-Make-Macro-SHELL.html,
> in particular:

>   Using @SHELL@ means that your makefile will benefit from the same improved
>   shell, such as bash or ksh, that was discovered during configure, so that
>   you aren't fighting two different sets of shell bugs between the two
>   contexts.

The pg_upgrade makefile does in fact use $(SHELL), so it will default to
whatever shell configure used.  However, I'd expect the Autoconf guys
to target a pretty darn low common denominator shell-wise.  It's unlikely
that the patches we just repaired were completely port-tested before
commit, and we now know that the buildfarm has been falling down on the
job as well.  (Partly my fault, since gaur/pademelon don't run the
TAP tests either.  I'm hesitant to triple their already long cycle
times, but ...)

The bigger picture here is that it's worth maintaining a common coding
style in this script.  Randomly using `...` in some places and $(...)
in others just increases the reader's cognitive load, as does spelling
conditional blocks in multiple styles.  So I'd have felt these changes
were appropriate on style grounds even if there were no portability
complaint.

In the long run, no doubt it'd be better to rewrite test.sh in Perl,
but this is what we've got today.

            regards, tom lane

On Sun, Jul 22, 2018 at 10:46:03AM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > I'd say the right way to fix this is the one specified in
> > https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/The-Make-Macro-SHELL.html,
> > in particular:
> 
> >   Using @SHELL@ means that your makefile will benefit from the same improved
> >   shell, such as bash or ksh, that was discovered during configure, so that
> >   you aren't fighting two different sets of shell bugs between the two
> >   contexts.
> 
> The pg_upgrade makefile does in fact use $(SHELL), so it will default to
> whatever shell configure used.

It will not, because we don't set $(SHELL) anywhere.  $(SHELL) is not @SHELL@.
In our makefiles, $(SHELL) is always /bin/sh, the GNU make default.

> The bigger picture here is that it's worth maintaining a common coding
> style in this script.  Randomly using `...` in some places and $(...)
> in others just increases the reader's cognitive load, as does spelling
> conditional blocks in multiple styles.  So I'd have felt these changes
> were appropriate on style grounds even if there were no portability
> complaint.

For 986127e narrowly, I agree.  However, I do want folks able to use $(...)
where it saves backslashes; that's more valuable than `...` uniformity.  For
commit 0426932, as the comment above the changed code indicates, the
conditional blocks are copies of "configure" code.  Consistency with their
source was more valuable than style, particularly since the code is
security-critical.

Noah Misch <noah@leadboat.com> writes:
> On Sun, Jul 22, 2018 at 10:46:03AM -0400, Tom Lane wrote:
>> The pg_upgrade makefile does in fact use $(SHELL), so it will default to
>> whatever shell configure used.

> It will not, because we don't set $(SHELL) anywhere.  $(SHELL) is not @SHELL@.
> In our makefiles, $(SHELL) is always /bin/sh, the GNU make default.

Oh!  Hm, I wonder whether we shouldn't do that, ie add SHELL = @SHELL@
to Makefile.global.in.

A quick trawl of the buildfarm logs says most of our animals compute
SHELL = /bin/sh anyway, and so would be unaffected.  There's a sizable
population that find /bin/bash though, and one active critter that finds
/bin/ksh.

            regards, tom lane

On Sun, Jul 22, 2018 at 02:53:51PM -0400, Tom Lane wrote:
> Oh!  Hm, I wonder whether we shouldn't do that, ie add SHELL = @SHELL@
> to Makefile.global.in.

That sounds like a good idea to me.

> A quick trawl of the buildfarm logs says most of our animals compute
> SHELL = /bin/sh anyway, and so would be unaffected.  There's a sizable
> population that find /bin/bash though, and one active critter that finds
> /bin/ksh.

Except for the FreeBSD boxes, right?  I thought that using directly
/bin/ and not /usr/local/bin/ was considered an abuse of Linux in their
universe.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Sun, Jul 22, 2018 at 02:53:51PM -0400, Tom Lane wrote:
>> A quick trawl of the buildfarm logs says most of our animals compute
>> SHELL = /bin/sh anyway, and so would be unaffected.  There's a sizable
>> population that find /bin/bash though, and one active critter that finds
>> /bin/ksh.

> Except for the FreeBSD boxes, right?  I thought that using directly
> /bin/ and not /usr/local/bin/ was considered an abuse of Linux in their
> universe.

No, I'm pretty sure that "sh" wins a place in /bin even on FreeBSD ;-)

            regards, tom lane

On Sun, Jul 22, 2018 at 02:53:51PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Sun, Jul 22, 2018 at 10:46:03AM -0400, Tom Lane wrote:
> >> The pg_upgrade makefile does in fact use $(SHELL), so it will default to
> >> whatever shell configure used.
> 
> > It will not, because we don't set $(SHELL) anywhere.  $(SHELL) is not @SHELL@.
> > In our makefiles, $(SHELL) is always /bin/sh, the GNU make default.
> 
> Oh!  Hm, I wonder whether we shouldn't do that, ie add SHELL = @SHELL@
> to Makefile.global.in.

I see that as the right way forward.