{"thread":{"id":"44350","subject":"[BUG] [PATCH]: run-command.c","startedAt":"2016-10-21T06:16:21Z","lastAt":"2016-10-21T17:19:15Z","messageCount":4,"participants":["Duncan Roe","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"304559","messageId":"20161021055013.GA31554@dimstar.local.net","threadId":"44350","inReplyTo":null,"subject":"[BUG] [PATCH]: run-command.c","fromName":"Duncan Roe","fromEmail":"duncan_roe@acslink.net.au","sentAt":"2016-10-21T05:50:13Z","receivedAt":"2016-10-21T06:16:21Z","isPatch":true,"sender":{"key":"duncan_roe@acslink.net.au","avatar":null},"body":"prepare_shell_cmd() executes /bin/sh with superfluous arguments on all but\nsingle-word shell commands.\n\nFor example, if .git/config has this alias (the sleep is to leave time to\nexamine output from ps, &c.):\n\n[alias]\n\ttryme = \"!echo $PWD;sleep 600\"\n\nrunning \"git tryme\" in one console and checking what it does in another\n\n--- 1st xterm\n\n16:42:12$ git tryme\n/usr/src/git/.git\necho $PWD;sleep 600: line 1:  2602 Terminated              sleep 600\n16:43:15$\n\n\n--- 2nd xterm\n\n16:42:06$ ps axf|grep -A2 trym[e]\n 2599 pts/4    S+     0:00      \\_ git tryme\n 2601 pts/4    S+     0:00          \\_ /bin/sh -c echo $PWD;sleep 600 echo $PWD;sleep 600\n 2602 pts/4    S+     0:00              \\_ sleep 600\n16:42:45$ cat /proc/2601/cmdline | xargs -0 -n1 echo\n/bin/sh\n-c\necho $PWD;sleep 600\necho $PWD;sleep 600\n16:43:04$ kill 2602\n16:43:15$\n\n---\n\nThere is an extra \"-c\" argument. This is caused by a missing \"else\", fixed by\nthe appended patch,\n\nCheers ... Duncan.\n\n----------8<-------------------\n\n--- a/run-command.c\n+++ b/run-command.c\n@@ -182,8 +182,8 @@ static const char **prepare_shell_cmd(struct argv_array *out, const char **argv)\n \t\telse\n \t\t\targv_array_pushf(out, \"%s \\\"$@\\\"\", argv[0]);\n \t}\n-\n-\targv_array_pushv(out, argv);\n+\telse\n+\t\targv_array_pushv(out, argv);\n \treturn out->argv;\n }\n\n"},{"id":"304563","messageId":"20161021090029.glr5u6gwrxluavir@sigill.intra.peff.net","threadId":"44350","inReplyTo":"20161021055013.GA31554@dimstar.local.net","subject":"Re: [BUG] [PATCH]: run-command.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-21T09:00:29Z","receivedAt":"2016-10-21T09:00:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2016 at 04:50:13PM +1100, Duncan Roe wrote:\n\n> For example, if .git/config has this alias (the sleep is to leave time to\n> examine output from ps, &c.):\n> \n> [alias]\n> \ttryme = \"!echo $PWD;sleep 600\"\n>\n> [...]\n> 16:42:06$ ps axf|grep -A2 trym[e]\n>  2599 pts/4    S+     0:00      \\_ git tryme\n>  2601 pts/4    S+     0:00          \\_ /bin/sh -c echo $PWD;sleep 600 echo $PWD;sleep 600\n>  2602 pts/4    S+     0:00              \\_ sleep 600\n> 16:42:45$ cat /proc/2601/cmdline | xargs -0 -n1 echo\n> /bin/sh\n> -c\n> echo $PWD;sleep 600\n> echo $PWD;sleep 600\n\nThis duplicated argument is expected and normal. The arguments after \"-c\nwhatever\" become positional parameters $0, $1, etc. The actual script\narguments start at \"$1\", and \"$0\" is typically the \"script name\".\nSo you have to stick some placeholder value in the \"$0\" slot, so that\nthe sub-script can find the actual arguments. E.g., try:\n\n  sh -c '\n    for i in \"$@\"; do\n      echo \"got $i\"\n    done\n  ' one two three\n\nit will print only:\n\n  got two\n  got three\n\nBut if you stick a placeholder there, it works:\n\n  sh -c '\n    for i in \"$@\"; do\n      echo \"got $i\"\n    done\n  ' placeholder one two three\n\nThe value of the placeholder does not matter to the shell. But it is\naccessible to the script inside via $0:\n\n  sh -c '\n    echo \"\\$0 = $0\"\n    echo \"\\$1 = $1\"\n    echo \"\\$2 = $2\"\n    echo \"\\$3 = $3\"\n  ' placeholder one two three\n\nSince our script does not have a filename, we just stick the script\ncontents there (which is really just a convention, and one I doubt\nanybody is really relying on, but there's no point in breaking it now).\n\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -182,8 +182,8 @@ static const char **prepare_shell_cmd(struct argv_array *out, const char **argv)\n>  \t\telse\n>  \t\t\targv_array_pushf(out, \"%s \\\"$@\\\"\", argv[0]);\n>  \t}\n> -\n> -\targv_array_pushv(out, argv);\n> +\telse\n> +\t\targv_array_pushv(out, argv);\n>  \treturn out->argv;\n>  }\n\nTry running \"make test\" with this. Lots of things break, because we are\nnot sending the positional parameters to the shell script at all.\n\nIf we just cared about the positional parmeters, we _could_ do something\nlike:\n\n  if (argv[0]) {\n\targv_array_push(out, \"sh\");\n\targv_array_pushv(out, argv + 1);\n  }\n\nThat would omit \"$0\" entirely when we have no positional parameters (and\nthe shell generally fills in \"sh\" there itself), and provide a dummy\n\"sh\" value when we need to use it as a placeholder.\n\nBut again, there's no real value in breaking the existing convention.\n\n-Peff\n"},{"id":"304572","messageId":"20161021110728.GB31554@dimstar.local.net","threadId":"44350","inReplyTo":"20161021090029.glr5u6gwrxluavir@sigill.intra.peff.net","subject":"Re: [BUG] [PATCH]: run-command.c","fromName":"Duncan Roe","fromEmail":"duncan_roe@acslink.net.au","sentAt":"2016-10-21T11:07:28Z","receivedAt":"2016-10-21T11:07:58Z","isPatch":true,"sender":{"key":"duncan_roe@acslink.net.au","avatar":null},"body":"On Fri, Oct 21, 2016 at 05:00:29AM -0400, Jeff King wrote:\n> On Fri, Oct 21, 2016 at 04:50:13PM +1100, Duncan Roe wrote:\n>\n> > For example, if .git/config has this alias (the sleep is to leave time to\n> > examine output from ps, &c.):\n> >\n> > [alias]\n> > \ttryme = \"!echo $PWD;sleep 600\"\n> >\n> > [...]\n> > 16:42:06$ ps axf|grep -A2 trym[e]\n> >  2599 pts/4    S+     0:00      \\_ git tryme\n> >  2601 pts/4    S+     0:00          \\_ /bin/sh -c echo $PWD;sleep 600 echo $PWD;sleep 600\n> >  2602 pts/4    S+     0:00              \\_ sleep 600\n> > 16:42:45$ cat /proc/2601/cmdline | xargs -0 -n1 echo\n> > /bin/sh\n> > -c\n> > echo $PWD;sleep 600\n> > echo $PWD;sleep 600\n>\n> This duplicated argument is expected and normal. The arguments after \"-c\n> whatever\" become positional parameters $0, $1, etc. The actual script\n> arguments start at \"$1\", and \"$0\" is typically the \"script name\".\n> So you have to stick some placeholder value in the \"$0\" slot, so that\n> the sub-script can find the actual arguments. E.g., try:\n>\n>   sh -c '\n>     for i in \"$@\"; do\n>       echo \"got $i\"\n>     done\n>   ' one two three\n>\n> it will print only:\n>\n>   got two\n>   got three\n>\n> But if you stick a placeholder there, it works:\n>\n>   sh -c '\n>     for i in \"$@\"; do\n>       echo \"got $i\"\n>     done\n>   ' placeholder one two three\n>\n> The value of the placeholder does not matter to the shell. But it is\n> accessible to the script inside via $0:\n>\n>   sh -c '\n>     echo \"\\$0 = $0\"\n>     echo \"\\$1 = $1\"\n>     echo \"\\$2 = $2\"\n>     echo \"\\$3 = $3\"\n>   ' placeholder one two three\n>\n> Since our script does not have a filename, we just stick the script\n> contents there (which is really just a convention, and one I doubt\n> anybody is really relying on, but there's no point in breaking it now).\n>\n> > --- a/run-command.c\n> > +++ b/run-command.c\n> > @@ -182,8 +182,8 @@ static const char **prepare_shell_cmd(struct argv_array *out, const char **argv)\n> >  \t\telse\n> >  \t\t\targv_array_pushf(out, \"%s \\\"$@\\\"\", argv[0]);\n> >  \t}\n> > -\n> > -\targv_array_pushv(out, argv);\n> > +\telse\n> > +\t\targv_array_pushv(out, argv);\n> >  \treturn out->argv;\n> >  }\n>\n> Try running \"make test\" with this. Lots of things break, because we are\n> not sending the positional parameters to the shell script at all.\n>\n> If we just cared about the positional parmeters, we _could_ do something\n> like:\n>\n>   if (argv[0]) {\n> \targv_array_push(out, \"sh\");\n> \targv_array_pushv(out, argv + 1);\n>   }\n>\n> That would omit \"$0\" entirely when we have no positional parameters (and\n> the shell generally fills in \"sh\" there itself), and provide a dummy\n> \"sh\" value when we need to use it as a placeholder.\n>\n> But again, there's no real value in breaking the existing convention.\n>\n> -Peff\nAgreed - tests 110 and 111 in t1300-repo-config.sh fail. After that, \"make test\"\ngives up, losing about 14000 lines of output.\n\nSorry for the noise,\n\nCheers ... Duncan.\n"},{"id":"304624","messageId":"xmqq37jpd3hw.fsf@gitster.mtv.corp.google.com","threadId":"44350","inReplyTo":"20161021090029.glr5u6gwrxluavir@sigill.intra.peff.net","subject":"Re: [BUG] [PATCH]: run-command.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-21T17:19:07Z","receivedAt":"2016-10-21T17:19:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> 16:42:45$ cat /proc/2601/cmdline | xargs -0 -n1 echo\n>> /bin/sh\n>> -c\n>> echo $PWD;sleep 600\n>> echo $PWD;sleep 600\n>\n> This duplicated argument is expected and normal.\n\nWell explained.  Thanks.\n"}]}