archive: error instead of triggering a segfault in `git archive --remote=""`
PATCH 1/1 archive: error instead of triggering a segfault in `git archive --remote=""`
From: emilylime <[email protected]>
---
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(+)
builtin/archive.c +4 -0
@@ -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
Signature · 1 line
gitgitgadget
Quoted 1 line · emilylime via GitGitGadget
From: emilylime <[email protected]>
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.
Quoted 1 line
Signed-off-by emilylime <[email protected]>
We usually prefer people to sign off with their full name.
builtin/archive.c int cmd_archive(int argc, · line 100
Quoted 9 lines
builtin/archive.c +1
-0@@ -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.
builtin/archive.c int cmd_archive(int argc, · line 101
Quoted 1 line
builtin/archive.c +1
-0+usage (N_ ("Option 'remote' may not be left empty" ));
Error and usage strings should start with a lower-case letter.
builtin/archive.c int cmd_archive(int argc, · line 102
Quoted 1 line
builtin/archive.c +1
-0+ }
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
Quoted 1 line · emilylime via GitGitGadget
From: emilylime <[email protected]>
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.
Quoted 1 line
Signed-off-by emilylime <[email protected]>
Documentation/SubmittingPatches:[[real-name]]?
Quoted 15 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/1877builtin/archive.c | 4 ++++ 1 file changed, 4 insertions(+)builtin/archive.c +0
-0
builtin/archive.c int cmd_archive(int argc, · line 102
Quoted 6 lines
builtin/archive.c +3
-0@@ -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.
Quoted 7 lines · Patrick Steinhardt
builtin/archive.c int cmd_archive(int argc, · line 100
Quoted 1 line
builtin/archive.c +1
-0+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.