From: Luke Shumaker <lukeshu@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