Austin English [off-list ref] writes:
quoted hunk
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.
On Thu, Dec 1, 2016 at 2:42 PM, Junio C Hamano [off-list ref] wrote:
Austin English [off-list ref] writes:
quoted
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.
Hi Junio,
Thanks for reply and reviewing. Your concerns are totally valid.
Some context for the change. I wrote a wrapper script for
checkbashisms/shellcheck that I use in my project. I decided to run it
on other projects I have checked out, out of curiosity, and looked at
some of the results. This was the only one in git, so I thought it was
worth fixing. I did not test the full pipeline.
I'll look again and send a follow up patch soon.
--
-Austin
GPG: 14FB D7EA A041 937B