threads / patch / 63080

patcharchive: error instead of triggering a segfault in `git archive --remote=""`

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

## tl;dr

4 messages between Mar 9, 2025 and Mar 10, 2025. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

emilylime via GitGitGadget· Mar 9, 2025, 10:12 UTC · lore
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(+)
Show changes to builtin/archive.c +4 −0
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
Patrick Steinhardt· Mar 10, 2025, 07:37 UTC · re: emilylime via GitGitGadget · lore

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

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
Junio C Hamano· Mar 10, 2025, 15:59 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:
Show 7 quoted lines
>> +	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.
Junio C Hamano· Mar 10, 2025, 11:25 UTC · re: emilylime via GitGitGadget · lore

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

"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]]?
Show 21 quoted lines
> ---
>     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.

← back to recent threads