From: ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=)
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: "Tels" <nospam-pg-abuse@bloodgate.com>,
  "Victor Wagner" <vitus@wagner.pp.ru>,
  "pgsql-hackers" <pgsql-hackers@postgresql.org>
Subject: Re: Non-portable shell code in pg_upgrade tap tests
References: <20180720153820.69e9ae6c@fafnir.local.vm>
	<17833.1532096747@sss.pgh.pa.us>
	<20180720175508.07776219@fafnir.local.vm>
	<c96303c04c360bbedaa04f90f515745b.squirrel@sm.webmail.pair.com>
	<4263.1532186967@sss.pgh.pa.us>
Mail-Copies-To: nobody
Date: Sat, 21 Jul 2018 17:47:00 +0100
In-Reply-To: <4263.1532186967@sss.pgh.pa.us> (Tom Lane's message of "Sat, 21
	Jul 2018 11:29:27 -0400")
Message-ID: <d8j60181qpn.fsf@dalvik.ping.uio.no>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.4 (gnu/linux)
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Precedence: bulk

--=-=-=
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

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


--=-=-=
Content-Type: text/x-diff
Content-Disposition: inline;
 filename=0001-Quote-PGDATA-in-pg_upgrade-test.sh.patch

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


--=-=-=--