[PATCH 0/1] upload-pack.c: fix filter spec quoting bug

This fixes a bug in upload-pack.c that only happens when you combine
partial clone with a pack-objects hook.


Jacob Vosmaer (1):
  upload-pack.c: fix filter spec quoting bug

 upload-pack.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

-- 
2.30.0

This fixes a bug that occurs when you combine partial clone and
uploadpack.packobjectshook. You can reproduce it as follows:

git clone -u 'git -c uploadpack.allowfilter '\
'-c uploadpack.packobjectshook=" exec" '\
'upload-pack' --filter=blob:none --no-local \
src.git dst.git

Be careful with the line endings because this has a long quoted string
as the -u argument. Note that there is an intentional space before
'exec'. Without that space, run-command.c tries to be smart and the
command fails for the wrong reason.

The error I get when I run this is:

Cloning into '/tmp/broken'...
remote: fatal: invalid filter-spec ''blob:none''
error: git upload-pack: git-pack-objects died with error.
fatal: git upload-pack: aborting due to possible repository corruption on the remote side.
remote: aborting due to possible repository corruption on the remote side.
fatal: early EOF
fatal: index-pack failed

The problem is an unnecessary and harmful layer of quoting. I tried
digging through the history of this function and I think this quoting
was there from the start. My best guess is that it stems from a
misunderstanding of what use_shell=1 means. The code seems to assume
it means "arguments get joined into one big string, then fed to
/bin/sh". But that is not what it means: use_shell=1 means that the
first argument in the arguments array may be a shell script and if so
should be passed to /bin/sh. All other arguments are passed as normal
arguments.

The solution is simple: never quote the filter spec.
---
 upload-pack.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 3b66bf92ba..eae1fdbc55 100644
--- a/upload-pack.c
+++ b/upload-pack.c

@@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,
 	if (pack_data->filter_options.choice) {
 		const char *spec =
 			expand_list_objects_filter_spec(&pack_data->filter_options);
-		if (pack_objects.use_shell) {
-			struct strbuf buf = STRBUF_INIT;
-			sq_quote_buf(&buf, spec);
-			strvec_pushf(&pack_objects.args, "--filter=%s", buf.buf);
-			strbuf_release(&buf);
-		} else {
-			strvec_pushf(&pack_objects.args, "--filter=%s", spec);
-		}
+		strvec_pushf(&pack_objects.args, "--filter=%s", spec);
 	}
 	if (uri_protocols) {
 		for (i = 0; i < uri_protocols->nr; i++)

-- 
2.30.0

On Fri, Jan 22, 2021 at 03:21:37PM +0100, Jacob Vosmaer wrote:

This fixes a bug that occurs when you combine partial clone and
uploadpack.packobjectshook. You can reproduce it as follows:

git clone -u 'git -c uploadpack.allowfilter '\
'-c uploadpack.packobjectshook=" exec" '\
'upload-pack' --filter=blob:none --no-local \
src.git dst.git

Be careful with the line endings because this has a long quoted string
as the -u argument. Note that there is an intentional space before
'exec'. Without that space, run-command.c tries to be smart and the
command fails for the wrong reason.

The "-u" command is run with a shell, so:

  git clone \
    -u 'git -c uploadpack.allowfilter \
            -c uploadpack.packobjectshook=env \
	    upload-pack' \
  --filter=blob:none --no-local src.git dst.git

may be a more readable version. I also found the use of " exec" clever,
but rather subtle; you need the extra space so that our "don't bother
using a shell" run-command optimization does not kick in. I replaced it
with "env" here, which is a slightly more canonical way of running a
sub-program that does not rely on shell builtins.

But all of this should be added as a new test, probably in t5544 with
the other pack-objects hook tests.

The problem is an unnecessary and harmful layer of quoting. I tried
digging through the history of this function and I think this quoting
was there from the start. My best guess is that it stems from a
misunderstanding of what use_shell=1 means. The code seems to assume
it means "arguments get joined into one big string, then fed to
/bin/sh". But that is not what it means: use_shell=1 means that the
first argument in the arguments array may be a shell script and if so
should be passed to /bin/sh. All other arguments are passed as normal
arguments.

Yeah, that is exactly right. "use_shell" just means that the command is
(possibly) run with a shell. Quoting for any extra arguments is handled
automatically.

I think you're correct that this was broken from the start in 10ac85c785
(upload-pack: add object filtering for partial clone, 2017-12-08).
That's even before the use_shell was added, and then later it was pushed
into that conditional by 0b6069fe0a (fetch-pack: test support excluding
large blobs, 2017-12-08). Presumably because the non-hook path would not
have worked at all, and that was the first time any of it was actually
tested. ;)

(I've cc'd authors of those commits as an FYI; I think both were
relatively new to the project at the time so misunderstanding this
subtlety of run-command is not too surprising).

I'm somewhat embarrassed to say that despite being the one who added the
pack-objects hook 4 years ago, we still have not switched over to it at
GitHub from our custom patch (the reason is just mundane; there's some
other adjustments that would have to happen and nobody has ever quite
gotten around to it). Presumably you are looking to use it at GitLab.
Just beware that you are probably treading new-ish ground, so there may
be other bugs like this lurking.

Yep, this looks like the right fix. I think with an addition to the test
suite, this will be good to go.

-Peff

On Fri, Jan 22, 2021 at 03:32:04PM -0500, Jeff King wrote:

Yeah, that is exactly right. "use_shell" just means that the command is
(possibly) run with a shell. Quoting for any extra arguments is handled
automatically.

I think you're correct that this was broken from the start in 10ac85c785
(upload-pack: add object filtering for partial clone, 2017-12-08).
That's even before the use_shell was added, and then later it was pushed
into that conditional by 0b6069fe0a (fetch-pack: test support excluding
large blobs, 2017-12-08). Presumably because the non-hook path would not
have worked at all, and that was the first time any of it was actually
tested. ;)

(I've cc'd authors of those commits as an FYI; I think both were
relatively new to the project at the time so misunderstanding this
subtlety of run-command is not too surprising).

While we're thinking about it, let's beef up the documentation a bit.

-- >8 --
Subject: [PATCH] run-command: document use_shell option

It's unclear how run-command's use_shell option should impact the
arguments fed to a command. Plausibly it could mean that we glue all of
the arguments together into a string to pass to the shell, in which case
that opens the question of whether the caller needs to quote them.

But in fact we don't implement it that way (and even if we did, we'd
probably auto-quote the arguments as part of the glue step). And we must
not receive quoted arguments, because we might actually optimize out the
shell entirely (i.e., the caller does not even know if a shell will be
involved in the end or not).

Since this ambiguity may have been the cause of a recent bug, let's
document the option a bit.

Signed-off-by: Jeff King <redacted>
---
 run-command.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/run-command.h b/run-command.h
index 6472b38bde..d08414a92e 100644
--- a/run-command.h
+++ b/run-command.h

@@ -126,8 +126,15 @@ struct child_process {
 	 */
 	unsigned silent_exec_failure:1;
 
-	unsigned stdout_to_stderr:1;
+	/**
+	 * Run the command from argv[0] using a shell (but note that we may
+	 * still optimize out the shell call if the command contains no
+	 * metacharacters). Note that further arguments to the command in
+	 * argv[1], etc, do not need to be shell-quoted.
+	 */
 	unsigned use_shell:1;
+
+	unsigned stdout_to_stderr:1;
 	unsigned clean_on_exit:1;
 	unsigned wait_after_clean:1;
 	void (*clean_on_exit_handler)(struct child_process *process);

-- 
2.30.0.664.g35e6628185

On Fri, Jan 22, 2021 at 04:03:33PM -0500, Jeff King wrote:

Subject: [PATCH] run-command: document use_shell option

Nice. This does indeed make things a little clearer (and as we've seen
it was certainly not as clear before), so I think this is worth doing.

  Reviewed-by: Taylor Blau [off-list ref]

Thanks,
Taylor

This fixes a bug that occurs when you combine partial clone and
uploadpack.packobjectshook. You can reproduce it as follows:

git clone -u 'git -c uploadpack.allowfilter '\
'-c uploadpack.packobjectshook=env '\
'upload-pack' --filter=blob:none --no-local \
src.git dst.git

Be careful with the line endings because this has a long quoted string
as the -u argument.

The error I get when I run this is:

Cloning into '/tmp/broken'...
remote: fatal: invalid filter-spec ''blob:none''
error: git upload-pack: git-pack-objects died with error.
fatal: git upload-pack: aborting due to possible repository corruption on the remote side.
remote: aborting due to possible repository corruption on the remote side.
fatal: early EOF
fatal: index-pack failed

The problem is an unnecessary and harmful layer of quoting. I tried
digging through the history of this function and I think this quoting
was there from the start. My best guess is that it stems from a
misunderstanding what use_shell=1 means. The code seems to assume it
means "arguments get joined into one big string, then fed to /bin/sh".
But that is not what it means: use_shell=1 means that the first
argument in the arguments array may be a shell script and if so should
be passed to /bin/sh. All other arguments are passed as normal
arguments.

The solution is simple: never quote the filter spec.

This commit removes the conditional quoting and adds a test for
partial clone in t5544.
---
 t/t5544-pack-objects-hook.sh | 9 +++++++++
 upload-pack.c                | 9 +--------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh
index 4357af1525..f5ba663d64 100755
--- a/t/t5544-pack-objects-hook.sh
+++ b/t/t5544-pack-objects-hook.sh

@@ -59,4 +59,13 @@ test_expect_success 'hook does not run from repo config' '
 	test_path_is_missing .git/hook.stdout
 '
 
+test_expect_success 'hook works with partial clone' '
+	clear_hook_results &&
+	test_config_global uploadpack.packObjectsHook ./hook &&
+	test_config_global uploadpack.allowFilter true &&
+	git clone --bare --no-local --filter=blob:none . dst.git &&
+	git -C dst.git rev-list --objects --missing=print HEAD >objects &&
+	grep "^?" objects
+'
+
 test_done

diff --git a/upload-pack.c b/upload-pack.c
index 3b66bf92ba..eae1fdbc55 100644
--- a/upload-pack.c
+++ b/upload-pack.c

@@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,
 	if (pack_data->filter_options.choice) {
 		const char *spec =
 			expand_list_objects_filter_spec(&pack_data->filter_options);
-		if (pack_objects.use_shell) {
-			struct strbuf buf = STRBUF_INIT;
-			sq_quote_buf(&buf, spec);
-			strvec_pushf(&pack_objects.args, "--filter=%s", buf.buf);
-			strbuf_release(&buf);
-		} else {
-			strvec_pushf(&pack_objects.args, "--filter=%s", spec);
-		}
+		strvec_pushf(&pack_objects.args, "--filter=%s", spec);
 	}
 	if (uri_protocols) {
 		for (i = 0; i < uri_protocols->nr; i++)

-- 
2.30.0

On Mon, Jan 25, 2021 at 06:09:21PM +0100, Jacob Vosmaer wrote:

+test_expect_success 'hook works with partial clone' '
+	clear_hook_results &&
+	test_config_global uploadpack.packObjectsHook ./hook &&
+	test_config_global uploadpack.allowFilter true &&

I was going to complain that:

 test_config -C dst.git uploadpack.packObjectsHook ./hook &&
 test_config -C dst.git uploadpack.allowFilter true &&

would be more clear, since it tells us which repo we care about
impacting. But the rest of the script doesn't do that, and indeed it
can't, because we don't allow packObjectsHook to be set in per-repo
config for security reasons. :)

(We could do it per-repo for allowFilter, but I think it is just as well
to keep the two calls consistent).

+	git clone --bare --no-local --filter=blob:none . dst.git &&
+	git -C dst.git rev-list --objects --missing=print HEAD >objects &&
+	grep "^?" objects

Previously that clone would fail, so the bug-fix is demonstrated by it
succeeding. But the other two commands are added to make sure that we
actually did apply the filter correctly. Even better. Thanks for being
thorough.

-Peff

Thanks, I changed the reproducing command to use 'env' and I added a
test case in t5544.


Best regards,

Jacob Vosmaer
GitLab, Inc.

On Fri, Jan 22, 2021 at 9:32 PM Jeff King [off-list ref] wrote:

On Fri, Jan 22, 2021 at 03:21:37PM +0100, Jacob Vosmaer wrote:

quoted

This fixes a bug that occurs when you combine partial clone and
uploadpack.packobjectshook. You can reproduce it as follows:

git clone -u 'git -c uploadpack.allowfilter '\
'-c uploadpack.packobjectshook=" exec" '\
'upload-pack' --filter=blob:none --no-local \
src.git dst.git

Be careful with the line endings because this has a long quoted string
as the -u argument. Note that there is an intentional space before
'exec'. Without that space, run-command.c tries to be smart and the
command fails for the wrong reason.

The "-u" command is run with a shell, so:

  git clone \
    -u 'git -c uploadpack.allowfilter \
            -c uploadpack.packobjectshook=env \
            upload-pack' \
  --filter=blob:none --no-local src.git dst.git

may be a more readable version. I also found the use of " exec" clever,
but rather subtle; you need the extra space so that our "don't bother
using a shell" run-command optimization does not kick in. I replaced it
with "env" here, which is a slightly more canonical way of running a
sub-program that does not rely on shell builtins.

But all of this should be added as a new test, probably in t5544 with
the other pack-objects hook tests.

quoted

The problem is an unnecessary and harmful layer of quoting. I tried
digging through the history of this function and I think this quoting
was there from the start. My best guess is that it stems from a
misunderstanding of what use_shell=1 means. The code seems to assume
it means "arguments get joined into one big string, then fed to
/bin/sh". But that is not what it means: use_shell=1 means that the
first argument in the arguments array may be a shell script and if so
should be passed to /bin/sh. All other arguments are passed as normal
arguments.

Yeah, that is exactly right. "use_shell" just means that the command is
(possibly) run with a shell. Quoting for any extra arguments is handled
automatically.

I think you're correct that this was broken from the start in 10ac85c785
(upload-pack: add object filtering for partial clone, 2017-12-08).
That's even before the use_shell was added, and then later it was pushed
into that conditional by 0b6069fe0a (fetch-pack: test support excluding
large blobs, 2017-12-08). Presumably because the non-hook path would not
have worked at all, and that was the first time any of it was actually
tested. ;)

(I've cc'd authors of those commits as an FYI; I think both were
relatively new to the project at the time so misunderstanding this
subtlety of run-command is not too surprising).

I'm somewhat embarrassed to say that despite being the one who added the
pack-objects hook 4 years ago, we still have not switched over to it at
GitHub from our custom patch (the reason is just mundane; there's some
other adjustments that would have to happen and nobody has ever quite
gotten around to it). Presumably you are looking to use it at GitLab.
Just beware that you are probably treading new-ish ground, so there may
be other bugs like this lurking.

quoted

diff --git a/upload-pack.c b/upload-pack.c
index 3b66bf92ba..eae1fdbc55 100644
--- a/upload-pack.c
+++ b/upload-pack.c

@@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,
      if (pack_data->filter_options.choice) {
              const char *spec =
                      expand_list_objects_filter_spec(&pack_data->filter_options);
-             if (pack_objects.use_shell) {
-                     struct strbuf buf = STRBUF_INIT;
-                     sq_quote_buf(&buf, spec);
-                     strvec_pushf(&pack_objects.args, "--filter=%s", buf.buf);
-                     strbuf_release(&buf);
-             } else {
-                     strvec_pushf(&pack_objects.args, "--filter=%s", spec);
-             }
+             strvec_pushf(&pack_objects.args, "--filter=%s", spec);

Yep, this looks like the right fix. I think with an addition to the test
suite, this will be good to go.

-Peff

On Fri, Jan 22, 2021 at 9:32 PM Jeff King [off-list ref] wrote:

I also found the use of " exec" clever,
but rather subtle; you need the extra space so that our "don't bother
using a shell" run-command optimization does not kick in. I replaced it
with "env" here, which is a slightly more canonical way of running a
sub-program that does not rely on shell builtins.

Yes good idea, the exec thing is too clever for its own good.

But all of this should be added as a new test, probably in t5544 with
the other pack-objects hook tests.

Did that, hope it is what you had in mind.

I'm somewhat embarrassed to say that despite being the one who added the
pack-objects hook 4 years ago, we still have not switched over to it at
GitHub from our custom patch (the reason is just mundane; there's some
other adjustments that would have to happen and nobody has ever quite
gotten around to it). Presumably you are looking to use it at GitLab.
Just beware that you are probably treading new-ish ground, so there may
be other bugs like this lurking.

Thanks for the heads up!

Jacob