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

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

[emilylime via GitGitGadget]

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

Signed-off-by: emilylime <emilyyyylime+git@gmail.com>
---
    Error instead of triggering a segfault in git archive --remote=""

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1877%2Femilyyyylime%2Ffix-archive-remote-segfault-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1877/emilyyyylime/fix-archive-remote-segfault-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1877

 builtin/archive.c | 4 ++++
 1 file changed, 4 insertions(+)

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]) {
+		usage(N_("Option 'remote' may not be left empty"));
+	}
+
 	init_archivers();
 
 	if (output)

base-commit: f93ff170b93a1782659637824b25923245ac9dd1
-- 
gitgitgadget

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

[Patrick Steinhardt]

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.

> 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

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

A good thing to point out.  If remote_get() segfaults, that is a
grave bug.  If remote_get() returns a NULL for such a non-existent
remote, the code should be able to cope with it, or you found a bug.

In short, I agree with you that this may merely be sweeping a
problem under a rug, not addressing a real problem.

run_remote_archiver() seems to run remote_get() and use the returned
value (which could be NULL, if you named a remote nickname that you
do not even have) without validating when it calls transport_get(),
so that is probably where the problem lies.  If I were writing this
code path, I would probably make run_remote_archiver() take a pointer
to an instance of "struct remote", moving the call to remote_get()
to the caller's side, and deal with an error inside cmd_archive().

Thanks.

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

[Junio C Hamano]

"emilylime via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

Here is a place to explain what the change is about, how to
reproduce and observe the symptom, why the current code behaves the
undesirable way, etc. and then propose how to fix it.

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

Documentation/SubmittingPatches:[[real-name]]?

> ---
>     Error instead of triggering a segfault in git archive --remote=""
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1877%2Femilyyyylime%2Ffix-archive-remote-segfault-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1877/emilyyyylime/fix-archive-remote-segfault-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1877
>
>  builtin/archive.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> 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]) {
> +		usage(N_("Option 'remote' may not be left empty"));
> +	}

Style--useless {braces} around a single-statement block.

Style--downcase "O" in "Option".

N_() merely marks the string for translation, but yields the string
as-is to the calling function (i.e. usage()).  You probably meant to
use _() instead.