diff mbox

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

Message ID 20171008070510.13259-1-eschwartz@archlinux.org
State Superseded, archived
Headers show

Commit Message

Eli Schwartz Oct. 8, 2017, 7:05 a.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.

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(-)

Comments

Allan McRae Oct. 8, 2017, 3:12 p.m. UTC | #1
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
>
Eli Schwartz Oct. 9, 2017, 4:07 a.m. UTC | #2
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???
diff mbox

Patch

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