{"thread":{"id":"60948","subject":"[PATCH] rerere: fix crash in during clear","startedAt":"2024-02-18T12:05:12Z","lastAt":"2024-04-16T10:54:15Z","messageCount":15,"participants":["Marcel Röthke","Kristoffer Haugsbakk","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"488872","messageId":"20240218114936.1121077-1-marcel@roethke.info","threadId":"60948","inReplyTo":null,"subject":"[PATCH] rerere: fix crash in during clear","fromName":"Marcel Röthke","fromEmail":"marcel@roethke.info","sentAt":"2024-02-18T11:49:36Z","receivedAt":"2024-02-18T12:05:12Z","isPatch":true,"sender":{"key":"marcel@roethke.info","avatar":"https://avatars.githubusercontent.com/u/21661035?v=4"},"body":"When rerere_clear is called, for instance when aborting a rebase, and\nthe current conflict does not have a pre or postimage recorded git\ncrashes with a SEGFAULT in has_rerere_resolution when accessing the\nstatus member of struct rerere_dir. This happens because scan_rerere_dir\nonly allocates the status field in struct rerere_dir when a post or\npreimage was found. In some cases a segfault may happen even if a post\nor preimage was recorded if it was not for the variant of interest and\nthe number of the variant that is present is lower than the variant of\ninterest.\n\nThis patch solves this by making sure the status field is large enough\nto accommodate for the variant of interest so it can be accesses without\nchecking if it is large enough.\n\nAn alternative solution would be to always check before accessing the\nstatus field, but I think the chosen solution aligns better with the\nassumptions made elsewhere in the code.\n\nSigned-off-by: Marcel Röthke \n---\n rerere.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/rerere.c b/rerere.c\nindex ca7e77ba68..3973ccce37 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -219,6 +219,9 @@ static void read_rr(struct repository *r, struct string_list *rr)\n \t\tbuf.buf[hexsz] = '\\0';\n \t\tid = new_rerere_id_hex(buf.buf);\n \t\tid->variant = variant;\n+\t\t/* make sure id->collection->status has enough space\n+\t\t * for the variant we are interested in */\n+\t\tfit_variant(id->collection, variant);\n \t\tstring_list_insert(rr, path)->util = id;\n \t}\n \tstrbuf_release(&buf);\n-- \n2.43.2\n\n"},{"id":"488874","messageId":"2359c888-a061-44ed-9d69-2aea9e1d3d80@app.fastmail.com","threadId":"60948","inReplyTo":"20240218114936.1121077-1-marcel@roethke.info","subject":"Re: [PATCH] rerere: fix crash in during clear","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-18T13:02:42Z","receivedAt":"2024-02-18T13:03:05Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Hi\n\n> rerere: fix crash in during clear\n\n“in during clear”? Did you mean “during clear”?\n\nOn Sun, Feb 18, 2024, at 12:49, Marcel Röthke wrote:\n> When rerere_clear is called, for instance when aborting a rebase, and\n> the current conflict does not have a pre or postimage recorded git\n> crashes with a SEGFAULT in has_rerere_resolution when accessing the\n> status member of struct rerere_dir. This happens because scan_rerere_dir\n> only allocates the status field in struct rerere_dir when a post or\n> preimage was found. In some cases a segfault may happen even if a post\n> or preimage was recorded if it was not for the variant of interest and\n> the number of the variant that is present is lower than the variant of\n> interest.\n>\n> This patch solves this by making sure the status field is large enough\n\nYou can simplify “This patch solves this” to “Solve this”; see\n`SubmittingPatches` under “imperative-mood”.\n\n> to accommodate for the variant of interest so it can be accesses without\n> checking if it is large enough.\n\n“accessed”\n\n>\n> An alternative solution would be to always check before accessing the\n> status field, but I think the chosen solution aligns better with the\n> assumptions made elsewhere in the code.\n>\n> Signed-off-by: Marcel Röthke \n> ---\n> rerere.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/rerere.c b/rerere.c\n> index ca7e77ba68..3973ccce37 100644\n> --- a/rerere.c\n> +++ b/rerere.c\n> @@ -219,6 +219,9 @@ static void read_rr(struct repository *r, struct\n> string_list *rr)\n> \t\tbuf.buf[hexsz] = '\\0';\n> \t\tid = new_rerere_id_hex(buf.buf);\n> \t\tid->variant = variant;\n> +\t\t/* make sure id->collection->status has enough space\n> +\t\t * for the variant we are interested in */\n\nMulti-line comments should have the delimiters on separate lines from\nthe text. See `CodingGuidelines` under “Multi-line comments”.\n\n> +\t\tfit_variant(id->collection, variant);\n> \t\tstring_list_insert(rr, path)->util = id;\n> \t}\n> \tstrbuf_release(&buf);\n> --\n> 2.43.2\n"},{"id":"488881","messageId":"ZdJcoYaZEnUNoQSQ@roethke.info","threadId":"60948","inReplyTo":"2359c888-a061-44ed-9d69-2aea9e1d3d80@app.fastmail.com","subject":"Re: [PATCH] rerere: fix crash in during clear","fromName":"Marcel Röthke","fromEmail":"marcel@roethke.info","sentAt":"2024-02-18T19:38:09Z","receivedAt":"2024-02-18T19:38:13Z","isPatch":true,"sender":{"key":"marcel@roethke.info","avatar":"https://avatars.githubusercontent.com/u/21661035?v=4"},"body":"On 2024-02-18 14:02:42, Kristoffer Haugsbakk wrote:\n> > rerere: fix crash in during clear\n>\n> “in during clear”? Did you mean “during clear”?\n\nYes, thanks.\n\n> On Sun, Feb 18, 2024, at 12:49, Marcel Röthke wrote:\n> > When rerere_clear is called, for instance when aborting a rebase, and\n> > the current conflict does not have a pre or postimage recorded git\n> > crashes with a SEGFAULT in has_rerere_resolution when accessing the\n> > status member of struct rerere_dir. This happens because scan_rerere_dir\n> > only allocates the status field in struct rerere_dir when a post or\n> > preimage was found. In some cases a segfault may happen even if a post\n> > or preimage was recorded if it was not for the variant of interest and\n> > the number of the variant that is present is lower than the variant of\n> > interest.\n> >\n> > This patch solves this by making sure the status field is large enough\n>\n> You can simplify “This patch solves this” to “Solve this”; see\n> `SubmittingPatches` under “imperative-mood”.\n\ndone\n\n>\n> > to accommodate for the variant of interest so it can be accesses without\n> > checking if it is large enough.\n>\n> “accessed”\n>\n\ndone\n\n> >\n> > An alternative solution would be to always check before accessing the\n> > status field, but I think the chosen solution aligns better with the\n> > assumptions made elsewhere in the code.\n> >\n> > Signed-off-by: Marcel Röthke \n> > ---\n> > rerere.c | 3 +++\n> > 1 file changed, 3 insertions(+)\n> >\n> > diff --git a/rerere.c b/rerere.c\n> > index ca7e77ba68..3973ccce37 100644\n> > --- a/rerere.c\n> > +++ b/rerere.c\n> > @@ -219,6 +219,9 @@ static void read_rr(struct repository *r, struct\n> > string_list *rr)\n> > \t\tbuf.buf[hexsz] = '\\0';\n> > \t\tid = new_rerere_id_hex(buf.buf);\n> > \t\tid->variant = variant;\n> > +\t\t/* make sure id->collection->status has enough space\n> > +\t\t * for the variant we are interested in */\n>\n> Multi-line comments should have the delimiters on separate lines from\n> the text. See `CodingGuidelines` under “Multi-line comments”.\n\ndone\n\n\nThank you for your feedback!\n"},{"id":"488882","messageId":"20240218194603.1210895-1-marcel@roethke.info","threadId":"60948","inReplyTo":"20240218114936.1121077-1-marcel@roethke.info","subject":"[PATCH v2] rerere: fix crash during clear","fromName":"Marcel Röthke","fromEmail":"marcel@roethke.info","sentAt":"2024-02-18T19:46:03Z","receivedAt":"2024-02-18T19:46:13Z","isPatch":true,"sender":{"key":"marcel@roethke.info","avatar":"https://avatars.githubusercontent.com/u/21661035?v=4"},"body":"When rerere_clear is called, for instance when aborting a rebase, and\nthe current conflict does not have a pre or postimage recorded git\ncrashes with a SEGFAULT in has_rerere_resolution when accessing the\nstatus member of struct rerere_dir. This happens because scan_rerere_dir\nonly allocates the status field in struct rerere_dir when a post or\npreimage was found. In some cases a segfault may happen even if a post\nor preimage was recorded if it was not for the variant of interest and\nthe number of the variant that is present is lower than the variant of\ninterest.\n\nSolve this by making sure the status field is large enough to\naccommodate for the variant of interest so it can be accessed without\nchecking if it is large enough.\n\nAn alternative solution would be to always check before accessing the\nstatus field, but I think the chosen solution aligns better with the\nassumptions made elsewhere in the code.\n\nSigned-off-by: Marcel Röthke \n---\nRange-diff against v1:\n1: 93f982d170 ! 1: 68178298fe rerere: fix crash in during clear\n @@ Metadata\n Author: Marcel Röthke \n \n ## Commit message ##\n - rerere: fix crash in during clear\n + rerere: fix crash during clear\n \n When rerere_clear is called, for instance when aborting a rebase, and\n the current conflict does not have a pre or postimage recorded git\n @@ Commit message\n the number of the variant that is present is lower than the variant of\n interest.\n \n - This patch solves this by making sure the status field is large enough\n - to accommodate for the variant of interest so it can be accesses without\n + Solve this by making sure the status field is large enough to\n + accommodate for the variant of interest so it can be accessed without\n checking if it is large enough.\n \n An alternative solution would be to always check before accessing the\n @@ rerere.c: static void read_rr(struct repository *r, struct string_list *rr)\n \t\tbuf.buf[hexsz] = '\\0';\n \t\tid = new_rerere_id_hex(buf.buf);\n \t\tid->variant = variant;\n -+\t\t/* make sure id->collection->status has enough space\n -+\t\t * for the variant we are interested in */\n ++\t\t/*\n ++\t\t * make sure id->collection->status has enough space\n ++\t\t * for the variant we are interested in\n ++\t\t */\n +\t\tfit_variant(id->collection, variant);\n \t\tstring_list_insert(rr, path)->util = id;\n \t}\n\n rerere.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/rerere.c b/rerere.c\nindex ca7e77ba68..4683d6cbb1 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -219,6 +219,11 @@ static void read_rr(struct repository *r, struct string_list *rr)\n \t\tbuf.buf[hexsz] = '\\0';\n \t\tid = new_rerere_id_hex(buf.buf);\n \t\tid->variant = variant;\n+\t\t/*\n+\t\t * make sure id->collection->status has enough space\n+\t\t * for the variant we are interested in\n+\t\t */\n+\t\tfit_variant(id->collection, variant);\n \t\tstring_list_insert(rr, path)->util = id;\n \t}\n \tstrbuf_release(&buf);\n-- \n2.43.2\n\n"},{"id":"488946","messageId":"xmqqplwsx730.fsf@gitster.g","threadId":"60948","inReplyTo":"20240218194603.1210895-1-marcel@roethke.info","subject":"Re: [PATCH v2] rerere: fix crash during clear","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-20T01:22:43Z","receivedAt":"2024-02-20T01:22:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marcel Röthke writes:\n\n> When rerere_clear is called, for instance when aborting a rebase, and\n> the current conflict does not have a pre or postimage recorded git\n> crashes with a SEGFAULT in has_rerere_resolution when accessing the\n> status member of struct rerere_dir.\n\nI had to read this twice before realizing the reason why I found it\nhard to grok was because of a missing comma between \"recorded\" and\n\"git\".\n\n> This happens because scan_rerere_dir\n> only allocates the status field in struct rerere_dir when a post or\n> preimage was found.\n\nBut that is not really the root cause, no? Readers following the\nabove text are probably wondering why the preimage was not recorded,\nwhen a conflict resulted in stopping a mergy-command and invoking\nrerere machinery, before rerere_clear() got called. Is that\nsomething that usually happen? How? Do we have a reproduction\nsequence of such a state that we can make it into a new test in\nt4200 where we already have tests for \"git rerere clear\" and its\nfriends?\n\n> In some cases a segfault may happen even if a post\n> or preimage was recorded if it was not for the variant of interest and\n> the number of the variant that is present is lower than the variant of\n> interest.\n\nDitto. What sequence of events would lead to such a state?\n\nThe answer *can* be \".git/rr-cache being a normal directory, the\nuser can poke around, removing files randomly, which can create such\na problematic situation\", and the reproduction test *can* also be to\nsimulate such an end-user action, but I am asking primarily because\nI want to make sure that we are *not* losing or failing to create\nnecessary preimage files, causing this problem to users who do not\nmuck with files in .git/rr-cache themselves.\n\nThanks.\n"},{"id":"489275","messageId":"ZdnSEFgLdev3L2qs@roethke.info","threadId":"60948","inReplyTo":"xmqqplwsx730.fsf@gitster.g","subject":"Re: [PATCH v2] rerere: fix crash during clear","fromName":"Marcel Röthke","fromEmail":"marcel@roethke.info","sentAt":"2024-02-24T11:25:04Z","receivedAt":"2024-02-24T11:25:15Z","isPatch":true,"sender":{"key":"marcel@roethke.info","avatar":"https://avatars.githubusercontent.com/u/21661035?v=4"},"body":"On 2024-02-19 17:22:43, Junio C Hamano wrote:\n> Marcel Röthke writes:\n>\n> > When rerere_clear is called, for instance when aborting a rebase, and\n> > the current conflict does not have a pre or postimage recorded git\n> > crashes with a SEGFAULT in has_rerere_resolution when accessing the\n> > status member of struct rerere_dir.\n>\n> I had to read this twice before realizing the reason why I found it\n> hard to grok was because of a missing comma between \"recorded\" and\n> \"git\".\n\nfixed\n\n> > This happens because scan_rerere_dir\n> > only allocates the status field in struct rerere_dir when a post or\n> > preimage was found.\n>\n> But that is not really the root cause, no? Readers following the\n> above text are probably wondering why the preimage was not recorded,\n> when a conflict resulted in stopping a mergy-command and invoking\n> rerere machinery, before rerere_clear() got called. Is that\n> something that usually happen? How? Do we have a reproduction\n> sequence of such a state that we can make it into a new test in\n> t4200 where we already have tests for \"git rerere clear\" and its\n> friends?\n\nI'm unfortunately not sure how it happened. I do have the initial state\nof the repository and I think I know the commands that were executed,\nbut I could not reproduce it.\n\nI will look into adding a test case though.\n"},{"id":"491379","messageId":"xmqqplvjpacq.fsf@gitster.g","threadId":"60948","inReplyTo":"ZdnSEFgLdev3L2qs@roethke.info","subject":"Re: [PATCH v2] rerere: fix crash during clear","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-24T21:51:17Z","receivedAt":"2024-03-24T21:51:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marcel Röthke writes:\n\n> On 2024-02-19 17:22:43, Junio C Hamano wrote:\n>> Marcel Röthke writes:\n>>\n>> > When rerere_clear is called, for instance when aborting a rebase, and\n>> > the current conflict does not have a pre or postimage recorded git\n>> > crashes with a SEGFAULT in has_rerere_resolution when accessing the\n>> > status member of struct rerere_dir.\n>>\n>> I had to read this twice before realizing the reason why I found it\n>> hard to grok was because of a missing comma between \"recorded\" and\n>> \"git\".\n>\n> fixed\n> ...\n> I'm unfortunately not sure how it happened. I do have the initial state\n> of the repository and I think I know the commands that were executed,\n> but I could not reproduce it.\n>\n> I will look into adding a test case though.\n\nIt has been about a month. Any new development on this topic?\n\nThanks. \n"},{"id":"491495","messageId":"ZgHQ8YsJxUtRpB8M@roethke.info","threadId":"60948","inReplyTo":"xmqqplvjpacq.fsf@gitster.g","subject":"Re: [PATCH v2] rerere: fix crash during clear","fromName":"Marcel Röthke","fromEmail":"marcel@roethke.info","sentAt":"2024-03-25T19:30:57Z","receivedAt":"2024-03-25T19:37:59Z","isPatch":true,"sender":{"key":"marcel@roethke.info","avatar":"https://avatars.githubusercontent.com/u/21661035?v=4"},"body":"On 2024-03-24 14:51:17, Junio C Hamano wrote:\n> Marcel Röthke writes:\n>\n> > On 2024-02-19 17:22:43, Junio C Hamano wrote:\n> >> Marcel Röthke writes:\n> >>\n> >> > When rerere_clear is called, for instance when aborting a rebase, and\n> >> > the current conflict does not have a pre or postimage recorded git\n> >> > crashes with a SEGFAULT in has_rerere_resolution when accessing the\n> >> > status member of struct rerere_dir.\n> >>\n> >> I had to read this twice before realizing the reason why I found it\n> >> hard to grok was because of a missing comma between \"recorded\" and\n> >> \"git\".\n> >\n> > fixed\n> > ...\n> > I'm unfortunately not sure how it happened. I do have the initial state\n> > of the repository and I think I know the commands that were executed,\n> > but I could not reproduce it.\n> >\n> > I will look into adding a test case though.\n>\n> It has been about a month. Any new development on this topic?\n>\n> Thanks.\n\nNo, I have been otherwise occupied. But I plan to get back to this soon. Probably this week, next week at the latest.\n"},{"id":"492474","messageId":"ZhL-Jt_Nb5egpKfx@roethke.info","threadId":"60948","inReplyTo":"xmqqplvjpacq.fsf@gitster.g","subject":"Re: [PATCH v2] rerere: fix crash during clear","fromName":"Marcel Röthke","fromEmail":"marcel@roethke.info","sentAt":"2024-04-07T20:12:22Z","receivedAt":"2024-04-07T20:12:25Z","isPatch":true,"sender":{"key":"marcel@roethke.info","avatar":"https://avatars.githubusercontent.com/u/21661035?v=4"},"body":"After reproducing this without manually corrupting the database, I\nnoticed that the summary line is no longer accurate. Because it fixes\ntwo SEGFAULTs and one of them does not happen during clear. Can I change this\nin v3 or should I start a new thread?\n"},{"id":"492540","messageId":"xmqqle5ng9xk.fsf@gitster.g","threadId":"60948","inReplyTo":"ZhL-Jt_Nb5egpKfx@roethke.info","subject":"Re: [PATCH v2] rerere: fix crash during clear","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-08T15:18:15Z","receivedAt":"2024-04-08T15:18:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marcel Röthke writes:\n\n> After reproducing this without manually corrupting the database, I\n> noticed that the summary line is no longer accurate. Because it fixes\n> two SEGFAULTs and one of them does not happen during clear. Can I change this\n> in v3 or should I start a new thread?\n\nUpdating the title in the course of evolution of a topic/patch is\nperfectly normal. Please do so in v3 as continuation of the\nprevious round.\n\nThanks.\n"},{"id":"492631","messageId":"20240409121708.131542-2-marcel@roethke.info","threadId":"60948","inReplyTo":"20240218194603.1210895-1-marcel@roethke.info","subject":"[PATCH v3] rerere: fix crashes due to unmatched opening conflict markers","fromName":"Marcel Röthke","fromEmail":"marcel@roethke.info","sentAt":"2024-04-09T12:13:51Z","receivedAt":"2024-04-09T12:20:43Z","isPatch":true,"sender":{"key":"marcel@roethke.info","avatar":"https://avatars.githubusercontent.com/u/21661035?v=4"},"body":"When rerere handles a conflict with an unmatched opening conflict marker\nin a file with other conflicts, it will fail create a preimage and also\nfail allocate the status member of struct rerere_dir. Currently the\nstatus member is allocated after the error handling. This will lead to a\nSEGFAULT when the status member is accessed during cleanup of the failed\nparse.\n\nAdditionally, in subsequent executions of rerere, after removing the\nMERGE_RR.lock manually, rerere crashes for a similar reason. MERGE_RR\npoints to a conflict id that has no preimage, therefore the status\nmember is not allocated and a SEGFAULT happens when trying to check if a\npreimage exists.\n\nSolve this by making sure the status field is allocated correctly and add\ntests to prevent the bug from reoccurring.\n\nThis does not fix the root cause, failing to parse stray conflict\nmarkers, but I don't think we can do much better than recognizing it,\nprinting an error, and moving on gracefully.\n\nSigned-off-by: Marcel Röthke \n---\nInterdiff against v2:\n diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\n index fb53dddf79..1e80f76860 100755\n --- a/t/t4200-rerere.sh\n +++ b/t/t4200-rerere.sh\n @@ -671,4 +671,67 @@ test_expect_success 'test simple stage 1 handling' '\n \t)\n '\n\n +test_expect_success 'rerere does not crash with missing preimage' '\n +\tgit config rerere.enabled true &&\n +\n +\techo bar >test &&\n +\tgit add test &&\n +\tgit commit -m \"one\" &&\n +\tgit branch rerere_no_crash &&\n +\n +\techo foo >>test &&\n +\tgit add test &&\n +\tgit commit -m \"two\" &&\n +\n +\tgit checkout rerere_no_crash &&\n +\techo \"bar\" >>test &&\n +\tgit add test &&\n +\tgit commit -m \"three\" &&\n +\n +\ttest_must_fail git rebase main &&\n +\trm .git/rr-cache/*/preimage &&\n +\tgit rebase --abort\n +'\n +\n +test_expect_success 'rerere does not crash with unmatched conflict marker' '\n +\tgit config rerere.enabled true &&\n +\n +\techo bar >test &&\n +\tgit add test &&\n +\tgit commit -m \"one\" &&\n +\tgit branch rerere_no_preimage &&\n +\n +\tcat >test <<-EOF &&\n +\ttest\n +\tbar\n +\tfoobar\n +\tEOF\n +\tgit add test &&\n +\tgit commit -m \"two\" &&\n +\n +\tgit checkout rerere_no_preimage &&\n +\techo \"bar\" >>test &&\n +\tgit add test &&\n +\tgit commit -m \"three\" &&\n +\n +\tcat >test <<-EOF &&\n +\tfoobar\n +\tbar\n +\tbar\n +\tEOF\n +\tgit add test &&\n +\tgit commit -m \"four\" &&\n +\n +\ttest_must_fail git rebase main &&\n +\tcat >test <<-EOF &&\n +\ttest\n +\tbar\n +\t<<<<<<< HEAD\n +\tfoobar\n +\tbar\n +\tEOF\n +\tgit add test &&\n +\tgit rebase --continue\n +'\n +\n test_done\n\n rerere.c | 5 ++++\n t/t4200-rerere.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 68 insertions(+)\n\ndiff --git a/rerere.c b/rerere.c\nindex ca7e77ba68..4683d6cbb1 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -219,6 +219,11 @@ static void read_rr(struct repository *r, struct string_list *rr)\n \t\tbuf.buf[hexsz] = '\\0';\n \t\tid = new_rerere_id_hex(buf.buf);\n \t\tid->variant = variant;\n+\t\t/*\n+\t\t * make sure id->collection->status has enough space\n+\t\t * for the variant we are interested in\n+\t\t */\n+\t\tfit_variant(id->collection, variant);\n \t\tstring_list_insert(rr, path)->util = id;\n \t}\n \tstrbuf_release(&buf);\ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex fb53dddf79..1e80f76860 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -671,4 +671,67 @@ test_expect_success 'test simple stage 1 handling' '\n \t)\n '\n\n+test_expect_success 'rerere does not crash with missing preimage' '\n+\tgit config rerere.enabled true &&\n+\n+\techo bar >test &&\n+\tgit add test &&\n+\tgit commit -m \"one\" &&\n+\tgit branch rerere_no_crash &&\n+\n+\techo foo >>test &&\n+\tgit add test &&\n+\tgit commit -m \"two\" &&\n+\n+\tgit checkout rerere_no_crash &&\n+\techo \"bar\" >>test &&\n+\tgit add test &&\n+\tgit commit -m \"three\" &&\n+\n+\ttest_must_fail git rebase main &&\n+\trm .git/rr-cache/*/preimage &&\n+\tgit rebase --abort\n+'\n+\n+test_expect_success 'rerere does not crash with unmatched conflict marker' '\n+\tgit config rerere.enabled true &&\n+\n+\techo bar >test &&\n+\tgit add test &&\n+\tgit commit -m \"one\" &&\n+\tgit branch rerere_no_preimage &&\n+\n+\tcat >test <<-EOF &&\n+\ttest\n+\tbar\n+\tfoobar\n+\tEOF\n+\tgit add test &&\n+\tgit commit -m \"two\" &&\n+\n+\tgit checkout rerere_no_preimage &&\n+\techo \"bar\" >>test &&\n+\tgit add test &&\n+\tgit commit -m \"three\" &&\n+\n+\tcat >test <<-EOF &&\n+\tfoobar\n+\tbar\n+\tbar\n+\tEOF\n+\tgit add test &&\n+\tgit commit -m \"four\" &&\n+\n+\ttest_must_fail git rebase main &&\n+\tcat >test <<-EOF &&\n+\ttest\n+\tbar\n+\t<<<<<<< HEAD\n+\tfoobar\n+\tbar\n+\tEOF\n+\tgit add test &&\n+\tgit rebase --continue\n+'\n+\n test_done\n--\n2.44.0\n\n"},{"id":"492897","messageId":"xmqqr0fai23t.fsf@gitster.g","threadId":"60948","inReplyTo":"20240409121708.131542-2-marcel@roethke.info","subject":"Re: [PATCH v3] rerere: fix crashes due to unmatched opening conflict markers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-12T23:37:58Z","receivedAt":"2024-04-12T23:38:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marcel Röthke writes:\n\n> When rerere handles a conflict with an unmatched opening conflict marker\n> in a file with other conflicts, it will fail create a preimage and also\n> fail allocate the status member of struct rerere_dir. Currently the\n> status member is allocated after the error handling. This will lead to a\n> SEGFAULT when the status member is accessed during cleanup of the failed\n> parse.\n\nNicely diagnosed. Yes, such a corrupt preimage should not enter the\nrerere database as it is unusable for future replaying of the\nresolution. rerere should be prepared to deal with such an input\nmore gracefully.\n\n> Additionally, in subsequent executions of rerere, after removing the\n> MERGE_RR.lock manually, rerere crashes for a similar reason. MERGE_RR\n> points to a conflict id that has no preimage, therefore the status\n> member is not allocated and a SEGFAULT happens when trying to check if a\n> preimage exists.\n>\n> Solve this by making sure the status field is allocated correctly and add\n> tests to prevent the bug from reoccurring.\n\nThanks.\n\n> This does not fix the root cause, failing to parse stray conflict\n> markers, but I don't think we can do much better than recognizing it,\n> printing an error, and moving on gracefully.\n\nI somehow doubt that \"parse stray markers\" is something we _want_ to\ndo in the first place. Is it \"the root cause\" that we refuse/reject\nsuch a corrupt input from entering the rerere database? To me, it\nseems like that the issue is that we do not do a very good job\nrejecting them, keeping the internal state consistent.\n\n> rerere.c | 5 ++++\n> t/t4200-rerere.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 68 insertions(+)\n>\n> diff --git a/rerere.c b/rerere.c\n> index ca7e77ba68..4683d6cbb1 100644\n> --- a/rerere.c\n> +++ b/rerere.c\n> @@ -219,6 +219,11 @@ static void read_rr(struct repository *r, struct string_list *rr)\n> \t\tbuf.buf[hexsz] = '\\0';\n> \t\tid = new_rerere_id_hex(buf.buf);\n> \t\tid->variant = variant;\n> +\t\t/*\n> +\t\t * make sure id->collection->status has enough space\n> +\t\t * for the variant we are interested in\n> +\t\t */\n> +\t\tfit_variant(id->collection, variant);\n> \t\tstring_list_insert(rr, path)->util = id;\n> \t}\n> \tstrbuf_release(&buf);\n\nBoth the fix, and the newly added tests, look great to me.\n\nThanks. Will queue.\n"},{"id":"492963","messageId":"xmqq7cgymlg8.fsf@gitster.g","threadId":"60948","inReplyTo":"20240409121708.131542-2-marcel@roethke.info","subject":"Re: [PATCH v3] rerere: fix crashes due to unmatched opening conflict markers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-15T20:15:35Z","receivedAt":"2024-04-15T20:15:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marcel Röthke writes:\n\n> +test_expect_success 'rerere does not crash with unmatched conflict marker' '\n> +\tgit config rerere.enabled true &&\n> ...\n> +\tgit rebase --continue\n> +'\n> +\n\nThis one fails, either standalone or when merged to 'seen'.\n\nA sample CI run that failed can be seen here (you probably need to\nbe logged in to view it):\n\nhttps://github.com/git/git/actions/runs/8694652245/job/23844028985#step:5:1894\n\n"},{"id":"492990","messageId":"Zh5X7rKXCkDdNqgO@roethke.info","threadId":"60948","inReplyTo":"xmqq7cgymlg8.fsf@gitster.g","subject":"Re: [PATCH v3] rerere: fix crashes due to unmatched opening conflict markers","fromName":"Marcel Röthke","fromEmail":"marcel@roethke.info","sentAt":"2024-04-16T10:50:22Z","receivedAt":"2024-04-16T10:50:33Z","isPatch":true,"sender":{"key":"marcel@roethke.info","avatar":"https://avatars.githubusercontent.com/u/21661035?v=4"},"body":"On 2024-04-15 13:15:35, Junio C Hamano wrote:\n> Marcel Röthke writes:\n>\n> > +test_expect_success 'rerere does not crash with unmatched conflict marker' '\n> > +\tgit config rerere.enabled true &&\n> > ...\n> > +\tgit rebase --continue\n> > +'\n> > +\n>\n> This one fails, either standalone or when merged to 'seen'.\n>\n> A sample CI run that failed can be seen here (you probably need to\n> be logged in to view it):\n>\n> https://github.com/git/git/actions/runs/8694652245/job/23844028985#step:5:1894\n\nYes, sorry that I missed that. The last git rebase command in that test needs\na test_must_fail because we expect a merge conflict.\nWill be fixed in v4.\n"},{"id":"492991","messageId":"20240416105320.1113401-1-marcel@roethke.info","threadId":"60948","inReplyTo":"20240409121708.131542-2-marcel@roethke.info","subject":"[PATCH v4] rerere: fix crashes due to unmatched opening conflict markers","fromName":"Marcel Röthke","fromEmail":"marcel@roethke.info","sentAt":"2024-04-16T10:52:48Z","receivedAt":"2024-04-16T10:54:15Z","isPatch":true,"sender":{"key":"marcel@roethke.info","avatar":"https://avatars.githubusercontent.com/u/21661035?v=4"},"body":"When rerere handles a conflict with an unmatched opening conflict marker\nin a file with other conflicts, it will fail create a preimage and also\nfail allocate the status member of struct rerere_dir. Currently the\nstatus member is allocated after the error handling. This will lead to a\nSEGFAULT when the status member is accessed during cleanup of the failed\nparse.\n\nAdditionally, in subsequent executions of rerere, after removing the\nMERGE_RR.lock manually, rerere crashes for a similar reason. MERGE_RR\npoints to a conflict id that has no preimage, therefore the status\nmember is not allocated and a SEGFAULT happens when trying to check if a\npreimage exists.\n\nSolve this by making sure the status field is allocated correctly and add\ntests to prevent the bug from reoccurring.\n\nThis does not fix the root cause, failing to parse stray conflict\nmarkers, but I don't think we can do much better than recognizing it,\nprinting an error, and moving on gracefully.\n\nSigned-off-by: Marcel Röthke \n---\nInterdiff against v3:\n diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\n index 1e80f76860..b0a3e84984 100755\n --- a/t/t4200-rerere.sh\n +++ b/t/t4200-rerere.sh\n @@ -731,7 +731,7 @@ test_expect_success 'rerere does not crash with unmatched conflict marker' '\n \tbar\n \tEOF\n \tgit add test &&\n -\tgit rebase --continue\n +\ttest_must_fail git rebase --continue\n '\n\n test_done\n\n rerere.c | 5 ++++\n t/t4200-rerere.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 68 insertions(+)\n\ndiff --git a/rerere.c b/rerere.c\nindex ca7e77ba68..4683d6cbb1 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -219,6 +219,11 @@ static void read_rr(struct repository *r, struct string_list *rr)\n \t\tbuf.buf[hexsz] = '\\0';\n \t\tid = new_rerere_id_hex(buf.buf);\n \t\tid->variant = variant;\n+\t\t/*\n+\t\t * make sure id->collection->status has enough space\n+\t\t * for the variant we are interested in\n+\t\t */\n+\t\tfit_variant(id->collection, variant);\n \t\tstring_list_insert(rr, path)->util = id;\n \t}\n \tstrbuf_release(&buf);\ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex fb53dddf79..b0a3e84984 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -671,4 +671,67 @@ test_expect_success 'test simple stage 1 handling' '\n \t)\n '\n\n+test_expect_success 'rerere does not crash with missing preimage' '\n+\tgit config rerere.enabled true &&\n+\n+\techo bar >test &&\n+\tgit add test &&\n+\tgit commit -m \"one\" &&\n+\tgit branch rerere_no_crash &&\n+\n+\techo foo >>test &&\n+\tgit add test &&\n+\tgit commit -m \"two\" &&\n+\n+\tgit checkout rerere_no_crash &&\n+\techo \"bar\" >>test &&\n+\tgit add test &&\n+\tgit commit -m \"three\" &&\n+\n+\ttest_must_fail git rebase main &&\n+\trm .git/rr-cache/*/preimage &&\n+\tgit rebase --abort\n+'\n+\n+test_expect_success 'rerere does not crash with unmatched conflict marker' '\n+\tgit config rerere.enabled true &&\n+\n+\techo bar >test &&\n+\tgit add test &&\n+\tgit commit -m \"one\" &&\n+\tgit branch rerere_no_preimage &&\n+\n+\tcat >test <<-EOF &&\n+\ttest\n+\tbar\n+\tfoobar\n+\tEOF\n+\tgit add test &&\n+\tgit commit -m \"two\" &&\n+\n+\tgit checkout rerere_no_preimage &&\n+\techo \"bar\" >>test &&\n+\tgit add test &&\n+\tgit commit -m \"three\" &&\n+\n+\tcat >test <<-EOF &&\n+\tfoobar\n+\tbar\n+\tbar\n+\tEOF\n+\tgit add test &&\n+\tgit commit -m \"four\" &&\n+\n+\ttest_must_fail git rebase main &&\n+\tcat >test <<-EOF &&\n+\ttest\n+\tbar\n+\t<<<<<<< HEAD\n+\tfoobar\n+\tbar\n+\tEOF\n+\tgit add test &&\n+\ttest_must_fail git rebase --continue\n+'\n+\n test_done\n--\n2.44.0\n\n"}]}