Re: [PATCH] archive: error instead of triggering a segfault in `git archive --remote=""`

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 10, 2025, 07:37 UTC
Message-ID: <Z86WtQ-3Rk9OSmn_@pks.im>
In-Reply-To: <pull.1877.git.1741515155475.gitgitgadget@gmail.com>

On Sun, Mar 09, 2025 at 10:12:35AM +0000, emilylime via GitGitGadget wrote:

> From: emilylime <emilyyyylime+git@gmail.com>

This is missing a bit of a description:

    - What is the observed bug?
    - When does the bug trigger?
    - Optional: since when does the bug exist?
    - How do we fix it?
    - Optional: are there alternative ways to fix this bug that you have
      considered but found to be less optimal.

> Signed-off-by: emilylime <emilyyyylime+git@gmail.com>

We usually prefer people to sign off with their full name.

Show 9 quoted lines

> diff --git a/builtin/archive.c b/builtin/archive.c
> index 13ea7308c8b..b6fdbfc7dca 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -97,6 +97,10 @@ int cmd_archive(int argc,
>  	argc = parse_options(argc, argv, prefix, local_opts, NULL,
>  			     PARSE_OPT_KEEP_ALL);
>  
> +	if (remote && !remote[0]) {

Okay, so this triggers in case the user passes "--remote ''"? I see that we ultimately pass the string to `remote_get()`, so does that function segfault? If so, can other callers of that function segfault in a similar way? In that case, we should probably address the issue deeper down in the call stack.

> +		usage(N_("Option 'remote' may not be left empty"));

Error and usage strings should start with a lower-case letter.

> +	}

The curly braces aren't required.

It would also be nice to add a testcase, e.g. in "t/t5000-tar-tree.sh".

Thanks!

Patrick