| Message ID | pull.1877.git.1741515155475.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | archive: error instead of triggering a segfault in `git archive --remote=""` | expand |
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
"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.
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.
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)