At Sat, 10 May 2014 09:44:33 -0400, Dave Reisner wrote:

This allows handling of args with whitespace and other nonsense to be passed properly to makepkg. Contrived example:

makechrootpkg -r /path/to/chroot -- --config "/path/to/some config"

Unless I'm missing something, this patch won't successfully do that.

@@ -100,7 +100,7 @@ else fi

# Pass all arguments after -- right to makepkg -makepkg_args="$makepkg_args ${*:$OPTIND}" +makepkg_args=("${@:OPTIND}")

What about the previous value of makepkg_args?

@@ -256,8 +256,9 @@ EOF

# This is a little gross, but this way the script is recreated every time in the # working copy + printf -v extra_args ' %q' "${makepkg_args[@]}" printf $'#!/bin/bash\n%s\n_chrootbuild %q %q' "$(declare -f _chrootbuild)" \ - "$makepkg_args" "$run_namcap" >"$copydir/chrootbuild" + "$extra_args" "$run_namcap" >"$copydir/chrootbuild" chmod +x "$copydir/chrootbuild" }

I believe that the arguments are getting double-escaped; once by %q in the first printf, then again in the second printf. Also, this leaks $extra_args as a global.

@@ -320,7 +321,7 @@ _chrootbuild() { exit 1 fi

- sudo -u nobody makepkg $makepkg_args || exit 1 + sudo -u nobody makepkg "$makepkg_args" || exit 1

However extra_args was escaped, doesn't this basically ignore that, and shove it all into one argument? ---- Interestingly, I was just getting ready to submit a patch also does this: From 35b2638d44de2b9033b8c71163c997c588ded6fa Mon Sep 17 00:00:00 2001 From: Luke Shumaker <LukeShu@sbcglobal.net> Date: Sat, 10 May 2014 19:21:44 -0400 Subject: [PATCH] makechrootpkg: Store makepkg_args as an array. To do this simply, instead of embedding makepkg_args in /chrootbuild, it passes them as arguments to /chrootbuild. --- makechrootpkg.in | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/makechrootpkg.in b/makechrootpkg.in index 6a99408..f646117 100644 --- a/makechrootpkg.in +++ b/makechrootpkg.in @@ -12,7 +12,8 @@ m4_include(lib/common.sh) shopt -s nullglob -makepkg_args='-s --noconfirm -L --holdver' +default_makepkg_args=(-s --noconfirm -L --holdver) +makepkg_args=("${default_makepkg_args[@]}") repack=false update_first=false clean_first=false @@ -46,7 +47,7 @@ usage() { echo 'command:' echo ' mkarchroot <chrootdir>/root base-devel' echo '' - echo "Default makepkg args: $makepkg_args" + echo "Default makepkg args: ${default_makepkg_args[*]}" echo '' echo 'Flags:' echo '-h This help' @@ -76,7 +77,7 @@ while getopts 'hcur:I:l:nTD:d:' arg; do r) passeddir="$OPTARG" ;; I) install_pkgs+=("$OPTARG") ;; l) copy="$OPTARG" ;; - n) run_namcap=true; makepkg_args="$makepkg_args -i" ;; + n) run_namcap=true; makepkg_args+=('-i') ;; T) temp_chroot=true; copy+="-$$" ;; esac done @@ -100,7 +101,7 @@ else fi # Pass all arguments after -- right to makepkg -makepkg_args="$makepkg_args ${*:$OPTIND}" +makepkg_args+=("${@:OPTIND}") # See if -R was passed to makepkg for arg in "${@:OPTIND}"; do @@ -256,8 +257,8 @@ EOF # This is a little gross, but this way the script is recreated every time in the # working copy - printf $'#!/bin/bash\n%s\n_chrootbuild %q %q' "$(declare -f _chrootbuild)" \ - "$makepkg_args" "$run_namcap" >"$copydir/chrootbuild" + printf $'#!/bin/bash\n%s\n_chrootbuild %q "$@"' "$(declare -f _chrootbuild)" \ + "$run_namcap" >"$copydir/chrootbuild" chmod +x "$copydir/chrootbuild" } @@ -283,8 +284,8 @@ download_sources() { _chrootbuild() { # This function isn't run in makechrootpkg, # so no global variables - local makepkg_args="$1" - local run_namcap="$2" + local run_namcap="$1"; shift + local makepkg_args=("$@") . /etc/profile export HOME=/build @@ -320,7 +321,7 @@ _chrootbuild() { exit 1 fi - sudo -u nobody makepkg $makepkg_args || exit 1 + sudo -u nobody makepkg "${makepkg_args[@]}" || exit 1 if $run_namcap; then pacman -S --needed --noconfirm namcap @@ -379,7 +380,7 @@ if arch-nspawn "$copydir" \ --bind-ro="$PWD:/startdir_host" \ --bind-ro="$SRCDEST:/srcdest_host" \ "${bindmounts_ro[@]}" "${bindmounts_rw[@]}" \ - /chrootbuild + /chrootbuild "${makepkg_args[@]}" then move_products else -- 1.9.2

On Sat, May 10, 2014 at 09:44:34AM -0400, Dave Reisner wrote:

We run from a non-interactive shell, so the exec which is inevitably called will replace the current process and 'die' will never run under any circumstances.

This also fixes a bug with the su fallback which would cause multiple arguments to be concatenated without any whitespace between them. --- lib/common.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/common.sh b/lib/common.sh index b885080..1798773 100644 --- a/lib/common.sh +++ b/lib/common.sh @@ -237,7 +237,6 @@ check_root() { if type -P sudo >/dev/null; then exec sudo -- "$@" else

Perhaps it would be better to not assume that su exists here, check for it, and then fall back on the 'die'.

- exec su root -c "$(printf '%q' "$@")" + exec su root -c "$(printf ' %q' "$@")" fi - die 'This script must be run as root.' } -- 1.9.2