[pacman-dev] [PATCH v3] makepkg: do not eval dlcmd

[Dave Reisner]

On Wed, Jan 07, 2015 at 02:24:19PM +1000, Allan McRae wrote:
> On 08/09/14 02:57, lolilolicon wrote:
> > This eval enables the following in a PKGBUILD to "just work":
> > 
> >   source=('$pkgname-$pkgver.tar.gz'::'https://host/$pkgver.tar.gz')
> > 
> > This has at least two problems:
> > 
> > - It violated the principle of least surprise.
> > - It could be a security issue since URLs are arbitrary input.
> > 
> > Instead, expand the dlagent command line into an array, replace the %o,
> > %u place holders, and run the resultant command line as is.
> > 
> > Embedded spaces in the DLAGENTS entry can be escaped with a backslash.
> > 
> > Fixes FS#41682
> > ---
> 
> This is broken with bash<4.3.  There are extra quotes everywhere in the
> command.
> 
> bash-4.2
> + /usr/bin/curl -fLC - --retry 3 --retry-delay 3 -o '"64.tar.xz.part"'
> '"http://allanmcrae.com/tmp/64.tar.xz"'
> 
> bash-4.3
> + /usr/bin/curl -fLC - --retry 3 --retry-delay 3 -o 64.tar.xz.part
> http://allanmcrae.com/tmp/64.tar.xz
> 
> @Dave: any suggestions on how to fix.
> 
> 

I'm not clear on what the problematic quoting actually fixes. If it
actually has a purpose, I'm fairly confident that we can just neglect
the esoteric edge case it covers, and leave behind a TODO that we should
re-fix it once we're comfortable with bash 4.3 being the required
minimum.

> >  scripts/makepkg.sh.in | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> > index 8e8a64c..446e9b0 100644
> > --- a/scripts/makepkg.sh.in
> > +++ b/scripts/makepkg.sh.in
> > @@ -342,8 +342,9 @@ download_file() {
> >  	local proto=$(get_protocol "$netfile")
> >  
> >  	# find the client we should use for this URL
> > -	local dlcmd
> > -	dlcmd=$(get_downloadclient "$proto") || exit $?
> > +	local -a cmdline
> > +	IFS=' ' read -a cmdline < <(get_downloadclient "$proto")
> > +	(( ${#cmdline[@]} )) || exit
> >  
> >  	local filename=$(get_filename "$netfile")
> >  	local url=$(get_url "$netfile")
> > @@ -359,20 +360,18 @@ download_file() {
> >  	local dlfile="${url##*/}"
> >  
> >  	# replace %o by the temporary dlfile if it exists
> > -	if [[ $dlcmd = *%o* ]]; then
> > -		dlcmd=${dlcmd//\%o/\"$filename.part\"}
> > -		dlfile="$filename.part"
> > +	if [[ ${cmdline[*]} = *%o* ]]; then
> > +		dlfile=$filename.part
> > +		cmdline=("${cmdline[@]//%o/"$dlfile"}")
> >  	fi
> >  	# add the URL, either in place of %u or at the end
> > -	if [[ $dlcmd = *%u* ]]; then
> > -		dlcmd=${dlcmd//\%u/\"$url\"}
> > +	if [[ ${cmdline[*]} = *%u* ]]; then
> > +		cmdline=("${cmdline[@]//%u/"$url"}")
> >  	else
> > -		dlcmd="$dlcmd \"$url\""
> > +		cmdline+=("$url")
> >  	fi
> >  
> > -	local ret=0
> > -	eval "$dlcmd >&2 || ret=\$?"
> > -	if (( ret )); then
> > +	if ! command -- "${cmdline[@]}" >&2; then
> >  		[[ ! -s $dlfile ]] && rm -f -- "$dlfile"
> >  		error "$(gettext "Failure while downloading %s")" "$filename"
> >  		plain "$(gettext "Aborting...")"
> >