Re: [PATCH] Documentation/install-webdoc.sh: quote a potentially unsafe shell expansion

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 1, 2016, 20:42 UTC
Message-ID: <xmqqfum78jq0.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CACC5Q1eFM_G4wKopkbxabLEu8+nbt66wF1jKSoTuL1vnS5Tb4Q@mail.gmail.com>

Austin English <austinenglish@gmail.com> writes:

Show 13 quoted lines

> diff --git a/Documentation/install-webdoc.sh b/Documentation/install-webdoc.sh
> index ed8b4ff..5fb2dc5 100755
> --- a/Documentation/install-webdoc.sh
> +++ b/Documentation/install-webdoc.sh
> @@ -18,7 +18,7 @@ do
>  	else
>  		echo >&2 "# install $h $T/$h"
>  		rm -f "$T/$h"
> -		mkdir -p $(dirname "$T/$h")
> +		mkdir -p "$(dirname "$T/$h")"
>  		cp "$h" "$T/$h"
>  	fi
>  done

We know $h is safe without quoting (see what the for loop iterates over a list and binding each element of it to this variable), but T is the parameter given to this script, which comes from these

install-html: html
	'$(SHELL_PATH_SQ)' ./install-webdoc.sh $(DESTDIR)$(htmldir)

install-webdoc : html
	'$(SHELL_PATH_SQ)' ./install-webdoc.sh $(WEBDOC_DEST)

in the Makefile. So quoting the result of $(dirname "$T/$h") is just as necessary as quoting the argument given to this dirname.

But I do not think it is sufficient, if we are truly worried about people who specify a path that contains IFS whitespace in DESTDIR, WEBDOC_DEST, htmldir and other *dir variables used in the Makefile. The references to these variables, when they are mentioned on the command lines of Makefile actions, all need to be quoted. The remainder of the Makefile tells me that we decided that we are not worried about those people at all.

So while I could take your patch as-is, I am not sure how much value it adds to the overall callchain that would reach the location that is updated by the patch. If you run

	make DESTDIR="/tmp/My Temporary Place" install

it would still not do the right thing even with your patch, I would suspect.

Thanks.