[PATCH] for-each-ref: avoid segfault on --no-format

[PATCH] for-each-ref: avoid segfault on --no-format

[aeron-gh]

The "--format" option is declared with OPT_STRING(), so the
parse-options machinery also accepts "--no-format" and sets the
format string to NULL. The default format is assigned before the
command line is parsed, so nothing puts it back, and the NULL is
dereferenced in verify_ref_format():

    $ git for-each-ref --no-format
    Segmentation fault

"git refs list" shares the code and crashes the same way. The
problem dates back to c3428da87f (Make builtin-for-each-ref.c use
parse-opts., 2007-10-13).

"git branch" and "git tag" have the same option and fall back to
their default format when it is NULL. Do the same here, and assign
the default after parsing the command line when no format is left.
"--no-format" then cancels an earlier "--format" and gives the
default output.

Marking the option with PARSE_OPT_NONEG would also stop the crash,
but then "--no-format" would work for "git branch" and "git tag"
and be an error here.

Signed-off-by: aeron-gh <agab0323@gmail.com>
---
 builtin/for-each-ref.c  | 4 ++--
 t/for-each-ref-tests.sh | 6 ++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 4a2fc421db..4390f3f5e9 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -52,14 +52,14 @@ int for_each_ref_core(int argc, const char **argv, const char *prefix, struct re
 		OPT_END(),
 	};
 
-	format.format = "%(objectname) %(objecttype)\t%(refname)";
-
 	repo_config(repo, git_default_config, NULL);
 
 	/* Set default (refname) sorting */
 	string_list_append(&sorting_options, "refname");
 
 	parse_options(argc, argv, prefix, opts, usage, 0);
+	if (!format.format)
+		format.format = "%(objectname) %(objecttype)\t%(refname)";
 	if (format.array_opts.max_count < 0) {
 		error("invalid --count argument: `%d'", format.array_opts.max_count);
 		usage_with_options(usage, opts);
diff --git a/t/for-each-ref-tests.sh b/t/for-each-ref-tests.sh
index 4991a82b91..3e0108a7db 100644
--- a/t/for-each-ref-tests.sh
+++ b/t/for-each-ref-tests.sh
@@ -285,6 +285,12 @@ test_expect_success 'Check invalid atoms names are errors' '
 	test_must_fail ${git_for_each_ref} --format="%(INVALID)" refs/heads
 '
 
+test_expect_success '--no-format restores the default format' '
+	${git_for_each_ref} refs/heads >expect &&
+	${git_for_each_ref} --format="%(refname)" --no-format refs/heads >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'Check format specifiers are ignored in naming date atoms' '
 	${git_for_each_ref} --format="%(authordate)" refs/heads &&
 	${git_for_each_ref} --format="%(authordate:default) %(authordate)" refs/heads &&
-- 
2.56.0

Re: [PATCH] for-each-ref: avoid segfault on --no-format

[Junio C Hamano]

aeron-gh <agab0323@gmail.com> writes:

> The "--format" option is declared with OPT_STRING(), so the
> parse-options machinery also accepts "--no-format" and sets the
> format string to NULL. The default format is assigned before the
> command line is parsed, so nothing puts it back, and the NULL is
> dereferenced in verify_ref_format():
>
>     $ git for-each-ref --no-format
>     Segmentation fault

That is bad.  PARSE_OPT_NONEG is necessary, I guess?  Another
plausible interpretation may be to exit without showing anything,
but that does not sound like a very useful behaviour.

> Marking the option with PARSE_OPT_NONEG would also stop the crash,
> but then "--no-format" would work for "git branch" and "git tag"
> and be an error here.

Shouldn't "git branch --no-format" also error out?

> Signed-off-by: aeron-gh <agab0323@gmail.com>

Please check [[dco]] and [[real-name]] A few sections in
Documentation/SubmittingPatches.

Thanks.