Patchwork [pacman-dev,1/2] makepkg: make $pkgdir non-accessible during build()

login
register
mail settings
Submitter Allan McRae
Date Feb. 2, 2013, 2:51 a.m.
Message ID <1359773484-23817-1-git-send-email-allan@archlinux.org>
Download mbox | patch
Permalink /patch/874/
State Accepted, archived
Headers show

Comments

Allan McRae - Feb. 2, 2013, 2:51 a.m.
The idea of having separate build() and package() functions is that
build() is run as a normal uses and package() as (fake)root.  Any
files placed in $pkgdir during build() can have the wrong permissions.

Restrict access to $pkgdir during build() - unless there is no package()
function.

Also, set $pkgdir to something "useful" during build().  For split
packages, this uses "<path>/pkg/$pkgbase" because it is not obvious
which $pkgdir is being referred to.

Signed-off-by: Allan McRae <allan@archlinux.org>
---
 scripts/makepkg.sh.in | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)
Martin Panter - Feb. 2, 2013, 4:16 a.m.
On 2 February 2013 02:51, Allan McRae <allan@archlinux.org> wrote:
> The idea of having separate build() and package() functions is that
> build() is run as a normal uses and package() as (fake)root.  Any
> files placed in $pkgdir during build() can have the wrong permissions.
>
> Restrict access to $pkgdir during build() - unless there is no package()
> function.
>
> Also, set $pkgdir to something "useful" during build().  For split
> packages, this uses "<path>/pkg/$pkgbase" because it is not obvious
> which $pkgdir is being referred to.
>
> Signed-off-by: Allan McRae <allan@archlinux.org>
> ---
>  scripts/makepkg.sh.in | 42 ++++++++++++++++++++++++------------------
>  1 file changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index bcc415b..ec39c2e 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -152,7 +152,7 @@ clean_up() {
>
>                 # If it's a clean exit and -c/--clean has been passed...
>                 msg "$(gettext "Cleaning up...")"
> -               rm -rf "$pkgdir" "$srcdir"
> +               rm -rf "$pkgdirbase" "$srcdir"
>                 if [[ -n $pkgbase ]]; then
>                         local fullver=$(get_full_version)
>                         # Can't do this unless the BUILDSCRIPT has been sourced.
> @@ -1519,7 +1519,7 @@ tidy_install() {
>         if find "${pkgdir}" -type f -print0 | xargs -0  grep -q -I "${srcdir}" ; then
>                 warning "$(gettext "Package contains reference to %s")" "\$srcdir"
>         fi
> -       if find "${pkgdir}" -type f -print0 | xargs -0  grep -q -I "${pkgdir}" ; then
> +       if find "${pkgdir}" -type f -print0 | xargs -0  grep -q -I "${pkgdirbase}" ; then
>                 warning "$(gettext "Package contains reference to %s")" "\$pkgdir"
>         fi
>
> @@ -2344,16 +2344,14 @@ restore_package_variables() {
>  run_split_packaging() {
>         local pkgname_backup=${pkgname[@]}
>         for pkgname in ${pkgname_backup[@]}; do
> -               pkgdir="$pkgdir/$pkgname"
> -               mkdir -p "$pkgdir"
> -               chmod a-s "$pkgdir"
> +               pkgdir="$pkgdirbase/$pkgname"
> +               mkdir "$pkgdir"
>                 backup_package_variables
>                 run_package $pkgname
>                 tidy_install
>                 create_package
>                 create_debug_package
>                 restore_package_variables
> -               pkgdir="${pkgdir%/*}"
>         done
>         pkgname=${pkgname_backup[@]}
>  }
> @@ -2689,12 +2687,16 @@ epoch=${epoch:-0}
>
>  if [[ $BUILDDIR = "$startdir" ]]; then
>         srcdir="$BUILDDIR/src"
> -       pkgdir="$BUILDDIR/pkg"
> +       pkgdirbase="$BUILDDIR/pkg"
>  else
>         srcdir="$BUILDDIR/$pkgbase/src"
> -       pkgdir="$BUILDDIR/$pkgbase/pkg"
> +       pkgdirbase="$BUILDDIR/$pkgbase/pkg"
> +
>  fi
>
> +# set pkgdir to something "sensible" for (not recommended) use during build()
> +pkgdir="$pkgdirbase/$pkgbase"
> +
>  if (( GENINTEG )); then
>         mkdir -p "$srcdir"
>         chmod a-s "$srcdir"
> @@ -2767,9 +2769,10 @@ if (( INFAKEROOT )); then
>                 exit 0 # $E_OK
>         fi
>
> +       chmod 755 "$pkgdirbase"
>         if (( ! SPLITPKG )); then
> -               pkgdir="$pkgdir/$pkgname"
> -               mkdir -p "$pkgdir"
> +               pkgdir="$pkgdirbase/$pkgname"
> +               mkdir "$pkgdir"

Looks like removing the “-p” option causes a crash when using
“--repackage” on a PKGBUILD with no package() function.

==> WARNING: Using a PKGBUILD without a package() function is deprecated.
==> Checking runtime dependencies...
==> Checking buildtime dependencies...
==> Entering fakeroot environment...
mkdir: cannot create directory ‘/home/proj/pacman/pacman/pkg/test’: File exists
/usr/bin/fakeroot: line 178:  5004 User defined signal 1
FAKEROOTKEY=$FAKEROOTKEY LD_LIBRARY_PATH="$PATHS" LD_PRELOAD="$LIB"
"$@"

Not that I ever used this option, just seeing what it takes to trigger
the empty package directory error below.

>                 if (( ! PKGFUNC )); then
>                         if (( ! REPKG )); then
>                                 if (( BUILDFUNC )); then
> @@ -2786,7 +2789,6 @@ if (( INFAKEROOT )); then
>                 tidy_install
>                 create_package
>                 create_debug_package
> -               pkgdir="${pkgdir%/*}"
>         else
>                 run_split_packaging
>         fi
> @@ -2881,7 +2883,7 @@ if (( NOEXTRACT )); then
>         warning "$(gettext "Using existing %s tree")" "src/"
>  elif (( REPKG )); then
>         if (( ! PKGFUNC && ! SPLITPKG )) \
> -            && { [[ ! -d $pkgdir ]] || dir_is_empty "$pkgdir"; }; then
> +            && { [[ ! -d $pkgdirbase ]] || dir_is_empty "$pkgdirbase"; }; then
>                 error "$(gettext "The package directory is empty, there is nothing to repackage!")"
>                 plain "$(gettext "Aborting...")"
>                 exit 1
> @@ -2900,22 +2902,27 @@ if (( NOBUILD )); then
>         exit 0 #E_OK
>  else
>         # check for existing pkg directory; don't remove if we are repackaging
> -       if [[ -d $pkgdir ]] && (( ! REPKG || PKGFUNC || SPLITPKG )); then
> +       if [[ -d $pkgdirbase ]] && (( ! REPKG || PKGFUNC || SPLITPKG )); then
>                 msg "$(gettext "Removing existing %s directory...")" "pkg/"
> -               rm -rf "$pkgdir"
> +               rm -rf "$pkgdirbase"
>         fi
> -       mkdir -p "$pkgdir"
> -       chmod a-s "$pkgdir"
> +       mkdir -p "$pkgdirbase"
> +       chmod a-srwx "$pkgdirbase"

Nice touch, makes it hard to accidentally install files there during build().

>         cd_safe "$startdir"
>
>         # if we are root or if fakeroot is not enabled, then we don't use it
>         if ! check_buildenv "fakeroot" "y" || (( EUID == 0 )); then
>                 if (( ! REPKG )); then
> +                       if (( ! ( SPLITPKG || PKGFUNC ) )); then
> +                               chmod 755 "$pkgdirbase"
> +                               mkdir "$pkgdir"
> +                       fi
>                         (( BUILDFUNC )) && run_build
>                         (( CHECKFUNC )) && run_check
>                 fi
> +               chmod 755 "$pkgdirbase"
>                 if (( ! SPLITPKG )); then
> -                       pkgdir="$pkgdir/$pkgname"
> +                       pkgdir="$pkgdirbase/$pkgname"
>                         mkdir -p "$pkgdir"
>                         if (( PKGFUNC )); then
>                                 run_package
> @@ -2926,7 +2933,6 @@ else
>                         tidy_install
>                         create_package
>                         create_debug_package
> -                       pkgdir="${pkgdir%/*}"
>                 else
>                         run_split_packaging
>                 fi
Allan McRae - Feb. 2, 2013, 5:41 a.m.
On 02/02/13 14:16, Martin Panter wrote:
> On 2 February 2013 02:51, Allan McRae <allan@archlinux.org> wrote:
>> The idea of having separate build() and package() functions is that
>> build() is run as a normal uses and package() as (fake)root.  Any
>> files placed in $pkgdir during build() can have the wrong permissions.
>>
>> Restrict access to $pkgdir during build() - unless there is no package()
>> function.
>>
>> Also, set $pkgdir to something "useful" during build().  For split
>> packages, this uses "<path>/pkg/$pkgbase" because it is not obvious
>> which $pkgdir is being referred to.
>>
>> Signed-off-by: Allan McRae <allan@archlinux.org>
>> ---
>>  scripts/makepkg.sh.in | 42 ++++++++++++++++++++++++------------------
>>  1 file changed, 24 insertions(+), 18 deletions(-)
>>
>> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
>> index bcc415b..ec39c2e 100644
>> --- a/scripts/makepkg.sh.in
>> +++ b/scripts/makepkg.sh.in
>> @@ -152,7 +152,7 @@ clean_up() {
>>
>>                 # If it's a clean exit and -c/--clean has been passed...
>>                 msg "$(gettext "Cleaning up...")"
>> -               rm -rf "$pkgdir" "$srcdir"
>> +               rm -rf "$pkgdirbase" "$srcdir"
>>                 if [[ -n $pkgbase ]]; then
>>                         local fullver=$(get_full_version)
>>                         # Can't do this unless the BUILDSCRIPT has been sourced.
>> @@ -1519,7 +1519,7 @@ tidy_install() {
>>         if find "${pkgdir}" -type f -print0 | xargs -0  grep -q -I "${srcdir}" ; then
>>                 warning "$(gettext "Package contains reference to %s")" "\$srcdir"
>>         fi
>> -       if find "${pkgdir}" -type f -print0 | xargs -0  grep -q -I "${pkgdir}" ; then
>> +       if find "${pkgdir}" -type f -print0 | xargs -0  grep -q -I "${pkgdirbase}" ; then
>>                 warning "$(gettext "Package contains reference to %s")" "\$pkgdir"
>>         fi
>>
>> @@ -2344,16 +2344,14 @@ restore_package_variables() {
>>  run_split_packaging() {
>>         local pkgname_backup=${pkgname[@]}
>>         for pkgname in ${pkgname_backup[@]}; do
>> -               pkgdir="$pkgdir/$pkgname"
>> -               mkdir -p "$pkgdir"
>> -               chmod a-s "$pkgdir"
>> +               pkgdir="$pkgdirbase/$pkgname"
>> +               mkdir "$pkgdir"
>>                 backup_package_variables
>>                 run_package $pkgname
>>                 tidy_install
>>                 create_package
>>                 create_debug_package
>>                 restore_package_variables
>> -               pkgdir="${pkgdir%/*}"
>>         done
>>         pkgname=${pkgname_backup[@]}
>>  }
>> @@ -2689,12 +2687,16 @@ epoch=${epoch:-0}
>>
>>  if [[ $BUILDDIR = "$startdir" ]]; then
>>         srcdir="$BUILDDIR/src"
>> -       pkgdir="$BUILDDIR/pkg"
>> +       pkgdirbase="$BUILDDIR/pkg"
>>  else
>>         srcdir="$BUILDDIR/$pkgbase/src"
>> -       pkgdir="$BUILDDIR/$pkgbase/pkg"
>> +       pkgdirbase="$BUILDDIR/$pkgbase/pkg"
>> +
>>  fi
>>
>> +# set pkgdir to something "sensible" for (not recommended) use during build()
>> +pkgdir="$pkgdirbase/$pkgbase"
>> +
>>  if (( GENINTEG )); then
>>         mkdir -p "$srcdir"
>>         chmod a-s "$srcdir"
>> @@ -2767,9 +2769,10 @@ if (( INFAKEROOT )); then
>>                 exit 0 # $E_OK
>>         fi
>>
>> +       chmod 755 "$pkgdirbase"
>>         if (( ! SPLITPKG )); then
>> -               pkgdir="$pkgdir/$pkgname"
>> -               mkdir -p "$pkgdir"
>> +               pkgdir="$pkgdirbase/$pkgname"
>> +               mkdir "$pkgdir"
> 
> Looks like removing the “-p” option causes a crash when using
> “--repackage” on a PKGBUILD with no package() function.
> 
> ==> WARNING: Using a PKGBUILD without a package() function is deprecated.
> ==> Checking runtime dependencies...
> ==> Checking buildtime dependencies...
> ==> Entering fakeroot environment...
> mkdir: cannot create directory ‘/home/proj/pacman/pacman/pkg/test’: File exists
> /usr/bin/fakeroot: line 178:  5004 User defined signal 1
> FAKEROOTKEY=$FAKEROOTKEY LD_LIBRARY_PATH="$PATHS" LD_PRELOAD="$LIB"
> "$@"
> 
> Not that I ever used this option, just seeing what it takes to trigger
> the empty package directory error below.

Nice catch...   I forgot "-p" is not just to create parents!

Fixed on my working branch.

Allan

Patch

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index bcc415b..ec39c2e 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -152,7 +152,7 @@  clean_up() {
 
 		# If it's a clean exit and -c/--clean has been passed...
 		msg "$(gettext "Cleaning up...")"
-		rm -rf "$pkgdir" "$srcdir"
+		rm -rf "$pkgdirbase" "$srcdir"
 		if [[ -n $pkgbase ]]; then
 			local fullver=$(get_full_version)
 			# Can't do this unless the BUILDSCRIPT has been sourced.
@@ -1519,7 +1519,7 @@  tidy_install() {
 	if find "${pkgdir}" -type f -print0 | xargs -0  grep -q -I "${srcdir}" ; then
 		warning "$(gettext "Package contains reference to %s")" "\$srcdir"
 	fi
-	if find "${pkgdir}" -type f -print0 | xargs -0  grep -q -I "${pkgdir}" ; then
+	if find "${pkgdir}" -type f -print0 | xargs -0  grep -q -I "${pkgdirbase}" ; then
 		warning "$(gettext "Package contains reference to %s")" "\$pkgdir"
 	fi
 
@@ -2344,16 +2344,14 @@  restore_package_variables() {
 run_split_packaging() {
 	local pkgname_backup=${pkgname[@]}
 	for pkgname in ${pkgname_backup[@]}; do
-		pkgdir="$pkgdir/$pkgname"
-		mkdir -p "$pkgdir"
-		chmod a-s "$pkgdir"
+		pkgdir="$pkgdirbase/$pkgname"
+		mkdir "$pkgdir"
 		backup_package_variables
 		run_package $pkgname
 		tidy_install
 		create_package
 		create_debug_package
 		restore_package_variables
-		pkgdir="${pkgdir%/*}"
 	done
 	pkgname=${pkgname_backup[@]}
 }
@@ -2689,12 +2687,16 @@  epoch=${epoch:-0}
 
 if [[ $BUILDDIR = "$startdir" ]]; then
 	srcdir="$BUILDDIR/src"
-	pkgdir="$BUILDDIR/pkg"
+	pkgdirbase="$BUILDDIR/pkg"
 else
 	srcdir="$BUILDDIR/$pkgbase/src"
-	pkgdir="$BUILDDIR/$pkgbase/pkg"
+	pkgdirbase="$BUILDDIR/$pkgbase/pkg"
+
 fi
 
+# set pkgdir to something "sensible" for (not recommended) use during build()
+pkgdir="$pkgdirbase/$pkgbase"
+
 if (( GENINTEG )); then
 	mkdir -p "$srcdir"
 	chmod a-s "$srcdir"
@@ -2767,9 +2769,10 @@  if (( INFAKEROOT )); then
 		exit 0 # $E_OK
 	fi
 
+	chmod 755 "$pkgdirbase"
 	if (( ! SPLITPKG )); then
-		pkgdir="$pkgdir/$pkgname"
-		mkdir -p "$pkgdir"
+		pkgdir="$pkgdirbase/$pkgname"
+		mkdir "$pkgdir"
 		if (( ! PKGFUNC )); then
 			if (( ! REPKG )); then
 				if (( BUILDFUNC )); then
@@ -2786,7 +2789,6 @@  if (( INFAKEROOT )); then
 		tidy_install
 		create_package
 		create_debug_package
-		pkgdir="${pkgdir%/*}"
 	else
 		run_split_packaging
 	fi
@@ -2881,7 +2883,7 @@  if (( NOEXTRACT )); then
 	warning "$(gettext "Using existing %s tree")" "src/"
 elif (( REPKG )); then
 	if (( ! PKGFUNC && ! SPLITPKG )) \
-	     && { [[ ! -d $pkgdir ]] || dir_is_empty "$pkgdir"; }; then
+	     && { [[ ! -d $pkgdirbase ]] || dir_is_empty "$pkgdirbase"; }; then
 		error "$(gettext "The package directory is empty, there is nothing to repackage!")"
 		plain "$(gettext "Aborting...")"
 		exit 1
@@ -2900,22 +2902,27 @@  if (( NOBUILD )); then
 	exit 0 #E_OK
 else
 	# check for existing pkg directory; don't remove if we are repackaging
-	if [[ -d $pkgdir ]] && (( ! REPKG || PKGFUNC || SPLITPKG )); then
+	if [[ -d $pkgdirbase ]] && (( ! REPKG || PKGFUNC || SPLITPKG )); then
 		msg "$(gettext "Removing existing %s directory...")" "pkg/"
-		rm -rf "$pkgdir"
+		rm -rf "$pkgdirbase"
 	fi
-	mkdir -p "$pkgdir"
-	chmod a-s "$pkgdir"
+	mkdir -p "$pkgdirbase"
+	chmod a-srwx "$pkgdirbase"
 	cd_safe "$startdir"
 
 	# if we are root or if fakeroot is not enabled, then we don't use it
 	if ! check_buildenv "fakeroot" "y" || (( EUID == 0 )); then
 		if (( ! REPKG )); then
+			if (( ! ( SPLITPKG || PKGFUNC ) )); then
+				chmod 755 "$pkgdirbase"
+				mkdir "$pkgdir"
+			fi
 			(( BUILDFUNC )) && run_build
 			(( CHECKFUNC )) && run_check
 		fi
+		chmod 755 "$pkgdirbase"
 		if (( ! SPLITPKG )); then
-			pkgdir="$pkgdir/$pkgname"
+			pkgdir="$pkgdirbase/$pkgname"
 			mkdir -p "$pkgdir"
 			if (( PKGFUNC )); then
 				run_package
@@ -2926,7 +2933,6 @@  else
 			tidy_install
 			create_package
 			create_debug_package
-			pkgdir="${pkgdir%/*}"
 		else
 			run_split_packaging
 		fi