{"thread":{"id":"63080","subject":"[PATCH] archive: error instead of triggering a segfault in `git archive --remote=\"\"`","startedAt":"2025-03-09T10:12:39Z","lastAt":"2025-03-10T15:59:58Z","messageCount":4,"participants":["emilylime via GitGitGadget","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"513847","messageId":"pull.1877.git.1741515155475.gitgitgadget@gmail.com","threadId":"63080","inReplyTo":null,"subject":"[PATCH] archive: error instead of triggering a segfault in `git archive --remote=\"\"`","fromName":"emilylime via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-09T10:12:35Z","receivedAt":"2025-03-09T10:12:39Z","isPatch":true,"sender":{"key":"name:emilylime","avatar":null},"body":"From: emilylime \n\nSigned-off-by: emilylime \n---\n Error instead of triggering a segfault in git archive --remote=\"\"\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1877%2Femilyyyylime%2Ffix-archive-remote-segfault-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1877/emilyyyylime/fix-archive-remote-segfault-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1877\n\n builtin/archive.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex 13ea7308c8b..b6fdbfc7dca 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -97,6 +97,10 @@ int cmd_archive(int argc,\n \targc = parse_options(argc, argv, prefix, local_opts, NULL,\n \t\t\t PARSE_OPT_KEEP_ALL);\n \n+\tif (remote && !remote[0]) {\n+\t\tusage(N_(\"Option 'remote' may not be left empty\"));\n+\t}\n+\n \tinit_archivers();\n \n \tif (output)\n\nbase-commit: f93ff170b93a1782659637824b25923245ac9dd1\n-- \ngitgitgadget\n"},{"id":"513885","messageId":"Z86WtQ-3Rk9OSmn_@pks.im","threadId":"63080","inReplyTo":"pull.1877.git.1741515155475.gitgitgadget@gmail.com","subject":"Re: [PATCH] archive: error instead of triggering a segfault in `git archive --remote=\"\"`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-10T07:37:25Z","receivedAt":"2025-03-10T07:37:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Mar 09, 2025 at 10:12:35AM +0000, emilylime via GitGitGadget wrote:\n> From: emilylime \n\nThis is missing a bit of a description:\n\n - What is the observed bug?\n - When does the bug trigger?\n - Optional: since when does the bug exist?\n - How do we fix it?\n - Optional: are there alternative ways to fix this bug that you have\n considered but found to be less optimal.\n\n> Signed-off-by: emilylime \n\nWe usually prefer people to sign off with their full name.\n\n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index 13ea7308c8b..b6fdbfc7dca 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -97,6 +97,10 @@ int cmd_archive(int argc,\n> \targc = parse_options(argc, argv, prefix, local_opts, NULL,\n> \t\t\t PARSE_OPT_KEEP_ALL);\n> \n> +\tif (remote && !remote[0]) {\n\nOkay, so this triggers in case the user passes \"--remote ''\"?\nI see that we ultimately pass the string to `remote_get()`, so does that\nfunction segfault? If so, can other callers of that function segfault in\na similar way? In that case, we should probably address the issue deeper\ndown in the call stack.\n\n> +\t\tusage(N_(\"Option 'remote' may not be left empty\"));\n\nError and usage strings should start with a lower-case letter.\n\n> +\t}\n\nThe curly braces aren't required.\n\nIt would also be nice to add a testcase, e.g. in \"t/t5000-tar-tree.sh\".\n\nThanks!\n\nPatrick\n"},{"id":"513892","messageId":"xmqq34flb00k.fsf@gitster.g","threadId":"63080","inReplyTo":"pull.1877.git.1741515155475.gitgitgadget@gmail.com","subject":"Re: [PATCH] archive: error instead of triggering a segfault in `git archive --remote=\"\"`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-10T11:25:31Z","receivedAt":"2025-03-10T11:25:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"emilylime via GitGitGadget\" writes:\n\n> From: emilylime \n\nHere is a place to explain what the change is about, how to\nreproduce and observe the symptom, why the current code behaves the\nundesirable way, etc. and then propose how to fix it.\n\n> Signed-off-by: emilylime \n\nDocumentation/SubmittingPatches:[[real-name]]?\n\n> ---\n> Error instead of triggering a segfault in git archive --remote=\"\"\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1877%2Femilyyyylime%2Ffix-archive-remote-segfault-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1877/emilyyyylime/fix-archive-remote-segfault-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1877\n>\n> builtin/archive.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index 13ea7308c8b..b6fdbfc7dca 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -97,6 +97,10 @@ int cmd_archive(int argc,\n> \targc = parse_options(argc, argv, prefix, local_opts, NULL,\n> \t\t\t PARSE_OPT_KEEP_ALL);\n> \n> +\tif (remote && !remote[0]) {\n> +\t\tusage(N_(\"Option 'remote' may not be left empty\"));\n> +\t}\n\nStyle--useless {braces} around a single-statement block.\n\nStyle--downcase \"O\" in \"Option\".\n\nN_() merely marks the string for translation, but yields the string\nas-is to the calling function (i.e. usage()). You probably meant to\nuse _() instead. \n\n"},{"id":"513916","messageId":"xmqq7c4wanb8.fsf@gitster.g","threadId":"63080","inReplyTo":"Z86WtQ-3Rk9OSmn_@pks.im","subject":"Re: [PATCH] archive: error instead of triggering a segfault in `git archive --remote=\"\"`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-10T15:59:55Z","receivedAt":"2025-03-10T15:59:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> +\tif (remote && !remote[0]) {\n>\n> Okay, so this triggers in case the user passes \"--remote ''\"?\n> I see that we ultimately pass the string to `remote_get()`, so does that\n> function segfault? If so, can other callers of that function segfault in\n> a similar way? In that case, we should probably address the issue deeper\n> down in the call stack.\n\nA good thing to point out. If remote_get() segfaults, that is a\ngrave bug. If remote_get() returns a NULL for such a non-existent\nremote, the code should be able to cope with it, or you found a bug.\n\nIn short, I agree with you that this may merely be sweeping a\nproblem under a rug, not addressing a real problem.\n\nrun_remote_archiver() seems to run remote_get() and use the returned\nvalue (which could be NULL, if you named a remote nickname that you\ndo not even have) without validating when it calls transport_get(),\nso that is probably where the problem lies. If I were writing this\ncode path, I would probably make run_remote_archiver() take a pointer\nto an instance of \"struct remote\", moving the call to remote_get()\nto the caller's side, and deal with an error inside cmd_archive().\n\nThanks.\n\n\n"}]}