{"thread":{"id":"55029","subject":"[PATCH 1/1] upload-pack.c: fix filter spec quoting bug","startedAt":"2021-01-22T14:23:00Z","lastAt":"2021-01-25T23:47:29Z","messageCount":13,"participants":["Jacob Vosmaer","Jeff King","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"414969","messageId":"20210122142137.21161-2-jacob@gitlab.com","threadId":"55029","inReplyTo":"20210122142137.21161-1-jacob@gitlab.com","subject":"[PATCH 1/1] upload-pack.c: fix filter spec quoting bug","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-22T14:21:37Z","receivedAt":"2021-01-22T14:23:00Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"This fixes a bug that occurs when you combine partial clone and\nuploadpack.packobjectshook. You can reproduce it as follows:\n\ngit clone -u 'git -c uploadpack.allowfilter '\\\n'-c uploadpack.packobjectshook=\" exec\" '\\\n'upload-pack' --filter=blob:none --no-local \\\nsrc.git dst.git\n\nBe careful with the line endings because this has a long quoted string\nas the -u argument. Note that there is an intentional space before\n'exec'. Without that space, run-command.c tries to be smart and the\ncommand fails for the wrong reason.\n\nThe error I get when I run this is:\n\nCloning into '/tmp/broken'...\nremote: fatal: invalid filter-spec ''blob:none''\nerror: git upload-pack: git-pack-objects died with error.\nfatal: git upload-pack: aborting due to possible repository corruption on the remote side.\nremote: aborting due to possible repository corruption on the remote side.\nfatal: early EOF\nfatal: index-pack failed\n\nThe problem is an unnecessary and harmful layer of quoting. I tried\ndigging through the history of this function and I think this quoting\nwas there from the start. My best guess is that it stems from a\nmisunderstanding of what use_shell=1 means. The code seems to assume\nit means \"arguments get joined into one big string, then fed to\n/bin/sh\". But that is not what it means: use_shell=1 means that the\nfirst argument in the arguments array may be a shell script and if so\nshould be passed to /bin/sh. All other arguments are passed as normal\narguments.\n\nThe solution is simple: never quote the filter spec.\n---\n upload-pack.c | 9 +--------\n 1 file changed, 1 insertion(+), 8 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3b66bf92ba..eae1fdbc55 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \tif (pack_data->filter_options.choice) {\n \t\tconst char *spec =\n \t\t\texpand_list_objects_filter_spec(&pack_data->filter_options);\n-\t\tif (pack_objects.use_shell) {\n-\t\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\t\tsq_quote_buf(&buf, spec);\n-\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", buf.buf);\n-\t\t\tstrbuf_release(&buf);\n-\t\t} else {\n-\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n-\t\t}\n+\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n \t}\n \tif (uri_protocols) {\n \t\tfor (i = 0; i < uri_protocols->nr; i++)\n-- \n2.30.0\n\n"},{"id":"414990","messageId":"20210122142137.21161-1-jacob@gitlab.com","threadId":"55029","inReplyTo":null,"subject":"[PATCH 0/1] upload-pack.c: fix filter spec quoting bug","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-22T14:21:36Z","receivedAt":"2021-01-22T19:36:00Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"This fixes a bug in upload-pack.c that only happens when you combine\npartial clone with a pack-objects hook.\n\n\nJacob Vosmaer (1):\n upload-pack.c: fix filter spec quoting bug\n\n upload-pack.c | 9 +--------\n 1 file changed, 1 insertion(+), 8 deletions(-)\n\n-- \n2.30.0\n\n"},{"id":"414992","messageId":"YAs2RMT1rEH/2LSp@coredump.intra.peff.net","threadId":"55029","inReplyTo":"20210122142137.21161-2-jacob@gitlab.com","subject":"Re: [PATCH 1/1] upload-pack.c: fix filter spec quoting bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-22T20:32:04Z","receivedAt":"2021-01-22T20:35:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 22, 2021 at 03:21:37PM +0100, Jacob Vosmaer wrote:\n\n> This fixes a bug that occurs when you combine partial clone and\n> uploadpack.packobjectshook. You can reproduce it as follows:\n> \n> git clone -u 'git -c uploadpack.allowfilter '\\\n> '-c uploadpack.packobjectshook=\" exec\" '\\\n> 'upload-pack' --filter=blob:none --no-local \\\n> src.git dst.git\n> \n> Be careful with the line endings because this has a long quoted string\n> as the -u argument. Note that there is an intentional space before\n> 'exec'. Without that space, run-command.c tries to be smart and the\n> command fails for the wrong reason.\n\nThe \"-u\" command is run with a shell, so:\n\n git clone \\\n -u 'git -c uploadpack.allowfilter \\\n -c uploadpack.packobjectshook=env \\\n\t upload-pack' \\\n --filter=blob:none --no-local src.git dst.git\n\nmay be a more readable version. I also found the use of \" exec\" clever,\nbut rather subtle; you need the extra space so that our \"don't bother\nusing a shell\" run-command optimization does not kick in. I replaced it\nwith \"env\" here, which is a slightly more canonical way of running a\nsub-program that does not rely on shell builtins.\n\nBut all of this should be added as a new test, probably in t5544 with\nthe other pack-objects hook tests.\n\n> The problem is an unnecessary and harmful layer of quoting. I tried\n> digging through the history of this function and I think this quoting\n> was there from the start. My best guess is that it stems from a\n> misunderstanding of what use_shell=1 means. The code seems to assume\n> it means \"arguments get joined into one big string, then fed to\n> /bin/sh\". But that is not what it means: use_shell=1 means that the\n> first argument in the arguments array may be a shell script and if so\n> should be passed to /bin/sh. All other arguments are passed as normal\n> arguments.\n\nYeah, that is exactly right. \"use_shell\" just means that the command is\n(possibly) run with a shell. Quoting for any extra arguments is handled\nautomatically.\n\nI think you're correct that this was broken from the start in 10ac85c785\n(upload-pack: add object filtering for partial clone, 2017-12-08).\nThat's even before the use_shell was added, and then later it was pushed\ninto that conditional by 0b6069fe0a (fetch-pack: test support excluding\nlarge blobs, 2017-12-08). Presumably because the non-hook path would not\nhave worked at all, and that was the first time any of it was actually\ntested. ;)\n\n(I've cc'd authors of those commits as an FYI; I think both were\nrelatively new to the project at the time so misunderstanding this\nsubtlety of run-command is not too surprising).\n\nI'm somewhat embarrassed to say that despite being the one who added the\npack-objects hook 4 years ago, we still have not switched over to it at\nGitHub from our custom patch (the reason is just mundane; there's some\nother adjustments that would have to happen and nobody has ever quite\ngotten around to it). Presumably you are looking to use it at GitLab.\nJust beware that you are probably treading new-ish ground, so there may\nbe other bugs like this lurking.\n\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 3b66bf92ba..eae1fdbc55 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n> \tif (pack_data->filter_options.choice) {\n> \t\tconst char *spec =\n> \t\t\texpand_list_objects_filter_spec(&pack_data->filter_options);\n> -\t\tif (pack_objects.use_shell) {\n> -\t\t\tstruct strbuf buf = STRBUF_INIT;\n> -\t\t\tsq_quote_buf(&buf, spec);\n> -\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", buf.buf);\n> -\t\t\tstrbuf_release(&buf);\n> -\t\t} else {\n> -\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n> -\t\t}\n> +\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n\nYep, this looks like the right fix. I think with an addition to the test\nsuite, this will be good to go.\n\n-Peff\n"},{"id":"414997","messageId":"YAs9pTBsdskC8CPN@coredump.intra.peff.net","threadId":"55029","inReplyTo":"YAs2RMT1rEH/2LSp@coredump.intra.peff.net","subject":"[PATCH] run-command: document use_shell option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-22T21:03:33Z","receivedAt":"2021-01-22T21:05:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 22, 2021 at 03:32:04PM -0500, Jeff King wrote:\n\n> Yeah, that is exactly right. \"use_shell\" just means that the command is\n> (possibly) run with a shell. Quoting for any extra arguments is handled\n> automatically.\n> \n> I think you're correct that this was broken from the start in 10ac85c785\n> (upload-pack: add object filtering for partial clone, 2017-12-08).\n> That's even before the use_shell was added, and then later it was pushed\n> into that conditional by 0b6069fe0a (fetch-pack: test support excluding\n> large blobs, 2017-12-08). Presumably because the non-hook path would not\n> have worked at all, and that was the first time any of it was actually\n> tested. ;)\n> \n> (I've cc'd authors of those commits as an FYI; I think both were\n> relatively new to the project at the time so misunderstanding this\n> subtlety of run-command is not too surprising).\n\nWhile we're thinking about it, let's beef up the documentation a bit.\n\n-- >8 --\nSubject: [PATCH] run-command: document use_shell option\n\nIt's unclear how run-command's use_shell option should impact the\narguments fed to a command. Plausibly it could mean that we glue all of\nthe arguments together into a string to pass to the shell, in which case\nthat opens the question of whether the caller needs to quote them.\n\nBut in fact we don't implement it that way (and even if we did, we'd\nprobably auto-quote the arguments as part of the glue step). And we must\nnot receive quoted arguments, because we might actually optimize out the\nshell entirely (i.e., the caller does not even know if a shell will be\ninvolved in the end or not).\n\nSince this ambiguity may have been the cause of a recent bug, let's\ndocument the option a bit.\n\nSigned-off-by: Jeff King \n---\n run-command.h | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.h b/run-command.h\nindex 6472b38bde..d08414a92e 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -126,8 +126,15 @@ struct child_process {\n \t */\n \tunsigned silent_exec_failure:1;\n \n-\tunsigned stdout_to_stderr:1;\n+\t/**\n+\t * Run the command from argv[0] using a shell (but note that we may\n+\t * still optimize out the shell call if the command contains no\n+\t * metacharacters). Note that further arguments to the command in\n+\t * argv[1], etc, do not need to be shell-quoted.\n+\t */\n \tunsigned use_shell:1;\n+\n+\tunsigned stdout_to_stderr:1;\n \tunsigned clean_on_exit:1;\n \tunsigned wait_after_clean:1;\n \tvoid (*clean_on_exit_handler)(struct child_process *process);\n-- \n2.30.0.664.g35e6628185\n\n"},{"id":"415002","messageId":"YAtEfuygxMhZpo2o@nand.local","threadId":"55029","inReplyTo":"YAs9pTBsdskC8CPN@coredump.intra.peff.net","subject":"Re: [PATCH] run-command: document use_shell option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-01-22T21:32:46Z","receivedAt":"2021-01-22T21:35:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jan 22, 2021 at 04:03:33PM -0500, Jeff King wrote:\n> Subject: [PATCH] run-command: document use_shell option\n\nNice. This does indeed make things a little clearer (and as we've seen\nit was certainly not as clear before), so I think this is worth doing.\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"415007","messageId":"xmqq4kj87hda.fsf@gitster.c.googlers.com","threadId":"55029","inReplyTo":"YAs2RMT1rEH/2LSp@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] upload-pack.c: fix filter spec quoting bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-22T22:10:25Z","receivedAt":"2021-01-22T22:11:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jan 22, 2021 at 03:21:37PM +0100, Jacob Vosmaer wrote:\n>\n>> This fixes a bug that occurs when you combine partial clone and\n>> uploadpack.packobjectshook. You can reproduce it as follows:\n> ...\n> I'm somewhat embarrassed to say that despite being the one who added the\n> pack-objects hook 4 years ago, we still have not switched over to it at\n> GitHub from our custom patch (the reason is just mundane; there's some\n> other adjustments that would have to happen and nobody has ever quite\n> gotten around to it). Presumably you are looking to use it at GitLab.\n> Just beware that you are probably treading new-ish ground, so there may\n> be other bugs like this lurking.\n>\n>> diff --git a/upload-pack.c b/upload-pack.c\n>> index 3b66bf92ba..eae1fdbc55 100644\n>> --- a/upload-pack.c\n>> +++ b/upload-pack.c\n>> @@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n>> \tif (pack_data->filter_options.choice) {\n>> \t\tconst char *spec =\n>> \t\t\texpand_list_objects_filter_spec(&pack_data->filter_options);\n>> -\t\tif (pack_objects.use_shell) {\n>> -\t\t\tstruct strbuf buf = STRBUF_INIT;\n>> -\t\t\tsq_quote_buf(&buf, spec);\n>> -\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", buf.buf);\n>> -\t\t\tstrbuf_release(&buf);\n>> -\t\t} else {\n>> -\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n>> -\t\t}\n>> +\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n>\n> Yep, this looks like the right fix. I think with an addition to the test\n> suite, this will be good to go.\n\nYeah, that looks simpler and better. It does deserve new tests.\n\nThanks, both.\n"},{"id":"415009","messageId":"xmqqzh1062as.fsf@gitster.c.googlers.com","threadId":"55029","inReplyTo":"YAs9pTBsdskC8CPN@coredump.intra.peff.net","subject":"Re: [PATCH] run-command: document use_shell option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-22T22:21:15Z","receivedAt":"2021-01-22T22:22:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/run-command.h b/run-command.h\n> index 6472b38bde..d08414a92e 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -126,8 +126,15 @@ struct child_process {\n> \t */\n> \tunsigned silent_exec_failure:1;\n> \n> -\tunsigned stdout_to_stderr:1;\n> +\t/**\n> +\t * Run the command from argv[0] using a shell (but note that we may\n> +\t * still optimize out the shell call if the command contains no\n> +\t * metacharacters). Note that further arguments to the command in\n> +\t * argv[1], etc, do not need to be shell-quoted.\n> +\t */\n> \tunsigned use_shell:1;\n> +\n> +\tunsigned stdout_to_stderr:1;\n\nReads well. Thanks.\n\nIt is curious why \"diff\" chose to move stdout_to_stderr line around,\nthough.\n\n> \tunsigned clean_on_exit:1;\n> \tunsigned wait_after_clean:1;\n> \tvoid (*clean_on_exit_handler)(struct child_process *process);\n"},{"id":"415026","messageId":"YAtoJ1ooPn9sdoGL@coredump.intra.peff.net","threadId":"55029","inReplyTo":"xmqqzh1062as.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] run-command: document use_shell option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-23T00:04:55Z","receivedAt":"2021-01-23T00:09:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 22, 2021 at 02:21:15PM -0800, Junio C Hamano wrote:\n\n> > -\tunsigned stdout_to_stderr:1;\n> > +\t/**\n> > +\t * Run the command from argv[0] using a shell (but note that we may\n> > +\t * still optimize out the shell call if the command contains no\n> > +\t * metacharacters). Note that further arguments to the command in\n> > +\t * argv[1], etc, do not need to be shell-quoted.\n> > +\t */\n> > \tunsigned use_shell:1;\n> > +\n> > +\tunsigned stdout_to_stderr:1;\n> \n> Reads well. Thanks.\n> \n> It is curious why \"diff\" chose to move stdout_to_stderr line around,\n> though.\n\nHeh, I also noticed that and tried a few options to no avail (both\npatience/histogram, but also diff.indentHeuristic). It is one deletion\nand 8 insertions even if it is done the other way:\n\n + /*\n + ... 6 more lines ...\n + */\n + unsigned use_shell:1;\n +\n unsigned stdout_to_stderr:1;\n - unsigned use_shell:1;\n\nso it may just come down to the order of xdiff parsing the lines.\n\n-Peff\n"},{"id":"415195","messageId":"20210125170921.14291-1-jacob@gitlab.com","threadId":"55029","inReplyTo":"YAs2RMT1rEH/2LSp@coredump.intra.peff.net","subject":"[PATCH v2] upload-pack.c: fix filter spec quoting bug","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-25T17:09:21Z","receivedAt":"2021-01-25T17:11:05Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"This fixes a bug that occurs when you combine partial clone and\nuploadpack.packobjectshook. You can reproduce it as follows:\n\ngit clone -u 'git -c uploadpack.allowfilter '\\\n'-c uploadpack.packobjectshook=env '\\\n'upload-pack' --filter=blob:none --no-local \\\nsrc.git dst.git\n\nBe careful with the line endings because this has a long quoted string\nas the -u argument.\n\nThe error I get when I run this is:\n\nCloning into '/tmp/broken'...\nremote: fatal: invalid filter-spec ''blob:none''\nerror: git upload-pack: git-pack-objects died with error.\nfatal: git upload-pack: aborting due to possible repository corruption on the remote side.\nremote: aborting due to possible repository corruption on the remote side.\nfatal: early EOF\nfatal: index-pack failed\n\nThe problem is an unnecessary and harmful layer of quoting. I tried\ndigging through the history of this function and I think this quoting\nwas there from the start. My best guess is that it stems from a\nmisunderstanding what use_shell=1 means. The code seems to assume it\nmeans \"arguments get joined into one big string, then fed to /bin/sh\".\nBut that is not what it means: use_shell=1 means that the first\nargument in the arguments array may be a shell script and if so should\nbe passed to /bin/sh. All other arguments are passed as normal\narguments.\n\nThe solution is simple: never quote the filter spec.\n\nThis commit removes the conditional quoting and adds a test for\npartial clone in t5544.\n---\n t/t5544-pack-objects-hook.sh | 9 +++++++++\n upload-pack.c | 9 +--------\n 2 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh\nindex 4357af1525..f5ba663d64 100755\n--- a/t/t5544-pack-objects-hook.sh\n+++ b/t/t5544-pack-objects-hook.sh\n@@ -59,4 +59,13 @@ test_expect_success 'hook does not run from repo config' '\n \ttest_path_is_missing .git/hook.stdout\n '\n \n+test_expect_success 'hook works with partial clone' '\n+\tclear_hook_results &&\n+\ttest_config_global uploadpack.packObjectsHook ./hook &&\n+\ttest_config_global uploadpack.allowFilter true &&\n+\tgit clone --bare --no-local --filter=blob:none . dst.git &&\n+\tgit -C dst.git rev-list --objects --missing=print HEAD >objects &&\n+\tgrep \"^?\" objects\n+'\n+\n test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3b66bf92ba..eae1fdbc55 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \tif (pack_data->filter_options.choice) {\n \t\tconst char *spec =\n \t\t\texpand_list_objects_filter_spec(&pack_data->filter_options);\n-\t\tif (pack_objects.use_shell) {\n-\t\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\t\tsq_quote_buf(&buf, spec);\n-\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", buf.buf);\n-\t\t\tstrbuf_release(&buf);\n-\t\t} else {\n-\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n-\t\t}\n+\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n \t}\n \tif (uri_protocols) {\n \t\tfor (i = 0; i < uri_protocols->nr; i++)\n-- \n2.30.0\n\n"},{"id":"415196","messageId":"CADMWQoMjsa3hpzQCk-4tzSxvGx4z1sMB5DuC+0a7NMq-rjfdEQ@mail.gmail.com","threadId":"55029","inReplyTo":"YAs2RMT1rEH/2LSp@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] upload-pack.c: fix filter spec quoting bug","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-25T17:14:28Z","receivedAt":"2021-01-25T17:32:33Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"Thanks, I changed the reproducing command to use 'env' and I added a\ntest case in t5544.\n\n\nBest regards,\n\nJacob Vosmaer\nGitLab, Inc.\n\nOn Fri, Jan 22, 2021 at 9:32 PM Jeff King wrote:\n>\n> On Fri, Jan 22, 2021 at 03:21:37PM +0100, Jacob Vosmaer wrote:\n>\n> > This fixes a bug that occurs when you combine partial clone and\n> > uploadpack.packobjectshook. You can reproduce it as follows:\n> >\n> > git clone -u 'git -c uploadpack.allowfilter '\\\n> > '-c uploadpack.packobjectshook=\" exec\" '\\\n> > 'upload-pack' --filter=blob:none --no-local \\\n> > src.git dst.git\n> >\n> > Be careful with the line endings because this has a long quoted string\n> > as the -u argument. Note that there is an intentional space before\n> > 'exec'. Without that space, run-command.c tries to be smart and the\n> > command fails for the wrong reason.\n>\n> The \"-u\" command is run with a shell, so:\n>\n> git clone \\\n> -u 'git -c uploadpack.allowfilter \\\n> -c uploadpack.packobjectshook=env \\\n> upload-pack' \\\n> --filter=blob:none --no-local src.git dst.git\n>\n> may be a more readable version. I also found the use of \" exec\" clever,\n> but rather subtle; you need the extra space so that our \"don't bother\n> using a shell\" run-command optimization does not kick in. I replaced it\n> with \"env\" here, which is a slightly more canonical way of running a\n> sub-program that does not rely on shell builtins.\n>\n> But all of this should be added as a new test, probably in t5544 with\n> the other pack-objects hook tests.\n>\n> > The problem is an unnecessary and harmful layer of quoting. I tried\n> > digging through the history of this function and I think this quoting\n> > was there from the start. My best guess is that it stems from a\n> > misunderstanding of what use_shell=1 means. The code seems to assume\n> > it means \"arguments get joined into one big string, then fed to\n> > /bin/sh\". But that is not what it means: use_shell=1 means that the\n> > first argument in the arguments array may be a shell script and if so\n> > should be passed to /bin/sh. All other arguments are passed as normal\n> > arguments.\n>\n> Yeah, that is exactly right. \"use_shell\" just means that the command is\n> (possibly) run with a shell. Quoting for any extra arguments is handled\n> automatically.\n>\n> I think you're correct that this was broken from the start in 10ac85c785\n> (upload-pack: add object filtering for partial clone, 2017-12-08).\n> That's even before the use_shell was added, and then later it was pushed\n> into that conditional by 0b6069fe0a (fetch-pack: test support excluding\n> large blobs, 2017-12-08). Presumably because the non-hook path would not\n> have worked at all, and that was the first time any of it was actually\n> tested. ;)\n>\n> (I've cc'd authors of those commits as an FYI; I think both were\n> relatively new to the project at the time so misunderstanding this\n> subtlety of run-command is not too surprising).\n>\n> I'm somewhat embarrassed to say that despite being the one who added the\n> pack-objects hook 4 years ago, we still have not switched over to it at\n> GitHub from our custom patch (the reason is just mundane; there's some\n> other adjustments that would have to happen and nobody has ever quite\n> gotten around to it). Presumably you are looking to use it at GitLab.\n> Just beware that you are probably treading new-ish ground, so there may\n> be other bugs like this lurking.\n>\n> > diff --git a/upload-pack.c b/upload-pack.c\n> > index 3b66bf92ba..eae1fdbc55 100644\n> > --- a/upload-pack.c\n> > +++ b/upload-pack.c\n> > @@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n> > if (pack_data->filter_options.choice) {\n> > const char *spec =\n> > expand_list_objects_filter_spec(&pack_data->filter_options);\n> > - if (pack_objects.use_shell) {\n> > - struct strbuf buf = STRBUF_INIT;\n> > - sq_quote_buf(&buf, spec);\n> > - strvec_pushf(&pack_objects.args, \"--filter=%s\", buf.buf);\n> > - strbuf_release(&buf);\n> > - } else {\n> > - strvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n> > - }\n> > + strvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n>\n> Yep, this looks like the right fix. I think with an addition to the test\n> suite, this will be good to go.\n>\n> -Peff\n"},{"id":"415198","messageId":"CADMWQoO5e29YO+F_iHT73PhO0T7nK2Q+tDsfCwSPBKG1Bur3tQ@mail.gmail.com","threadId":"55029","inReplyTo":"YAs2RMT1rEH/2LSp@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] upload-pack.c: fix filter spec quoting bug","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-25T17:41:23Z","receivedAt":"2021-01-25T17:51:18Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"On Fri, Jan 22, 2021 at 9:32 PM Jeff King wrote:\n> I also found the use of \" exec\" clever,\n> but rather subtle; you need the extra space so that our \"don't bother\n> using a shell\" run-command optimization does not kick in. I replaced it\n> with \"env\" here, which is a slightly more canonical way of running a\n> sub-program that does not rely on shell builtins.\nYes good idea, the exec thing is too clever for its own good.\n\n\n> But all of this should be added as a new test, probably in t5544 with\n> the other pack-objects hook tests.\n\nDid that, hope it is what you had in mind.\n\n\n> I'm somewhat embarrassed to say that despite being the one who added the\n> pack-objects hook 4 years ago, we still have not switched over to it at\n> GitHub from our custom patch (the reason is just mundane; there's some\n> other adjustments that would have to happen and nobody has ever quite\n> gotten around to it). Presumably you are looking to use it at GitLab.\n> Just beware that you are probably treading new-ish ground, so there may\n> be other bugs like this lurking.\n\nThanks for the heads up!\n\nJacob\n"},{"id":"415235","messageId":"xmqqlfcgyf0o.fsf@gitster.c.googlers.com","threadId":"55029","inReplyTo":"20210125170921.14291-1-jacob@gitlab.com","subject":"Re: [PATCH v2] upload-pack.c: fix filter spec quoting bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-25T19:48:07Z","receivedAt":"2021-01-25T20:43:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Vosmaer writes:\n\n> This fixes a bug that occurs when you combine partial clone and\n> uploadpack.packobjectshook. You can reproduce it as follows:\n>\n> git clone -u 'git -c uploadpack.allowfilter '\\\n> '-c uploadpack.packobjectshook=env '\\\n> 'upload-pack' --filter=blob:none --no-local \\\n> src.git dst.git\n>\n> Be careful with the line endings because this has a long quoted string\n> as the -u argument.\n>\n> The error I get when I run this is:\n>\n> Cloning into '/tmp/broken'...\n> remote: fatal: invalid filter-spec ''blob:none''\n> error: git upload-pack: git-pack-objects died with error.\n> fatal: git upload-pack: aborting due to possible repository corruption on the remote side.\n> remote: aborting due to possible repository corruption on the remote side.\n> fatal: early EOF\n> fatal: index-pack failed\n>\n> The problem is an unnecessary and harmful layer of quoting. I tried\n> digging through the history of this function and I think this quoting\n> was there from the start.\n\nMeaning that 10ac85c7 (upload-pack: add object filtering for partial\nclone, 2017-12-08) that added:\n\n if (filter_options.filter_spec) {\n struct strbuf buf = STRBUF_INIT;\n sq_quote_buf(&buf, filter_options.filter_spec);\n argv_array_pushf(&pack_objects.args, \"--filter=%s\", buf.buf);\n strbuf_release(&buf);\n }\n\n> My best guess is that it stems from a\n> misunderstanding what use_shell=1 means. The code seems to assume it\n> means \"arguments get joined into one big string, then fed to /bin/sh\".\n> But that is not what it means: use_shell=1 means that the first\n> argument in the arguments array may be a shell script and if so should\n> be passed to /bin/sh. All other arguments are passed as normal\n> arguments.\n\nI noticed another thing that hasn't changed since that commit, which\nis that the setting of .use_shell is conditional. In today's code,\nat the beginning of create_pack_file(), we have\n\n if (!pack_data->pack_objects_hook)\n pack_objects.git_cmd = 1;\n else {\n strvec_push(&pack_objects.args, pack_data->pack_objects_hook);\n strvec_push(&pack_objects.args, \"git\");\n pack_objects.use_shell = 1;\n }\n\nI suspect that 0b6069fe (fetch-pack: test support excluding large\nblobs, 2017-12-08) sort-of fixed half of the problem (i.e. the half\nwhen there is no hook used) while leaving the other half still\nbroken as before.\n\nBut because .use_shell does not affect if we should or should not\nquote, we can unconditionally drop the use of sq_quote_buf().\n\n> The solution is simple: never quote the filter spec.\n\nWhich makes sense.\n\n> This commit removes the conditional quoting and adds a test for\n> partial clone in t5544.\n> ---\n\nThanks. Missing sign-off.\n\n> \tif (pack_data->filter_options.choice) {\n> \t\tconst char *spec =\n> \t\t\texpand_list_objects_filter_spec(&pack_data->filter_options);\n> -\t\tif (pack_objects.use_shell) {\n> -\t\t\tstruct strbuf buf = STRBUF_INIT;\n> -\t\t\tsq_quote_buf(&buf, spec);\n> -\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", buf.buf);\n> -\t\t\tstrbuf_release(&buf);\n> -\t\t} else {\n> -\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n> -\t\t}\n> +\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n> \t}\n> \tif (uri_protocols) {\n> \t\tfor (i = 0; i < uri_protocols->nr; i++)\n"},{"id":"415260","messageId":"YA81EwddIG7QVI/1@coredump.intra.peff.net","threadId":"55029","inReplyTo":"20210125170921.14291-1-jacob@gitlab.com","subject":"Re: [PATCH v2] upload-pack.c: fix filter spec quoting bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-25T21:16:03Z","receivedAt":"2021-01-25T23:47:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 25, 2021 at 06:09:21PM +0100, Jacob Vosmaer wrote:\n\n> +test_expect_success 'hook works with partial clone' '\n> +\tclear_hook_results &&\n> +\ttest_config_global uploadpack.packObjectsHook ./hook &&\n> +\ttest_config_global uploadpack.allowFilter true &&\n\nI was going to complain that:\n\n test_config -C dst.git uploadpack.packObjectsHook ./hook &&\n test_config -C dst.git uploadpack.allowFilter true &&\n\nwould be more clear, since it tells us which repo we care about\nimpacting. But the rest of the script doesn't do that, and indeed it\ncan't, because we don't allow packObjectsHook to be set in per-repo\nconfig for security reasons. :)\n\n(We could do it per-repo for allowFilter, but I think it is just as well\nto keep the two calls consistent).\n\n> +\tgit clone --bare --no-local --filter=blob:none . dst.git &&\n> +\tgit -C dst.git rev-list --objects --missing=print HEAD >objects &&\n> +\tgrep \"^?\" objects\n\nPreviously that clone would fail, so the bug-fix is demonstrated by it\nsucceeding. But the other two commands are added to make sure that we\nactually did apply the filter correctly. Even better. Thanks for being\nthorough.\n\n-Peff\n"}]}