patchupload-pack.c: fix filter spec quoting bug

> 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 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.

> 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);

> 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).

> 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 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;
> 
> Reads well.  Thanks.
> 
> It is curious why "diff" chose to move stdout_to_stderr line around,
> though.

> 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:
> ...
> 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.
>
>> 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.

> 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.

>  	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++)

>
> 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.
>
> > 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

> 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.

> 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.