{"thread":{"id":"57763","subject":"[PATCH] show-branch: fix SEGFAULT when `--current` and `--reflog` together","startedAt":"2022-04-19T16:32:20Z","lastAt":"2022-04-20T10:04:17Z","messageCount":3,"participants":["Gregory David","Ævar Arnfjörð Bjarmason","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"453934","messageId":"a36fd2b0-0573-b93e-a765-ce57a651934e@p1sec.com","threadId":"57763","inReplyTo":null,"subject":"[PATCH] show-branch: fix SEGFAULT when `--current` and `--reflog` together","fromName":"Gregory David","fromEmail":"gregory.david@p1sec.com","sentAt":"2022-04-19T16:32:13Z","receivedAt":"2022-04-19T16:32:20Z","isPatch":true,"sender":{"key":"gregory.david@p1sec.com","avatar":"https://gravatar.com/avatar/4fdf1717095ab69bdb7ecf30c8f0efdf273e91591f65b043562a93b27239a944?d=mp&s=160"},"body":"If run `show-branch` with `--current` and `--reflog` simultaneously, a\nSEGFAULT appears.\n\nThe bug is that we read over the end of the `reflog_msg` array after\nhaving `append_one_rev()` for the current branch without supplying a\nconvenient message to it.\n\nIt seems that it has been introduced in:\nCommit 1aa68d6735 (show-branch: --current includes the current branch.,\n2006-01-11)\n\nSigned-off-by: Gregory DAVID \nThanks-to: Ævar Arnfjörð Bjarmason \n---\n builtin/show-branch.c | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex e12c5e80e3..892241ce0d 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -812,6 +812,26 @@ int cmd_show_branch(int ac, const char **av, const\nchar *prefix)\n \t\t}\n \t\tif (!has_head) {\n \t\t\tconst char *name = head;\n+\t\t\tstruct object_id oid;\n+\t\t\tchar *ref;\n+\t\t\tunsigned int flags = 0;\n+\t\t\tchar *log_msg;\n+\t\t\tchar *end_log_msg;\n+\t\t\ttimestamp_t timestamp;\n+\t\t\tint tz;\n+\n+\t\t\tif (!dwim_ref(*av, strlen(*av), &oid, &ref, 0))\n+\t\t\t\tdie(_(\"no such ref %s\"), *av);\n+\t\t\tif(read_ref_at(get_main_ref_store(the_repository),\n+\t\t\t\t\tref, flags, 0, i, &oid, &log_msg,\n+\t\t\t\t\t×tamp, &tz, NULL)) {\n+\t\t\t\tend_log_msg = strchr(log_msg, '\\n');\n+\t\t\t\tif (end_log_msg)\n+\t\t\t\t\t*end_log_msg = '\\0';\n+\t\t\t}\n+\t\t\tif(log_msg == 0 || *log_msg == '\\0')\n+\t\t\t\tlog_msg = xstrfmt(\"(none)\");\n+\t\t\treflog_msg[ref_name_cnt] = xstrfmt(\"(%s) (current) %s\",\nshow_date(timestamp, tz, DATE_MODE(RELATIVE)), log_msg);\n \t\t\tskip_prefix(name, \"refs/heads/\", &name);\n \t\t\tappend_one_rev(name);\n \t\t}\n-- \n2.35.1\n"},{"id":"453942","messageId":"220419.86ee1s94mp.gmgdl@evledraar.gmail.com","threadId":"57763","inReplyTo":"a36fd2b0-0573-b93e-a765-ce57a651934e@p1sec.com","subject":"Re: [PATCH] show-branch: fix SEGFAULT when `--current` and `--reflog` together","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-19T20:49:19Z","receivedAt":"2022-04-19T20:53:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 19 2022, Gregory David wrote:\n\n> If run `show-branch` with `--current` and `--reflog` simultaneously, a\n> SEGFAULT appears.\n>\n> The bug is that we read over the end of the `reflog_msg` array after\n> having `append_one_rev()` for the current branch without supplying a\n> convenient message to it.\n>\n> It seems that it has been introduced in:\n> Commit 1aa68d6735 (show-branch: --current includes the current branch.,\n> 2006-01-11)\n>\n> Signed-off-by: Gregory DAVID \n> Thanks-to: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/show-branch.c | 20 ++++++++++++++++++++\n> 1 file changed, 20 insertions(+)\n>\n> diff --git a/builtin/show-branch.c b/builtin/show-branch.c\n> index e12c5e80e3..892241ce0d 100644\n> --- a/builtin/show-branch.c\n> +++ b/builtin/show-branch.c\n> @@ -812,6 +812,26 @@ int cmd_show_branch(int ac, const char **av, const\n> char *prefix)\n> \t\t}\n\nHi. Thanks for sticking with this.\n\nFirst, your patch is corrupt, I think because your mailer is wrapping it\nfor you. The Documentation/SubmittingPatches has some advice on how to\nuse \"git format-patch/send-email\" directly.\n\nIt would be great to have a test case for the existing segfault, is it\nsometihng you think you could add to t/t3202-show-branch.sh?\n\n> \t\tif (!has_head) {\n> \t\t\tconst char *name = head;\n> +\t\t\tstruct object_id oid;\n> +\t\t\tchar *ref;\n> +\t\t\tunsigned int flags = 0;\n> +\t\t\tchar *log_msg;\n> +\t\t\tchar *end_log_msg;\n> +\t\t\ttimestamp_t timestamp;\n> +\t\t\tint tz;\n> +\n> +\t\t\tif (!dwim_ref(*av, strlen(*av), &oid, &ref, 0))\n> +\t\t\t\tdie(_(\"no such ref %s\"), *av);\n> +\t\t\tif(read_ref_at(get_main_ref_store(the_repository),\n\nPlease use the usual coding style & one in that file, e.g. \"if (\" not\n\"if(\". \n\n> +\t\t\t\t\tref, flags, 0, i, &oid, &log_msg,\n> +\t\t\t\t\t×tamp, &tz, NULL)) {\n> +\t\t\t\tend_log_msg = strchr(log_msg, '\\n');\n> +\t\t\t\tif (end_log_msg)\n> +\t\t\t\t\t*end_log_msg = '\\0';\n> +\t\t\t}\n\nMost of this code is copied from a few lines above where we do much the\nsame for another case, I wonder if it's worth it to combine the two into\na helper.\n\n> +\t\t\tif(log_msg == 0 || *log_msg == '\\0')\n\nif (!log_msg || !*log_msg) instead, but I see some of this is using a\npattern copied from above...\n\n> +\t\t\t\tlog_msg = xstrfmt(\"(none)\");\n> +\t\t\treflog_msg[ref_name_cnt] = xstrfmt(\"(%s) (current) %s\",\n> show_date(timestamp, tz, DATE_MODE(RELATIVE)), log_msg);\n\n\nThis code already leaks memory, but we can avoid this case easily by\nmaking that \"(none)\" part of the second xstrfmt()'s argument list\n(i.e. as a ternary).\n\n> \t\t\tskip_prefix(name, \"refs/heads/\", &name);\n> \t\t\tappend_one_rev(name);\n> \t\t}\n\n"},{"id":"453966","messageId":"0825f153-1d03-feb4-1e34-4aaed766deb0@gmail.com","threadId":"57763","inReplyTo":"a36fd2b0-0573-b93e-a765-ce57a651934e@p1sec.com","subject":"Re: [PATCH] show-branch: fix SEGFAULT when `--current` and `--reflog` together","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-04-20T10:04:11Z","receivedAt":"2022-04-20T10:04:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Gregory\n\nThanks for working on this\n\nOn 19/04/2022 17:32, Gregory David wrote:\n> If run `show-branch` with `--current` and `--reflog` simultaneously, a\n> SEGFAULT appears.\n> \n> The bug is that we read over the end of the `reflog_msg` array after\n> having `append_one_rev()` for the current branch without supplying a\n> convenient message to it.\n> \n> It seems that it has been introduced in:\n> Commit 1aa68d6735 (show-branch: --current includes the current branch.,\n> 2006-01-11)\n> \n> Signed-off-by: Gregory DAVID \n> Thanks-to: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/show-branch.c | 20 ++++++++++++++++++++\n> 1 file changed, 20 insertions(+)\n> \n> diff --git a/builtin/show-branch.c b/builtin/show-branch.c\n> index e12c5e80e3..892241ce0d 100644\n> --- a/builtin/show-branch.c\n> +++ b/builtin/show-branch.c\n> @@ -812,6 +812,26 @@ int cmd_show_branch(int ac, const char **av, const\n> char *prefix)\n> \t\t}\n> \t\tif (!has_head) {\n> \t\t\tconst char *name = head;\n> +\t\t\tstruct object_id oid;\n> +\t\t\tchar *ref;\n> +\t\t\tunsigned int flags = 0;\n> +\t\t\tchar *log_msg;\n> +\t\t\tchar *end_log_msg;\n> +\t\t\ttimestamp_t timestamp;\n> +\t\t\tint tz;\n> +\n> +\t\t\tif (!dwim_ref(*av, strlen(*av), &oid, &ref, 0))\n> +\t\t\t\tdie(_(\"no such ref %s\"), *av);\n> +\t\t\tif(read_ref_at(get_main_ref_store(the_repository),\n> +\t\t\t\t\tref, flags, 0, i, &oid, &log_msg,\n> +\t\t\t\t\t×tamp, &tz, NULL)) {\n> +\t\t\t\tend_log_msg = strchr(log_msg, '\\n');\n> +\t\t\t\tif (end_log_msg)\n> +\t\t\t\t\t*end_log_msg = '\\0';\n> +\t\t\t}\n> +\t\t\tif(log_msg == 0 || *log_msg == '\\0')\n> +\t\t\t\tlog_msg = xstrfmt(\"(none)\");\n> +\t\t\treflog_msg[ref_name_cnt] = xstrfmt(\"(%s) (current) %s\",\n\nDo we need a bounds check here? I think we're only guaranteed that \nref_name_cnt - 1 < MAX_REVS though I found the existing code a bit hard \nto follow\n\nBest Wishes\n\nPhillip\n\n> show_date(timestamp, tz, DATE_MODE(RELATIVE)), log_msg);\n> \t\t\tskip_prefix(name, \"refs/heads/\", &name);\n> \t\t\tappend_one_rev(name);\n> \t\t}\n\n"}]}