{"thread":{"id":"44580","subject":"[PATCH] Documentation/install-webdoc.sh: quote a potentially unsafe shell expansion","startedAt":"2016-12-01T01:23:41Z","lastAt":"2016-12-02T01:41:31Z","messageCount":3,"participants":["Austin English","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"306657","messageId":"CACC5Q1eFM_G4wKopkbxabLEu8+nbt66wF1jKSoTuL1vnS5Tb4Q@mail.gmail.com","threadId":"44580","inReplyTo":null,"subject":"[PATCH] Documentation/install-webdoc.sh: quote a potentially unsafe shell expansion","fromName":"Austin English","fromEmail":"austinenglish@gmail.com","sentAt":"2016-12-01T01:22:56Z","receivedAt":"2016-12-01T01:23:41Z","isPatch":true,"sender":{"key":"austinenglish@gmail.com","avatar":null},"body":"Found via shellcheck\n\nIn Documentation/install-webdoc.sh line 21:\nmkdir -p $(dirname \"$T/$h\")\n                         ^-- SC2046: Quote this to prevent word splitting.\n\n-- \n-Austin\nGPG: 14FB D7EA A041 937B\n\n\nFrom 1050538f252d22311185065ab8837c71b17003fb Mon Sep 17 00:00:00 2001\nFrom: Austin English <austinenglish@gmail.com>\nDate: Wed, 30 Nov 2016 19:21:25 -0600\nSubject: [PATCH] Documentation/install-webdoc.sh: quote a potentially unsafe\n shell expansion\n\nSigned-off-by: Austin English <austinenglish@gmail.com>\n---\n Documentation/install-webdoc.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/install-webdoc.sh b/Documentation/install-webdoc.sh\nindex ed8b4ff..5fb2dc5 100755\n--- a/Documentation/install-webdoc.sh\n+++ b/Documentation/install-webdoc.sh\n@@ -18,7 +18,7 @@ do\n \telse\n \t\techo >&2 \"# install $h $T/$h\"\n \t\trm -f \"$T/$h\"\n-\t\tmkdir -p $(dirname \"$T/$h\")\n+\t\tmkdir -p \"$(dirname \"$T/$h\")\"\n \t\tcp \"$h\" \"$T/$h\"\n \tfi\n done\n-- \n2.7.3\n\n"},{"id":"306749","messageId":"xmqqfum78jq0.fsf@gitster.mtv.corp.google.com","threadId":"44580","inReplyTo":"CACC5Q1eFM_G4wKopkbxabLEu8+nbt66wF1jKSoTuL1vnS5Tb4Q@mail.gmail.com","subject":"Re: [PATCH] Documentation/install-webdoc.sh: quote a potentially unsafe shell expansion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-01T20:42:15Z","receivedAt":"2016-12-01T20:42:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Austin English <austinenglish@gmail.com> writes:\n\n> diff --git a/Documentation/install-webdoc.sh b/Documentation/install-webdoc.sh\n> index ed8b4ff..5fb2dc5 100755\n> --- a/Documentation/install-webdoc.sh\n> +++ b/Documentation/install-webdoc.sh\n> @@ -18,7 +18,7 @@ do\n>  \telse\n>  \t\techo >&2 \"# install $h $T/$h\"\n>  \t\trm -f \"$T/$h\"\n> -\t\tmkdir -p $(dirname \"$T/$h\")\n> +\t\tmkdir -p \"$(dirname \"$T/$h\")\"\n>  \t\tcp \"$h\" \"$T/$h\"\n>  \tfi\n>  done\n\nWe know $h is safe without quoting (see what the for loop iterates\nover a list and binding each element of it to this variable), but T\nis the parameter given to this script, which comes from these\n\ninstall-html: html\n\t'$(SHELL_PATH_SQ)' ./install-webdoc.sh $(DESTDIR)$(htmldir)\n\ninstall-webdoc : html\n\t'$(SHELL_PATH_SQ)' ./install-webdoc.sh $(WEBDOC_DEST)\n\nin the Makefile.  So quoting the result of $(dirname \"$T/$h\") is\njust as necessary as quoting the argument given to this dirname.\n\nBut I do not think it is sufficient, if we are truly worried about\npeople who specify a path that contains IFS whitespace in DESTDIR,\nWEBDOC_DEST, htmldir and other *dir variables used in the Makefile.\nThe references to these variables, when they are mentioned on the\ncommand lines of Makefile actions, all need to be quoted.  The\nremainder of the Makefile tells me that we decided that we are not\nworried about those people at all.\n\nSo while I could take your patch as-is, I am not sure how much value\nit adds to the overall callchain that would reach the location that\nis updated by the patch.  If you run\n\n\tmake DESTDIR=\"/tmp/My Temporary Place\" install\n\nit would still not do the right thing even with your patch, I would\nsuspect.\n\nThanks.\n"},{"id":"306791","messageId":"CACC5Q1dfbDcDdFNmbsM63nkLWzbE6WXxLrTLZm5YTRTbVtgoOQ@mail.gmail.com","threadId":"44580","inReplyTo":"xmqqfum78jq0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Documentation/install-webdoc.sh: quote a potentially unsafe shell expansion","fromName":"Austin English","fromEmail":"austinenglish@gmail.com","sentAt":"2016-12-02T01:40:44Z","receivedAt":"2016-12-02T01:41:31Z","isPatch":true,"sender":{"key":"austinenglish@gmail.com","avatar":null},"body":"On Thu, Dec 1, 2016 at 2:42 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Austin English <austinenglish@gmail.com> writes:\n>\n>> diff --git a/Documentation/install-webdoc.sh b/Documentation/install-webdoc.sh\n>> index ed8b4ff..5fb2dc5 100755\n>> --- a/Documentation/install-webdoc.sh\n>> +++ b/Documentation/install-webdoc.sh\n>> @@ -18,7 +18,7 @@ do\n>>       else\n>>               echo >&2 \"# install $h $T/$h\"\n>>               rm -f \"$T/$h\"\n>> -             mkdir -p $(dirname \"$T/$h\")\n>> +             mkdir -p \"$(dirname \"$T/$h\")\"\n>>               cp \"$h\" \"$T/$h\"\n>>       fi\n>>  done\n>\n> We know $h is safe without quoting (see what the for loop iterates\n> over a list and binding each element of it to this variable), but T\n> is the parameter given to this script, which comes from these\n>\n> install-html: html\n>         '$(SHELL_PATH_SQ)' ./install-webdoc.sh $(DESTDIR)$(htmldir)\n>\n> install-webdoc : html\n>         '$(SHELL_PATH_SQ)' ./install-webdoc.sh $(WEBDOC_DEST)\n>\n> in the Makefile.  So quoting the result of $(dirname \"$T/$h\") is\n> just as necessary as quoting the argument given to this dirname.\n>\n> But I do not think it is sufficient, if we are truly worried about\n> people who specify a path that contains IFS whitespace in DESTDIR,\n> WEBDOC_DEST, htmldir and other *dir variables used in the Makefile.\n> The references to these variables, when they are mentioned on the\n> command lines of Makefile actions, all need to be quoted.  The\n> remainder of the Makefile tells me that we decided that we are not\n> worried about those people at all.\n>\n> So while I could take your patch as-is, I am not sure how much value\n> it adds to the overall callchain that would reach the location that\n> is updated by the patch.  If you run\n>\n>         make DESTDIR=\"/tmp/My Temporary Place\" install\n>\n> it would still not do the right thing even with your patch, I would\n> suspect.\n>\n> Thanks.\n\nHi Junio,\n\nThanks for reply and reviewing. Your concerns are totally valid.\n\nSome context for the change. I wrote a wrapper script for\ncheckbashisms/shellcheck that I use in my project. I decided to run it\non other projects I have checked out, out of curiosity, and looked at\nsome of the results. This was the only one in git, so I thought it was\nworth fixing. I did not test the full pipeline.\n\nI'll look again and send a follow up patch soon.\n\n-- \n-Austin\nGPG: 14FB D7EA A041 937B\n"}]}