{"thread":{"id":"57781","subject":"[PATCH] show-branch: fix SEGFAULT on both --current & --reflog","startedAt":"2022-04-22T13:38:33Z","lastAt":"2022-04-25T07:34:01Z","messageCount":4,"participants":["gdd via GitGitGadget","Junio C Hamano","Gregory David"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"454259","messageId":"pull.1222.git.1650634704191.gitgitgadget@gmail.com","threadId":"57781","inReplyTo":null,"subject":"[PATCH] show-branch: fix SEGFAULT on both --current & --reflog","fromName":"gdd via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-22T13:38:24Z","receivedAt":"2022-04-22T13:38:33Z","isPatch":true,"sender":{"key":"name:gdd","avatar":null},"body":"From: Gregory DAVID \n\nIf 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: Commit\n1aa68d6735 (show-branch: --current includes the current branch.,\n2006-01-11)\n\nSigned-off-by: Gregory DAVID \nThanks-to: Ævar Arnfjörð Bjarmason \n---\n show-branch: fix SEGFAULT on --current + --reflog\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: Commit 1aa68d6735 (show-branch:\n --current includes the current branch., 2006-01-11)\n \n Signed-off-by: Gregory DAVID gregory.david@p1sec.com Thanks-to: Ævar\n Arnfjörð Bjarmason avarab@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1222%2Fp1-gdd%2Ffix%2Fshow-branch-segfault-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1222/p1-gdd/fix/show-branch-segfault-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1222\n\n builtin/show-branch.c | 16 ++++++++++++++++\n t/t3202-show-branch.sh | 43 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 59 insertions(+)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex e12c5e80e3e..723637079d8 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -812,8 +812,24 @@ int cmd_show_branch(int ac, const char **av, const 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 = NULL;\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\tread_ref_at(get_main_ref_store(the_repository), ref, flags, 0, i, &oid, &log_msg, ×tamp, &tz, NULL);\n+\t\t\tend_log_msg = strchr(log_msg, '\\n');\n+\t\t\tif (end_log_msg)\n+\t\t\t\t*end_log_msg = '\\0';\n \t\t\tskip_prefix(name, \"refs/heads/\", &name);\n \t\t\tappend_one_rev(name);\n+\t\t\treflog_msg[ref_name_cnt - 1] = xstrfmt(\"(%s) (current) %s\", show_date(timestamp, tz, DATE_MODE(RELATIVE)), (!log_msg || !*log_msg) ? \"(none)\" : log_msg);\n+\t\t\tfree(log_msg);\n \t\t}\n \t}\n \ndiff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\nindex 7a1be73ce87..7f6ffcf8a57 100755\n--- a/t/t3202-show-branch.sh\n+++ b/t/t3202-show-branch.sh\n@@ -161,4 +161,47 @@ test_expect_success 'show branch --reflog=2' '\n \ttest_cmp actual expect\n '\n \n+test_expect_success 'show branch --reflog=2 --current' '\n+\tsed \"s/^>\t//\" >expect <<-\\EOF &&\n+\t>\t! [refs/heads/branch10@{0}] (4 years, 5 months ago) commit: branch10\n+\t>\t ! [refs/heads/branch10@{1}] (4 years, 5 months ago) commit: branch10\n+\t>\t * [branch10] (4 years, 5 months ago) (current) branch: Created from initial\n+\t>\t---\n+\t>\t+ * [refs/heads/branch10@{0}] branch10\n+\t>\t++* [refs/heads/branch10@{1}] initial\n+\tEOF\n+\tgit show-branch --reflog=2 --current >actual &&\n+\ttest_cmp actual expect\n+'\n+\n+test_expect_success 'show branch --current' '\n+\tsed \"s/^>\t//\" >expect <<-\\EOF &&\n+\t>\t! [branch1] branch1\n+\t>\t ! [branch2] branch2\n+\t>\t ! [branch3] branch3\n+\t>\t ! [branch4] branch4\n+\t>\t ! [branch5] branch5\n+\t>\t ! [branch6] branch6\n+\t>\t ! [branch7] branch7\n+\t>\t ! [branch8] branch8\n+\t>\t ! [branch9] branch9\n+\t>\t * [branch10] branch10\n+\t>\t ! [master] initial\n+\t>\t-----------\n+\t>\t * [branch10] branch10\n+\t>\t + [branch9] branch9\n+\t>\t + [branch8] branch8\n+\t>\t + [branch7] branch7\n+\t>\t + [branch6] branch6\n+\t>\t + [branch5] branch5\n+\t>\t + [branch4] branch4\n+\t>\t + [branch3] branch3\n+\t>\t + [branch2] branch2\n+\t>\t+ [branch1] branch1\n+\t>\t+++++++++*+ [master] initial\n+\tEOF\n+\tgit show-branch --current >actual &&\n+\ttest_cmp actual expect\n+'\n+\n test_done\n\nbase-commit: d516b2db0af2221bd6b13e7347abdcb5830b2829\n-- \ngitgitgadget\n"},{"id":"454267","messageId":"xmqqv8v19ght.fsf@gitster.g","threadId":"57781","inReplyTo":"pull.1222.git.1650634704191.gitgitgadget@gmail.com","subject":"Re: [PATCH] show-branch: fix SEGFAULT on both --current & --reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-22T17:26:22Z","receivedAt":"2022-04-22T18:10:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"gdd via GitGitGadget\" writes:\n\n> From: Gregory DAVID \n>\n> If run `show-branch` with `--current` and `--reflog` simultaneously, a\n> SEGFAULT appears.\n\nThanks for noticing. As I said elsewhere, \"--current\" was invented\nfor the sole purpose of being used together with branches and\nindividual commits, not in \"--reflog\" or \"--merge-base\" modes.\n\nAnd as I also said elsewhere, I do not think of a good reason why a\nuser would want to use these two together.\n\nCan you tell us a bit more about what you were trying to achieve\nwhen you used them together?\n\nWhile waiting for your (and others) valid use cases I probably have\nmissed (I said \"do not think of\" above, not \"I think there cannot\nbe\"), let's speculate what sensible meaning the combination could\nhave.\n\nAs is clear from an existing error when two branches are given:\n\n $ git show-branch --reflog master maint\n fatal: --reflog option needs one branch name\n\nthe \"--reflog\" mode is not even prepared to work with more than one\nbranch. It is to show reflog entries taken from one branch (it\ncould be HEAD)'s reflog.\n\nBut \"--current\" is about \"Among the branches I listed on the command\nline, the current branch may or may not be included. If not, please\npretend as if I had the current branch there, too\".\n\nSo, if we said\n\n $ git show-branch --reflog --current maint\n\nwhile we are on 'master' branch, that is the same as saying\n\n $ git show-branch --reflog master maint\n\nwhich should get a usage error, and if we are on 'maint' branch,\nthen maint is already there, so there is no point in giving\n'--current' to begin with.\n\nBecause\n\n $ git show-branch --reflog\n\ndefaults to showing the reflog entries from current branch,\n\n $ git show-branch --reflog --current \n\nhoping that it would show the reflog entries of the current branch\nis indeed a plausible interpretation, but even in this case, it is\nnot necessary to give \"--current\".\n\nSo, unless there is a reason why it makes sense to enumerate recent\nreflog entries from a branch *and* the tip of the current branch at\nthe same time, I am very much inclined to make it clear that\n\"--reflog\" and \"--current\" are mutually incompatible by making it an\nerror to give both.\n\nIn addition, we _could_ allow a command line with \"--reflog --current\"\nand nothing else on it, and ignore \"--current\" only in that case, to\n\"support\" the \"plausible interpretation\" above, but I do not think\nit is worth it.\n\n> It seems that it has been introduced in: Commit 1aa68d6735\n> (show-branch: --current includes the current branch., 2006-01-11)\n\nYes, the commit should have noticed the invalid combination of\noptions were given and errored out. Since omission of such a check\nlead to a segfaulting bug without producing any useful output, it\nis safe to make it an error to give these options at the same time.\n\nThanks.\n"},{"id":"454275","messageId":"xmqq4k2l9cwo.fsf@gitster.g","threadId":"57781","inReplyTo":"xmqqv8v19ght.fsf@gitster.g","subject":"Re: [PATCH] show-branch: fix SEGFAULT on both --current & --reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-22T18:43:51Z","receivedAt":"2022-04-22T20:14:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> It seems that it has been introduced in: Commit 1aa68d6735\n>> (show-branch: --current includes the current branch., 2006-01-11)\n>\n> Yes, the commit should have noticed the invalid combination of\n> options were given and errored out. Since omission of such a check\n> lead to a segfaulting bug without producing any useful output, it\n> is safe to make it an error to give these options at the same time.\n\nActually, no, the commit couldn't have been the culprit. Back when\n1aa58d6735 was written, \"--reflog\" option did not even exist.\n\nIf we want to find a commit to blame, it is 76a44c5c (show-branch\n--reflog: show the reflog message at the top., 2007-01-19). That\ncommit should have caught invalid combination. It did attempt to\ncatch some invalid combinations (like --more in the reflog mode),\nbut apparently forgot to notice that --current does not make sense\nto be used in the new mode it was adding.\n\nThanks.\n"},{"id":"454368","messageId":"0c9f8841-eb36-05a2-988f-929ab46dc6a5@p1sec.com","threadId":"57781","inReplyTo":"xmqqv8v19ght.fsf@gitster.g","subject":"Re: [PATCH] show-branch: fix SEGFAULT on both --current & --reflog","fromName":"Gregory David","fromEmail":"gregory.david@p1sec.com","sentAt":"2022-04-25T07:33:54Z","receivedAt":"2022-04-25T07:34:01Z","isPatch":true,"sender":{"key":"gregory.david@p1sec.com","avatar":"https://gravatar.com/avatar/4fdf1717095ab69bdb7ecf30c8f0efdf273e91591f65b043562a93b27239a944?d=mp&s=160"},"body":"You are right Junio! They are mutually exclusives and your last\nsuggestion's patch is totally coherent, instead of mine.\n\nMy attempt about this combination was driven by a missunderstanding of\nthe command and the goal to find all ancestors' branches, sorted\nreverse-chronologicaly, for a given commit.\n\nI'm happy to have contributed to the git project by discovering the\nsegfault. Happy, also, that together, we achieve a better fix than I done.\n\nHope this would help all of us, as it will ensure a more stable use in\nour devs.\n\nSincerely.\n\nOn 22/04/2022 17:26, Junio C Hamano wrote:\n> \"gdd via GitGitGadget\" writes:\n> \n>> From: Gregory DAVID \n>>\n>> If run `show-branch` with `--current` and `--reflog` simultaneously, a\n>> SEGFAULT appears.\n> \n> Thanks for noticing. As I said elsewhere, \"--current\" was invented\n> for the sole purpose of being used together with branches and\n> individual commits, not in \"--reflog\" or \"--merge-base\" modes.\n> \n> And as I also said elsewhere, I do not think of a good reason why a\n> user would want to use these two together.\n> \n> Can you tell us a bit more about what you were trying to achieve\n> when you used them together?\n> \n> While waiting for your (and others) valid use cases I probably have\n> missed (I said \"do not think of\" above, not \"I think there cannot\n> be\"), let's speculate what sensible meaning the combination could\n> have.\n> \n> As is clear from an existing error when two branches are given:\n> \n> $ git show-branch --reflog master maint\n> fatal: --reflog option needs one branch name\n> \n> the \"--reflog\" mode is not even prepared to work with more than one\n> branch. It is to show reflog entries taken from one branch (it\n> could be HEAD)'s reflog.\n> \n> But \"--current\" is about \"Among the branches I listed on the command\n> line, the current branch may or may not be included. If not, please\n> pretend as if I had the current branch there, too\".\n> \n> So, if we said\n> \n> $ git show-branch --reflog --current maint\n> \n> while we are on 'master' branch, that is the same as saying\n> \n> $ git show-branch --reflog master maint\n> \n> which should get a usage error, and if we are on 'maint' branch,\n> then maint is already there, so there is no point in giving\n> '--current' to begin with.\n> \n> Because\n> \n> $ git show-branch --reflog\n> \n> defaults to showing the reflog entries from current branch,\n> \n> $ git show-branch --reflog --current \n> \n> hoping that it would show the reflog entries of the current branch\n> is indeed a plausible interpretation, but even in this case, it is\n> not necessary to give \"--current\".\n> \n> So, unless there is a reason why it makes sense to enumerate recent\n> reflog entries from a branch *and* the tip of the current branch at\n> the same time, I am very much inclined to make it clear that\n> \"--reflog\" and \"--current\" are mutually incompatible by making it an\n> error to give both.\n> \n> In addition, we _could_ allow a command line with \"--reflog --current\"\n> and nothing else on it, and ignore \"--current\" only in that case, to\n> \"support\" the \"plausible interpretation\" above, but I do not think\n> it is worth it.\n> \n>> It seems that it has been introduced in: Commit 1aa68d6735\n>> (show-branch: --current includes the current branch., 2006-01-11)\n> \n> Yes, the commit should have noticed the invalid combination of\n> options were given and errored out. Since omission of such a check\n> lead to a segfaulting bug without producing any useful output, it\n> is safe to make it an error to give these options at the same time.\n> \n> Thanks.\n\n-- \nGregory David\nSecurity Engineer\nhttps://www.p1sec.com"}]}