[pacman-dev,v2,2/2] makepkg: refactor checking for write permissions into a utility function

Message ID 20171030180329.24189-2-eschwartz@archlinux.org
State Superseded, archived
Headers show
Series [pacman-dev,v2,1/2] makepkg: reorganize the restoration of settings by precedence | expand

Commit Message

Eli Schwartz Oct. 30, 2017, 6:03 p.m. UTC
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(-)

Comments

Allan McRae Dec. 7, 2017, 5:31 a.m. UTC | #1
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

Patch

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