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

[lolilolicon]

Something like `source=('$pkgname-$pkgver.tar.gz')` in the PKGBUILD
would "just work" with this eval. This could be a security issue since
URLs are arbitrary external data.

Instead, expand the dlagent command line into an array, replace the %o,
%u place holders, and run the resultant command line as is.

A shortcoming of this is the dlagent command line cannot have embedded
spaces. If this turns out to be a problem, we can do an early eval
before substituting %o and %u.
---
 scripts/makepkg.sh.in | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index 8e8a64c..a134453 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -288,12 +288,12 @@ get_protocol() {
 get_downloadclient() {
 	local proto=$1
 
-	# loop through DOWNLOAD_AGENTS variable looking for protocol
+	# loop through DLAGENTS variable looking for protocol
 	local i
 	for i in "${DLAGENTS[@]}"; do
 		local handler="${i%%::*}"
 		if [[ $proto = "$handler" ]]; then
-			local agent="${i##*::}"
+			local agent="${i#*::}"
 			break
 		fi
 	done
@@ -308,8 +308,7 @@ get_downloadclient() {
 	# ensure specified program is installed
 	local program="${agent%% *}"
 	if [[ ! -x $program ]]; then
-		local baseprog="${program##*/}"
-		error "$(gettext "The download program %s is not installed.")" "$baseprog"
+		error "$(gettext "The download program '%s' is not installed.")" "$program"
 		plain "$(gettext "Aborting...")"
 		exit 1 # $E_MISSING_PROGRAM
 	fi
@@ -342,15 +341,16 @@ 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
+	local IFS=' '
+	cmdline=($(get_downloadclient "$proto")) || exit
 
 	local filename=$(get_filename "$netfile")
 	local url=$(get_url "$netfile")
 
 	if [[ $proto = "scp" ]]; then
 		# scp downloads should not pass the protocol in the url
-		url="${url##*://}"
+		url="${url#*://}"
 	fi
 
 	msg2 "$(gettext "Downloading %s...")" "$filename"
@@ -359,20 +359,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...")"
-- 
2.1.0