From: Luke Shumaker <lukeshu(a)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