[arch-projects] [dbscripts] [PATCH 1/8] Fix quoting around variables, especially arrays.

[Eli Schwartz]

On 03/13/2018 09:51 PM, Luke Shumaker wrote:
> From: Luke Shumaker <lukeshu at parabola.nu>
> 
> Other than pure quoting, this involved:
>  - swapping */@ for array access in a few places
>  - fiddling with printf in a pipeline
>  - replacing `$(echo ${array[@]})` with `${array[*]}`
>  - replacing `echo $(...)` with `...`
> 
> When searching for these things, I used the command:
> 
>     grep -Prn --exclude-dir=.git '(?<!["=]|\[\[ |\[\[ -[zn] )\$(?!{?#|\(|\? )'
> 
> and ignored a bunch of false positives.

I don't really see the need to quote every variable just because it is a
variable, at least in cases where the variable is fairly statically
defined... I'd like to see path components that could have spaces
quoted, and arrays I guess because semantically that's how you iterate
over an array. Also you introduced some bugs, in cases where we actually
want whitespace splitting...

>  backup_package_variables() {
> -	for var in ${splitpkg_overrides[@]}; do
> +	for var in "${splitpkg_overrides[@]}"; do
>  		indirect="${var}_backup"
> -		eval "${indirect}=(\${$var[@]})"
> +		eval "${indirect}=(\"\${$var[@]}\")"
>  	done
>  }
>  
>  restore_package_variables() {
> -	for var in ${splitpkg_overrides[@]}; do
> +	for var in "${splitpkg_overrides[@]}"; do
>  		indirect="${var}_backup"
>  		if [ -n "${!indirect}" ]; then
> -			eval "${var}=(\${$indirect[@]})"
> +			eval "${var}=(\"\${$indirect[@]}\")"
>  		else
> -			unset ${var}
> +			unset "${var}"
>  		fi
>  	done

This is too much escaping and metaprogramming, there are better ways of
backing up a variable to begin with. :/

We do it in makepkg, I will have us do it here as well. Advantage: using
declare -p means the shell auto-escapes things where needed.

> -	if ! ${CLEANUP_DRYRUN}; then
> +	if ! "${CLEANUP_DRYRUN}"; then

This is a variable being run as a command, so if there were to be spaces
in it we'd end up trying to run a command with a space in it. Arguably
we should not be running this as a command (even though they are set to
true/false which is a shell builtin blah blah blah) but since we are it
would be illogical to indicate that if there are spaces they should be
interpreted as string literals in an executable filename.


> -${CLEANUP_DRYRUN} && warning 'dry run mode is active'
> +"${CLEANUP_DRYRUN}" && warning 'dry run mode is active'

Same.

> -		if ! ${CLEANUP_DRYRUN}; then
> +		if ! "${CLEANUP_DRYRUN}"; then

Same.

>  # Create a readable file for each repo with the following format
>  # <pkgbase|pkgname> <pkgver>-<pkgrel> <arch> <license>[ <license>]

When we consume this file...

>  	while read line; do
> -		pkginfo=(${line})
> +		pkginfo=("${line}")

That's completely wrong, just look at the next five lines.

>  		pkgbase=${pkginfo[0]}
>  		pkgver=${pkginfo[1]}
>  		pkgarch=${pkginfo[2]}
> -		pkglicense=(${pkginfo[@]:3})
> +		pkglicense=("${pkginfo[@]:3}")

How will we extract elements of this array, if your quoting squashes
everything down into one array element? ${pkginfo[0]} will have too
much, and the other elements simply won't exist at all.

We go from having:

declare -a pkginfo=([0]="foo" [1]="1.0-1" [2]="any" [3]="GPL")

to having:

declare -a pkginfo=([0]="foo 1.0-1 any GPL")

> -		if ! ([[ -z ${ALLOWED_LICENSES[@]} ]] || chk_license ${pkglicense[@]} || grep -Fqx "${pkgbase}" "${dirname}/sourceballs.force"); then
> +		if ! ([[ -z ${ALLOWED_LICENSES[*]} ]] || chk_license "${pkglicense[@]}" || grep -Fqx "${pkgbase}" "${dirname}/sourceballs.force"); then

What's the check here anyways? This considers ALLOWED_LICENSES=('') to
be non-empty, so we might as well check the length of
${#ALLOWED_LICENSES[@]} which is a clearer read.


> -	${SOURCE_CLEANUP_DRYRUN} && warning 'dry run mode is active'
> -	for old_pkg in ${old_pkgs[@]}; do
> +	"${SOURCE_CLEANUP_DRYRUN}" && warning 'dry run mode is active'
> +	for old_pkg in "${old_pkgs[@]}"; do
>  		msg2 "${old_pkg}"
> -		if ! ${SOURCE_CLEANUP_DRYRUN}; then
> +		if ! "${SOURCE_CLEANUP_DRYRUN}"; then
>  			mv_acl "$FTP_BASE/${SRCPOOL}/${old_pkg}" "${SOURCE_CLEANUP_DESTDIR}/${old_pkg}"
>  			touch "${SOURCE_CLEANUP_DESTDIR}/${old_pkg}"
>  		fi
> @@ -147,9 +147,9 @@ done
>  
>  if (( ${#old_pkgs[@]} >= 1 )); then
>  	msg "Removing old source packages from the cleanup directory..."
> -	for old_pkg in ${old_pkgs[@]}; do
> +	for old_pkg in "${old_pkgs[@]}"; do
>  		msg2 "${old_pkg}"
> -		${SOURCE_CLEANUP_DRYRUN} || rm -f "${SOURCE_CLEANUP_DESTDIR}/${old_pkg}"
> +		"${SOURCE_CLEANUP_DRYRUN}" || rm -f "${SOURCE_CLEANUP_DESTDIR}/${old_pkg}"
>  	done

More commands-as-variables where whitespace if it existed would actually
be significant and wanted.

> -	if [[ ! -z "$(echo ${@%\.*} | sed "s/ /\n/g" | sort | uniq -D)" ]]; then
> +	if [[ ! -z "$(printf '%s\n' "${@%\.*}" | sort | uniq -D)" ]]; then

Thanks for noticing this! printf is a lot nicer. I'm also wondering if
it makes more sense to use a single awk 'a[$0]++{exit 1}' rather than
chaining sort/uniq and reading its length in a bash test... the problem
here was always that uniq -D returns successfully even if it prints
nothing. With awk we can actually have an exit code stating whether
duplicates were found.

> -		local svnnames=($(. "${WORKDIR}/pkgbuilds/${repo}-${_pkgarch}/${_pkgbase}"; echo ${pkgname[@]}))
> -		for svnname in ${svnnames[@]}; do
> -			echo "${svnname}" >> "${repo}/${_pkgarch}/${_pkgbase}/svn"
> -		done
> +		local svnnames=($(. "${WORKDIR}/pkgbuilds/${repo}-${_pkgarch}/${_pkgbase}"; echo "${pkgname[@]}"))
> +		printf '%s\n' "${svnnames[@]}" >> "${repo}/${_pkgarch}/${_pkgbase}/svn"

Again this does actually look a lot nicer, thanks for spotting this.


> -		arch_pkgs=($(getpkgfiles "${STAGING}/${repo}/"*-${pkgarch}${PKGEXTS} 2>/dev/null))
> -		for pkg in ${arch_pkgs[@]} ${any_pkgs[@]}; do
> +		arch_pkgs=($(getpkgfiles "${STAGING}/${repo}/"*-"${pkgarch}"${PKGEXTS} 2>/dev/null))
> +		for pkg in "${arch_pkgs[@]}" "${any_pkgs[@]}"; do

Dropping in and out of quotes here a number of times sort of hammers
home the point to me that maybe we really don't need to do this.
Configurable directories need to be quoted, yes, but then dropping out
to expand a wildcard, failing to quote the - for good measure, and
quoting pkgarch only to resort to expanding PKGEXTS as a glob pattern...

It is only possible anyways for $STAGING to have spaces.

> -		${found_source} || die "%s not found in [%s]" "$pkgbase" "$TESTING_REPO"
> +		"${found_source}" || die "%s not found in [%s]" "$pkgbase" "$TESTING_REPO"

command as variable

> -		${found_target} || die "%s not found in any of these repos: %s" "$pkgbase" "${STABLE_REPOS[*]}"
> +		"${found_target}" || die "%s not found in any of these repos: %s" "$pkgbase" "${STABLE_REPOS[*]}"

This too.



-- 
Eli Schwartz
Bug Wrangler and Trusted User

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.archlinux.org/pipermail/arch-projects/attachments/20180314/ab2e1710/attachment.asc>