[pacman-dev] [PATCH] makepkg: Handle errors when canonicalizing paths

[Luke Shumaker]

From: Luke Shumaker <lukeshu at parabola.nu>

If you don't have adequate permission to `cd` to one of the DEST
directories, it's clear that that something's not doing error handling
right:

    $ install -d -m0000 srcdest
    $ SRCDEST=$PWD/srcdest makepkg
    /usr/share/makepkg/util/util.sh: line 75: cd: /.../srcdest: Permission denied
    ==> ERROR: Failed to change to directory /.../srcdest
        Aborting...
    ==> ERROR: Failed to create the directory $SRCDEST ().
        Aborting...

The first error comes from cd_safe() inside of canonicalize_path().
Because that bit runs in a subshell, it doesn't actually abort right there.
Then, makepkg.sh doesn't check for failures from canonicalize_path.  So
then the variable gets set to an empty string (which makes the next error
message from ensure_writable_dir() look funny), and keeps going.

As an additional "fun" quirk, canonicalize_path doesn't canonicalize it if
ensure_writable_dir has any work to do.  That's probably not intended, but
I'm not sure it is wrong either (see below).

 1. Combine canonicalize_path() and ensure_writable_dir() in to
    canonicalize_and_ensure_writable_dir().  Both are only ever called on
    exactly the same variables (except that ensure_writable_dir() might
    skip PKGDEST, SRCPKGDEST, or LOGDEST; depending on the flags).
 2. Use plain `cd` and check the status ourselves, rather than `cd_safe`.
    We can come up with a better (context-aware) error message than
    `cd_safe`, and we avoid duplicating the "Aborting..." message.

This has a few consequences:

 a. We no longer canonicalize dirpaths that we don't ensure are writable.
    That's probably safe.  This means that makepkg might now error in fewer
    cases; for example:

        $ install -d -m0000 pkgdest
        $ PKGDEST=$PWD/pkgdest makepkg -g

    There's no reason for it to care that it can't `cd` to PKGDEST; it
    won't be using it.

 b. We now canonicalize paths that we create that didn't exist before.

That said, I'm not sure why we ever need to canonicalize the paths; I'm not
sure why c0f58ea was necessary even after 960c2cd was already applied.
---
 scripts/libmakepkg/util/util.sh.in | 31 +++++++++++-------------------
 scripts/makepkg.sh.in              | 14 +++++++-------
 2 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/scripts/libmakepkg/util/util.sh.in b/scripts/libmakepkg/util/util.sh.in
index e1ca5cb7..1dfe6da9 100644
--- a/scripts/libmakepkg/util/util.sh.in
+++ b/scripts/libmakepkg/util/util.sh.in
@@ -49,20 +49,6 @@ is_array() {
 	return $ret
 }
 
-# Canonicalize a directory path if it exists
-canonicalize_path() {
-	local path="$1";
-
-	if [[ -d $path ]]; then
-		(
-			cd_safe "$path"
-			pwd -P
-		)
-	else
-		printf "%s\n" "$path"
-	fi
-}
-
 dir_is_empty() {
 	(
 		shopt -s dotglob nullglob
@@ -79,10 +65,10 @@ cd_safe() {
 	fi
 }
 
-# Try to create directory if one does not yet exist. Fails if the directory
-# exists but has no write permissions, or if there is an existing file with
-# the same name.
-ensure_writable_dir() {
+# Try to create directory if one does not yet exist, and prints the
+# canonical path to the directory. Fails if the directory exists but has no
+# write permissions, or if there is an existing file with the same name.
+canonicalize_and_ensure_writable_dir() {
 	local dirtype="$1" dirpath="$2"
 
 	if ! mkdir -p "$dirpath" 2>/dev/null; then
@@ -92,6 +78,11 @@ ensure_writable_dir() {
 		error "$(gettext "You do not have write permission for the directory \$%s (%s).")" "$dirtype" "$dirpath"
 		return 1
 	fi
-
-	return 0
+	(
+		if ! cd "$dirpath"; then
+			error "$(gettext "Could not canonicalize the directory \$%s (%s).")" "$dirtype" "$dirpath"
+			return 1
+		fi
+		pwd -P
+	)
 }
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index 8ee0f5c5..62c2db80 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -1353,9 +1353,9 @@ if (( ${#extra_environment[*]} )); then
 	export "${extra_environment[@]}"
 fi
 
-# canonicalize paths and provide defaults if anything is still undefined
+# provide defaults if anything is still undefined
 for var in PKGDEST SRCDEST SRCPKGDEST LOGDEST BUILDDIR; do
-	printf -v "$var" "$(canonicalize_path "${!var:-$startdir}")"
+	printf -v "$var" "${!var:-$startdir}"
 done
 unset var
 PACKAGER=${PACKAGER:-"Unknown Packager"}
@@ -1378,23 +1378,23 @@ lint_config || exit $E_CONFIG_ERROR
 
 
 # check that all settings directories are user-writable
-if ! ensure_writable_dir "BUILDDIR" "$BUILDDIR"; then
+if ! BUILDDIR=$(canonicalize_and_ensure_writable_dir "BUILDDIR" "$BUILDDIR"); then
 	plain "$(gettext "Aborting...")"
 	exit $E_FS_PERMISSIONS
 fi
 
-if (( ! (NOBUILD || GENINTEG) )) && ! ensure_writable_dir "PKGDEST" "$PKGDEST"; then
+if (( ! (NOBUILD || GENINTEG) )) && ! PKGDEST=$(canonicalize_and_ensure_writable_dir "PKGDEST" "$PKGDEST"); then
 	plain "$(gettext "Aborting...")"
 	exit $E_FS_PERMISSIONS
 fi
 
-if ! ensure_writable_dir "SRCDEST" "$SRCDEST" ; then
+if ! SRCDEST=$(canonicalize_and_ensure_writable_dir "SRCDEST" "$SRCDEST"); then
 	plain "$(gettext "Aborting...")"
 	exit $E_FS_PERMISSIONS
 fi
 
 if (( SOURCEONLY )); then
-	if ! ensure_writable_dir "SRCPKGDEST" "$SRCPKGDEST"; then
+	if ! SRCPKGDEST=$(canonicalize_and_ensure_writable_dir "SRCPKGDEST" "$SRCPKGDEST"); then
 		plain "$(gettext "Aborting...")"
 		exit $E_FS_PERMISSIONS
 	fi
@@ -1404,7 +1404,7 @@ if (( SOURCEONLY )); then
 	IGNOREARCH=1
 fi
 
-if (( LOGGING )) && ! ensure_writable_dir "LOGDEST" "$LOGDEST"; then
+if (( LOGGING )) && ! LOGDEST=$(canonicalize_and_ensure_writable_dir "LOGDEST" "$LOGDEST"); then
 	plain "$(gettext "Aborting...")"
 	exit $E_FS_PERMISSIONS
 fi
-- 
2.18.0