[pacman-dev] [PATCH] makepkg: refactor checking for write permissions into a utility function
Additionally provide a separate error for the confusing if unlikely event that the user tries to use an existing file as a package output directory. This also means we now consistently try to create any nonexistent *DEST directories as needed before aborting with E_FS_PERMISSIONS. Previously only $BUILDDIR received that kindness. In the process, move the handling of $extra_environment[@] to where it always belonged, above $BUILDDIR. Fixes FS#43537 Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- Am I trying to do way, way too much here? mkdir will hopefully already provide a somewhat useful error message in the case of an existing file, as we don't quiet it. And this will need to be rebased on top of escondida's error codes patch scripts/libmakepkg/util/util.sh.in | 19 +++++++++++++++++++ scripts/makepkg.sh.in | 29 +++++++++++------------------ 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/scripts/libmakepkg/util/util.sh.in b/scripts/libmakepkg/util/util.sh.in index d676249d..a8813338 100644 --- a/scripts/libmakepkg/util/util.sh.in +++ b/scripts/libmakepkg/util/util.sh.in @@ -83,3 +83,22 @@ cd_safe() { exit 1 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_dir() { + local parent=$1 dirname=$1 + + until [[ -e $parent ]]; do + parent="${parent%/*}" + done + if [[ -f $parent ]]; then + error "$(gettext "Cannot create needed directory %s because it is already a file.")" "$parent" + return 1 + fi + if ( [[ ! -d $dirname ]] && ! mkdir -p "$dirname" ) || [[ ! -w $dirname ]]; then + return 1 + fi + return 0 +} diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 6449accd..b25e4106 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1359,32 +1359,25 @@ else unset ALL_OFF BOLD BLUE GREEN RED YELLOW fi +# override settings from extra variables on commandline, if any +if (( ${#extra_environment[*]} )); then + export "${extra_environment[@]}" +fi + # override settings with an environment variable for batch processing BUILDDIR=${_BUILDDIR:-$BUILDDIR} BUILDDIR=${BUILDDIR:-$startdir} #default to $startdir if undefined -if [[ ! -d $BUILDDIR ]]; then - if ! mkdir -p "$BUILDDIR"; then - error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR" - plain "$(gettext "Aborting...")" - exit 1 - fi - chmod a-s "$BUILDDIR" -fi -if [[ ! -w $BUILDDIR ]]; then +if ! ensure_dir "$BUILDDIR"; then error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR" plain "$(gettext "Aborting...")" exit 1 fi - -# override settings from extra variables on commandline, if any -if (( ${#extra_environment[*]} )); then - export "${extra_environment[@]}" -fi +chmod a-s "$BUILDDIR" PKGDEST=${_PKGDEST:-$PKGDEST} PKGDEST=${PKGDEST:-$startdir} #default to $startdir if undefined -if (( ! (NOBUILD || GENINTEG) )) && [[ ! -w $PKGDEST ]]; then +if (( ! (NOBUILD || GENINTEG) )) && ! ensure_dir "$PKGDEST"; then error "$(gettext "You do not have write permission to store packages in %s.")" "$PKGDEST" plain "$(gettext "Aborting...")" exit 1 @@ -1392,7 +1385,7 @@ fi SRCDEST=${_SRCDEST:-$SRCDEST} SRCDEST=${SRCDEST:-$startdir} #default to $startdir if undefined -if [[ ! -w $SRCDEST ]] ; then +if ! ensure_dir "$SRCDEST" ; then error "$(gettext "You do not have write permission to store downloads in %s.")" "$SRCDEST" plain "$(gettext "Aborting...")" exit 1 @@ -1401,7 +1394,7 @@ fi SRCPKGDEST=${_SRCPKGDEST:-$SRCPKGDEST} SRCPKGDEST=${SRCPKGDEST:-$startdir} #default to $startdir if undefined if (( SOURCEONLY )); then - if [[ ! -w $SRCPKGDEST ]]; then + if ! ensure_dir "$SRCPKGDEST"; then error "$(gettext "You do not have write permission to store source tarballs in %s.")" "$SRCPKGDEST" plain "$(gettext "Aborting...")" exit 1 @@ -1414,7 +1407,7 @@ fi LOGDEST=${_LOGDEST:-$LOGDEST} LOGDEST=${LOGDEST:-$startdir} #default to $startdir if undefined -if (( LOGGING )) && [[ ! -w $LOGDEST ]]; then +if (( LOGGING )) && ! ensure_dir "$LOGDEST"; then error "$(gettext "You do not have write permission to store logs in %s.")" "$LOGDEST" plain "$(gettext "Aborting...")" exit 1 -- 2.14.2
On 08/10/17 17:05, Eli Schwartz wrote:
Additionally provide a separate error for the confusing if unlikely event that the user tries to use an existing file as a package output directory.
This also means we now consistently try to create any nonexistent *DEST directories as needed before aborting with E_FS_PERMISSIONS. Previously only $BUILDDIR received that kindness.
In the process, move the handling of $extra_environment[@] to where it always belonged, above $BUILDDIR.
This is a separate change and should be a separate patch. (Does it need to go even higher?)
Fixes FS#43537
Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> ---
Am I trying to do way, way too much here? mkdir will hopefully already provide a somewhat useful error message in the case of an existing file, as we don't quiet it.
(added after review below). The more I look at this, the more I think the patch is doing too much. Letting mkdir error and then giving the makepkg error afterward is probably enough here. The extra may be error prone. Although mkdir gives a really bad error when a file exists that conflicts with the path to be created...
And this will need to be rebased on top of escondida's error codes patch
scripts/libmakepkg/util/util.sh.in | 19 +++++++++++++++++++ scripts/makepkg.sh.in | 29 +++++++++++------------------ 2 files changed, 30 insertions(+), 18 deletions(-)
diff --git a/scripts/libmakepkg/util/util.sh.in b/scripts/libmakepkg/util/util.sh.in index d676249d..a8813338 100644 --- a/scripts/libmakepkg/util/util.sh.in +++ b/scripts/libmakepkg/util/util.sh.in @@ -83,3 +83,22 @@ cd_safe() { exit 1 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_dir() {
ensure_writable_dir() Every time I read "ensure_dir", I had to think what was ensured!
+ local parent=$1 dirname=$1 + + until [[ -e $parent ]]; do + parent="${parent%/*}" + done + if [[ -f $parent ]]; then
Is that a strong enough test? Isn't "! -d $parent" what is really wanted? This might be too strong (symlink to directory would fail).
+ error "$(gettext "Cannot create needed directory %s because it is already a file.")" "$parent" + return 1 + fi
I think we can change the error message to be more succinct: "$(gettext "Cannot create %s due to conflicting file.")" "$dirname"
+ if ( [[ ! -d $dirname ]] && ! mkdir -p "$dirname" ) || [[ ! -w $dirname ]]; then + return 1 + fi + return 0 +} diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 6449accd..b25e4106 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1359,32 +1359,25 @@ else unset ALL_OFF BOLD BLUE GREEN RED YELLOW fi
+# override settings from extra variables on commandline, if any +if (( ${#extra_environment[*]} )); then + export "${extra_environment[@]}" +fi +
# override settings with an environment variable for batch processing BUILDDIR=${_BUILDDIR:-$BUILDDIR} BUILDDIR=${BUILDDIR:-$startdir} #default to $startdir if undefined -if [[ ! -d $BUILDDIR ]]; then - if ! mkdir -p "$BUILDDIR"; then - error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR" - plain "$(gettext "Aborting...")" - exit 1 - fi - chmod a-s "$BUILDDIR" -fi -if [[ ! -w $BUILDDIR ]]; then +if ! ensure_dir "$BUILDDIR"; then error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR" plain "$(gettext "Aborting...")" exit 1 fi - -# override settings from extra variables on commandline, if any -if (( ${#extra_environment[*]} )); then - export "${extra_environment[@]}" -fi +chmod a-s "$BUILDDIR"
PKGDEST=${_PKGDEST:-$PKGDEST} PKGDEST=${PKGDEST:-$startdir} #default to $startdir if undefined -if (( ! (NOBUILD || GENINTEG) )) && [[ ! -w $PKGDEST ]]; then +if (( ! (NOBUILD || GENINTEG) )) && ! ensure_dir "$PKGDEST"; then error "$(gettext "You do not have write permission to store packages in %s.")" "$PKGDEST" plain "$(gettext "Aborting...")" exit 1 @@ -1392,7 +1385,7 @@ fi
SRCDEST=${_SRCDEST:-$SRCDEST} SRCDEST=${SRCDEST:-$startdir} #default to $startdir if undefined -if [[ ! -w $SRCDEST ]] ; then +if ! ensure_dir "$SRCDEST" ; then error "$(gettext "You do not have write permission to store downloads in %s.")" "$SRCDEST" plain "$(gettext "Aborting...")" exit 1 @@ -1401,7 +1394,7 @@ fi SRCPKGDEST=${_SRCPKGDEST:-$SRCPKGDEST} SRCPKGDEST=${SRCPKGDEST:-$startdir} #default to $startdir if undefined if (( SOURCEONLY )); then - if [[ ! -w $SRCPKGDEST ]]; then + if ! ensure_dir "$SRCPKGDEST"; then error "$(gettext "You do not have write permission to store source tarballs in %s.")" "$SRCPKGDEST" plain "$(gettext "Aborting...")" exit 1 @@ -1414,7 +1407,7 @@ fi
LOGDEST=${_LOGDEST:-$LOGDEST} LOGDEST=${LOGDEST:-$startdir} #default to $startdir if undefined -if (( LOGGING )) && [[ ! -w $LOGDEST ]]; then +if (( LOGGING )) && ! ensure_dir "$LOGDEST"; then error "$(gettext "You do not have write permission to store logs in %s.")" "$LOGDEST" plain "$(gettext "Aborting...")" exit 1
On 10/08/2017 11:12 AM, Allan McRae wrote:
On 08/10/17 17:05, Eli Schwartz wrote:
Additionally provide a separate error for the confusing if unlikely event that the user tries to use an existing file as a package output directory.
This also means we now consistently try to create any nonexistent *DEST directories as needed before aborting with E_FS_PERMISSIONS. Previously only $BUILDDIR received that kindness.
In the process, move the handling of $extra_environment[@] to where it always belonged, above $BUILDDIR.
This is a separate change and should be a separate patch. (Does it need to go even higher?)
In fact it does. Actually, I reworked the order altogether, since it used to happen after BUILDDIR permissions was checked and before anything else was either restored or checked, but really should happen after all variables are restored and before checking anything.
Fixes FS#43537
Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> ---
Am I trying to do way, way too much here? mkdir will hopefully already provide a somewhat useful error message in the case of an existing file, as we don't quiet it.
(added after review below). The more I look at this, the more I think the patch is doing too much. Letting mkdir error and then giving the makepkg error afterward is probably enough here. The extra may be error prone.
Although mkdir gives a really bad error when a file exists that conflicts with the path to be created...
That really bad error is the only real reason I bothered, and I'm anyways okay with being convinced to drop that logic altogether. But that mkdir error really is kind of gross.
And this will need to be rebased on top of escondida's error codes patch
scripts/libmakepkg/util/util.sh.in | 19 +++++++++++++++++++ scripts/makepkg.sh.in | 29 +++++++++++------------------ 2 files changed, 30 insertions(+), 18 deletions(-)
diff --git a/scripts/libmakepkg/util/util.sh.in b/scripts/libmakepkg/util/util.sh.in index d676249d..a8813338 100644 --- a/scripts/libmakepkg/util/util.sh.in +++ b/scripts/libmakepkg/util/util.sh.in @@ -83,3 +83,22 @@ cd_safe() { exit 1 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_dir() {
ensure_writable_dir()
Every time I read "ensure_dir", I had to think what was ensured!
That there *is* one :p though I guess I see your confusion. What I regard as logical may not be readily apparent to the rest of the world, so I guess I will humor the rest of the world and change this. :)
+ local parent=$1 dirname=$1 + + until [[ -e $parent ]]; do + parent="${parent%/*}" + done + if [[ -f $parent ]]; then
Is that a strong enough test? Isn't "! -d $parent" what is really wanted? This might be too strong (symlink to directory would fail).
``` $ ln -s /tmp tmp $ if [[ -f tmp ]]; then echo "symlink to dir is recognized as a file"; else echo "symlink to dir is not recognized as a file"; fi symlink to dir is not recognized as a file ``` Seems to work fine??? -- Eli Schwartz
The extra variables on the commandline were inconsistently applied. They should override anything else, instead, most were overridden by environment variables with the exception of BUILDDIR (and this was not sanity-checked to see if it had write permissions). e.g. given the commandline: `PKGDEST="$(pwd)"` BUILDDIR="$(pwd)" makepkg PKGDEST=/doesnt/exist BUILDDIR=/doesnt/exist` We would incorrectly use the current working directory for PKGDEST. Meanwhile, we checked the wrong directory for BUILDDIR, and later errored when we tried to create $srcdir inside the non-writable directory "/doesnt/exist". In order to fix this, use the preferred bash builtin for saving variable definitions, similar to how we restore traps etc. rather than tediously redefining each one by hand, and restore this immediately after makepkg.conf is sourced. Finally, the `make`-style commandline overrides are applied. Also canonicalize_path is applied only on the final paths we try to use. While it is unlikely the value in makepkg.conf will be a relative path, since we now properly respect commandline overrides, they should be canonicalized as well. Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- scripts/makepkg.sh.in | 50 ++++++++++++++++---------------------------------- 1 file changed, 16 insertions(+), 34 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 462ee4bb..20cc22ad 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1307,17 +1307,8 @@ done trap 'trap_exit INT "$(gettext "Aborted by user! Exiting...")"' INT trap 'trap_exit USR1 "$(gettext "An unknown error has occurred. Exiting...")"' ERR -# preserve environment variables and canonicalize path -[[ -n ${PKGDEST} ]] && _PKGDEST=$(canonicalize_path ${PKGDEST}) -[[ -n ${SRCDEST} ]] && _SRCDEST=$(canonicalize_path ${SRCDEST}) -[[ -n ${SRCPKGDEST} ]] && _SRCPKGDEST=$(canonicalize_path ${SRCPKGDEST}) -[[ -n ${LOGDEST} ]] && _LOGDEST=$(canonicalize_path ${LOGDEST}) -[[ -n ${BUILDDIR} ]] && _BUILDDIR=$(canonicalize_path ${BUILDDIR}) -[[ -n ${PKGEXT} ]] && _PKGEXT=${PKGEXT} -[[ -n ${SRCEXT} ]] && _SRCEXT=${SRCEXT} -[[ -n ${GPGKEY} ]] && _GPGKEY=${GPGKEY} -[[ -n ${PACKAGER} ]] && _PACKAGER=${PACKAGER} -[[ -n ${CARCH} ]] && _CARCH=${CARCH} +# preserve environment variables to override makepkg.conf +restore_envvars=$(declare -p PKGDEST SRCDEST SRCPKGDEST LOGDEST BUILDDIR PKGEXT SRCEXT GPGKEY PACKAGER CARCH 2>/dev/null || true) # default config is makepkg.conf MAKEPKG_CONF=${MAKEPKG_CONF:-$confdir/makepkg.conf} @@ -1342,6 +1333,20 @@ if [[ "$MAKEPKG_CONF" = "$confdir/makepkg.conf" ]]; then fi fi +eval "$restore_envvars" + +# override settings from extra variables on commandline, if any +if (( ${#extra_environment[*]} )); then + export "${extra_environment[@]}" +fi + +# canonicalize paths and provide defaults if anything is still undefined +for var in PKGDEST SRCDEST SRCPKGDEST LOGDEST BUILDDIR; do + printf -v "$var" "$(canonicalize_path "${!var:-$startdir}")" +done +unset var +PACKAGER=${PACKAGER:-"Unknown Packager"} + # set pacman command if not already defined PACMAN=${PACMAN:-pacman} # save full path to command as PATH may change when sourcing /etc/profile @@ -1355,9 +1360,6 @@ else fi -# override settings with an environment variable for batch processing -BUILDDIR=${_BUILDDIR:-$BUILDDIR} -BUILDDIR=${BUILDDIR:-$startdir} #default to $startdir if undefined if [[ ! -d $BUILDDIR ]]; then if ! mkdir -p "$BUILDDIR"; then error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR" @@ -1372,29 +1374,18 @@ if [[ ! -w $BUILDDIR ]]; then exit 1 fi -# override settings from extra variables on commandline, if any -if (( ${#extra_environment[*]} )); then - export "${extra_environment[@]}" -fi - -PKGDEST=${_PKGDEST:-$PKGDEST} -PKGDEST=${PKGDEST:-$startdir} #default to $startdir if undefined if (( ! (NOBUILD || GENINTEG) )) && [[ ! -w $PKGDEST ]]; then error "$(gettext "You do not have write permission to store packages in %s.")" "$PKGDEST" plain "$(gettext "Aborting...")" exit 1 fi -SRCDEST=${_SRCDEST:-$SRCDEST} -SRCDEST=${SRCDEST:-$startdir} #default to $startdir if undefined if [[ ! -w $SRCDEST ]] ; then error "$(gettext "You do not have write permission to store downloads in %s.")" "$SRCDEST" plain "$(gettext "Aborting...")" exit 1 fi -SRCPKGDEST=${_SRCPKGDEST:-$SRCPKGDEST} -SRCPKGDEST=${SRCPKGDEST:-$startdir} #default to $startdir if undefined if (( SOURCEONLY )); then if [[ ! -w $SRCPKGDEST ]]; then error "$(gettext "You do not have write permission to store source tarballs in %s.")" "$SRCPKGDEST" @@ -1407,21 +1398,12 @@ if (( SOURCEONLY )); then IGNOREARCH=1 fi -LOGDEST=${_LOGDEST:-$LOGDEST} -LOGDEST=${LOGDEST:-$startdir} #default to $startdir if undefined if (( LOGGING )) && [[ ! -w $LOGDEST ]]; then error "$(gettext "You do not have write permission to store logs in %s.")" "$LOGDEST" plain "$(gettext "Aborting...")" exit 1 fi -PKGEXT=${_PKGEXT:-$PKGEXT} -SRCEXT=${_SRCEXT:-$SRCEXT} -GPGKEY=${_GPGKEY:-$GPGKEY} -PACKAGER=${_PACKAGER:-$PACKAGER} -PACKAGER=${PACKAGER:-"Unknown Packager"} #default if undefined -CARCH=${_CARCH:-$CARCH} - if (( ! INFAKEROOT )); then if (( EUID == 0 )); then error "$(gettext "Running %s as root is not allowed as it can cause permanent,\n\ -- 2.15.0
Additionally provide a separate error for the confusing if unlikely event that the user tries to use an existing file as a package output directory. This also means we now consistently try to create any nonexistent *DEST directories as needed before aborting with E_FS_PERMISSIONS. Previously only $BUILDDIR received that kindness. Fixes FS#43537 Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- mkdir -p has a really bad error when a file exists that conflicts with the path to be created. But I'm still not sure whether to include that code in ensure_writable_dir scripts/libmakepkg/util/util.sh.in | 19 +++++++++++++++++++ scripts/makepkg.sh.in | 19 ++++++------------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/scripts/libmakepkg/util/util.sh.in b/scripts/libmakepkg/util/util.sh.in index d676249d..8a6149ed 100644 --- a/scripts/libmakepkg/util/util.sh.in +++ b/scripts/libmakepkg/util/util.sh.in @@ -83,3 +83,22 @@ cd_safe() { exit 1 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() { + local parent=$1 dirname=$1 + + until [[ -e $parent ]]; do + parent="${parent%/*}" + done + if [[ -f $parent ]]; then + error "$(gettext "Cannot create %s due to conflicting file.")" "$parent" + return 1 + fi + if ( [[ ! -d $dirname ]] && ! mkdir -p "$dirname" ) || [[ ! -w $dirname ]]; then + return 1 + fi + return 0 +} diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 20cc22ad..35665f16 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1360,34 +1360,27 @@ else fi -if [[ ! -d $BUILDDIR ]]; then - if ! mkdir -p "$BUILDDIR"; then - error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR" - plain "$(gettext "Aborting...")" - exit 1 - fi - chmod a-s "$BUILDDIR" -fi -if [[ ! -w $BUILDDIR ]]; then +# check that all settings directories are user-writable +if ! ensure_writable_dir "$BUILDDIR"; then error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR" plain "$(gettext "Aborting...")" exit 1 fi -if (( ! (NOBUILD || GENINTEG) )) && [[ ! -w $PKGDEST ]]; then +if (( ! (NOBUILD || GENINTEG) )) && ! ensure_writable_dir "$PKGDEST"; then error "$(gettext "You do not have write permission to store packages in %s.")" "$PKGDEST" plain "$(gettext "Aborting...")" exit 1 fi -if [[ ! -w $SRCDEST ]] ; then +if ! ensure_writable_dir "$SRCDEST" ; then error "$(gettext "You do not have write permission to store downloads in %s.")" "$SRCDEST" plain "$(gettext "Aborting...")" exit 1 fi if (( SOURCEONLY )); then - if [[ ! -w $SRCPKGDEST ]]; then + if ! ensure_writable_dir "$SRCPKGDEST"; then error "$(gettext "You do not have write permission to store source tarballs in %s.")" "$SRCPKGDEST" plain "$(gettext "Aborting...")" exit 1 @@ -1398,7 +1391,7 @@ if (( SOURCEONLY )); then IGNOREARCH=1 fi -if (( LOGGING )) && [[ ! -w $LOGDEST ]]; then +if (( LOGGING )) && ! ensure_writable_dir "$LOGDEST"; then error "$(gettext "You do not have write permission to store logs in %s.")" "$LOGDEST" plain "$(gettext "Aborting...")" exit 1 -- 2.15.0
On 31/10/17 04:03, Eli Schwartz wrote:
Additionally provide a separate error for the confusing if unlikely event that the user tries to use an existing file as a package output directory.
This also means we now consistently try to create any nonexistent *DEST directories as needed before aborting with E_FS_PERMISSIONS. Previously only $BUILDDIR received that kindness.
Fixes FS#43537
Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> ---
mkdir -p has a really bad error when a file exists that conflicts with the path to be created. But I'm still not sure whether to include that code in ensure_writable_dir
scripts/libmakepkg/util/util.sh.in | 19 +++++++++++++++++++ scripts/makepkg.sh.in | 19 ++++++------------- 2 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/scripts/libmakepkg/util/util.sh.in b/scripts/libmakepkg/util/util.sh.in index d676249d..8a6149ed 100644 --- a/scripts/libmakepkg/util/util.sh.in +++ b/scripts/libmakepkg/util/util.sh.in @@ -83,3 +83,22 @@ cd_safe() { exit 1 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() { + local parent=$1 dirname=$1 + + until [[ -e $parent ]]; do + parent="${parent%/*}" + done + if [[ -f $parent ]]; then + error "$(gettext "Cannot create %s due to conflicting file.")" "$parent" + return 1 + fi + if ( [[ ! -d $dirname ]] && ! mkdir -p "$dirname" ) || [[ ! -w $dirname ]]; then + return 1 + fi + return 0 +} diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 20cc22ad..35665f16 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1360,34 +1360,27 @@ else fi
-if [[ ! -d $BUILDDIR ]]; then - if ! mkdir -p "$BUILDDIR"; then - error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR" - plain "$(gettext "Aborting...")" - exit 1 - fi - chmod a-s "$BUILDDIR"
As discussed on IRC, this chmod is important. If the build dir gets created with a sticky bit, that propagates through the package. A
Additionally provide a separate error for the confusing if unlikely event that the user tries to use an existing file as a package output directory. This also means we now consistently try to create any nonexistent *DEST directories as needed before aborting with E_FS_PERMISSIONS. Previously only $BUILDDIR received that kindness. Fixes FS#43537 Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- v3: rebase against escondida's error codes patch Also re-add the erroneously removed `chmod a-s`. Just do this unconditionally now. scripts/libmakepkg/util/util.sh.in | 19 +++++++++++++++++++ scripts/makepkg.sh.in | 20 +++++++------------- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/scripts/libmakepkg/util/util.sh.in b/scripts/libmakepkg/util/util.sh.in index d676249d..8a6149ed 100644 --- a/scripts/libmakepkg/util/util.sh.in +++ b/scripts/libmakepkg/util/util.sh.in @@ -83,3 +83,22 @@ cd_safe() { exit 1 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() { + local parent=$1 dirname=$1 + + until [[ -e $parent ]]; do + parent="${parent%/*}" + done + if [[ -f $parent ]]; then + error "$(gettext "Cannot create %s due to conflicting file.")" "$parent" + return 1 + fi + if ( [[ ! -d $dirname ]] && ! mkdir -p "$dirname" ) || [[ ! -w $dirname ]]; then + return 1 + fi + return 0 +} diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 1191e4ec..c9c80bca 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1359,34 +1359,28 @@ else fi -if [[ ! -d $BUILDDIR ]]; then - if ! mkdir -p "$BUILDDIR"; then - error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR" - plain "$(gettext "Aborting...")" - exit $E_FS_PERMISSIONS - fi - chmod a-s "$BUILDDIR" -fi -if [[ ! -w $BUILDDIR ]]; then +# check that all settings directories are user-writable +if ! ensure_writable_dir "$BUILDDIR"; then error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR" plain "$(gettext "Aborting...")" exit $E_FS_PERMISSIONS fi +chmod a-s "$BUILDDIR" -if (( ! (NOBUILD || GENINTEG) )) && [[ ! -w $PKGDEST ]]; then +if (( ! (NOBUILD || GENINTEG) )) && ! ensure_writable_dir "$PKGDEST"; then error "$(gettext "You do not have write permission to store packages in %s.")" "$PKGDEST" plain "$(gettext "Aborting...")" exit $E_FS_PERMISSIONS fi -if [[ ! -w $SRCDEST ]] ; then +if ! ensure_writable_dir "$SRCDEST" ; then error "$(gettext "You do not have write permission to store downloads in %s.")" "$SRCDEST" plain "$(gettext "Aborting...")" exit $E_FS_PERMISSIONS fi if (( SOURCEONLY )); then - if [[ ! -w $SRCPKGDEST ]]; then + if ! ensure_writable_dir "$SRCPKGDEST"; then error "$(gettext "You do not have write permission to store source tarballs in %s.")" "$SRCPKGDEST" plain "$(gettext "Aborting...")" exit $E_FS_PERMISSIONS @@ -1397,7 +1391,7 @@ if (( SOURCEONLY )); then IGNOREARCH=1 fi -if (( LOGGING )) && [[ ! -w $LOGDEST ]]; then +if (( LOGGING )) && ! ensure_writable_dir "$LOGDEST"; then error "$(gettext "You do not have write permission to store logs in %s.")" "$LOGDEST" plain "$(gettext "Aborting...")" exit $E_FS_PERMISSIONS -- 2.15.1
participants (2)
-
Allan McRae
-
Eli Schwartz