threads / patch / 60948

patchrerere: fix crash in during clear

Subject: [PATCH] rerere: fix crash in during clear

## tl;dr

15 messages between Feb 18, 2024 and Apr 16, 2024. Diffs are folded; open one to read it.

replies: 14people: 3as markdown or json

Marcel Röthke· Feb 18, 2024, 11:49 UTC · lore

When rerere_clear is called, for instance when aborting a rebase, and the current conflict does not have a pre or postimage recorded git crashes with a SEGFAULT in has_rerere_resolution when accessing the status member of struct rerere_dir. This happens because scan_rerere_dir only allocates the status field in struct rerere_dir when a post or preimage was found. In some cases a segfault may happen even if a post or preimage was recorded if it was not for the variant of interest and the number of the variant that is present is lower than the variant of interest.

This patch solves this by making sure the status field is large enough to accommodate for the variant of interest so it can be accesses without checking if it is large enough.

An alternative solution would be to always check before accessing the status field, but I think the chosen solution aligns better with the assumptions made elsewhere in the code.

Signed-off-by: Marcel Röthke <marcel@roethke.info>
---
 rerere.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to rerere.c +3 −0
diff --git a/rerere.c b/rerere.c
index ca7e77ba68..3973ccce37 100644
--- a/rerere.c
+++ b/rerere.c
@@ -219,6 +219,9 @@ static void read_rr(struct repository *r, struct string_list *rr)
 		buf.buf[hexsz] = '\0';
 		id = new_rerere_id_hex(buf.buf);
 		id->variant = variant;
+		/* make sure id->collection->status has enough space
+		 * for the variant we are interested in */
+		fit_variant(id->collection, variant);
 		string_list_insert(rr, path)->util = id;
 	}
 	strbuf_release(&buf);

-- 
2.43.2
Kristoffer Haugsbakk· Feb 18, 2024, 13:02 UTC · re: Marcel Röthke · lore

Re: [PATCH] rerere: fix crash in during clear

Hi
> rerere: fix crash in during clear
“in during clear”? Did you mean “during clear”?
On Sun, Feb 18, 2024, at 12:49, Marcel Röthke wrote:
Show 11 quoted lines
> When rerere_clear is called, for instance when aborting a rebase, and
> the current conflict does not have a pre or postimage recorded git
> crashes with a SEGFAULT in has_rerere_resolution when accessing the
> status member of struct rerere_dir. This happens because scan_rerere_dir
> only allocates the status field in struct rerere_dir when a post or
> preimage was found. In some cases a segfault may happen even if a post
> or preimage was recorded if it was not for the variant of interest and
> the number of the variant that is present is lower than the variant of
> interest.
>
> This patch solves this by making sure the status field is large enough

You can simplify “This patch solves this” to “Solve this”; see `SubmittingPatches` under “imperative-mood”.

> to accommodate for the variant of interest so it can be accesses without
> checking if it is large enough.
“accessed”
Show 21 quoted lines
>
> An alternative solution would be to always check before accessing the
> status field, but I think the chosen solution aligns better with the
> assumptions made elsewhere in the code.
>
> Signed-off-by: Marcel Röthke <marcel@roethke.info>
> ---
>  rerere.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/rerere.c b/rerere.c
> index ca7e77ba68..3973ccce37 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -219,6 +219,9 @@ static void read_rr(struct repository *r, struct
> string_list *rr)
>  		buf.buf[hexsz] = '\0';
>  		id = new_rerere_id_hex(buf.buf);
>  		id->variant = variant;
> +		/* make sure id->collection->status has enough space
> +		 * for the variant we are interested in */

Multi-line comments should have the delimiters on separate lines from the text. See `CodingGuidelines` under “Multi-line comments”.

Show 6 quoted lines
> +		fit_variant(id->collection, variant);
>  		string_list_insert(rr, path)->util = id;
>  	}
>  	strbuf_release(&buf);
> --
> 2.43.2
Marcel Röthke· Feb 18, 2024, 19:38 UTC · re: Kristoffer Haugsbakk · lore

Re: [PATCH] rerere: fix crash in during clear

On 2024-02-18 14:02:42, Kristoffer Haugsbakk wrote:
> > rerere: fix crash in during clear
>
> “in during clear”? Did you mean “during clear”?
Yes, thanks.
Show 15 quoted lines
> On Sun, Feb 18, 2024, at 12:49, Marcel Röthke wrote:
> > When rerere_clear is called, for instance when aborting a rebase, and
> > the current conflict does not have a pre or postimage recorded git
> > crashes with a SEGFAULT in has_rerere_resolution when accessing the
> > status member of struct rerere_dir. This happens because scan_rerere_dir
> > only allocates the status field in struct rerere_dir when a post or
> > preimage was found. In some cases a segfault may happen even if a post
> > or preimage was recorded if it was not for the variant of interest and
> > the number of the variant that is present is lower than the variant of
> > interest.
> >
> > This patch solves this by making sure the status field is large enough
>
> You can simplify “This patch solves this” to “Solve this”; see
> `SubmittingPatches` under “imperative-mood”.
done
Show 6 quoted lines
>
> > to accommodate for the variant of interest so it can be accesses without
> > checking if it is large enough.
>
> “accessed”
>
done
Show 24 quoted lines
> >
> > An alternative solution would be to always check before accessing the
> > status field, but I think the chosen solution aligns better with the
> > assumptions made elsewhere in the code.
> >
> > Signed-off-by: Marcel Röthke <marcel@roethke.info>
> > ---
> >  rerere.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/rerere.c b/rerere.c
> > index ca7e77ba68..3973ccce37 100644
> > --- a/rerere.c
> > +++ b/rerere.c
> > @@ -219,6 +219,9 @@ static void read_rr(struct repository *r, struct
> > string_list *rr)
> >  		buf.buf[hexsz] = '\0';
> >  		id = new_rerere_id_hex(buf.buf);
> >  		id->variant = variant;
> > +		/* make sure id->collection->status has enough space
> > +		 * for the variant we are interested in */
>
> Multi-line comments should have the delimiters on separate lines from
> the text. See `CodingGuidelines` under “Multi-line comments”.
done
Thank you for your feedback!
Marcel Röthke· Feb 18, 2024, 19:46 UTC · re: Marcel Röthke · lore

[PATCH v2] rerere: fix crash during clear

When rerere_clear is called, for instance when aborting a rebase, and the current conflict does not have a pre or postimage recorded git crashes with a SEGFAULT in has_rerere_resolution when accessing the status member of struct rerere_dir. This happens because scan_rerere_dir only allocates the status field in struct rerere_dir when a post or preimage was found. In some cases a segfault may happen even if a post or preimage was recorded if it was not for the variant of interest and the number of the variant that is present is lower than the variant of interest.

Solve this by making sure the status field is large enough to accommodate for the variant of interest so it can be accessed without checking if it is large enough.

An alternative solution would be to always check before accessing the status field, but I think the chosen solution aligns better with the assumptions made elsewhere in the code.

Signed-off-by: Marcel Röthke <marcel@roethke.info>
---
Range-diff against v1:
1:  93f982d170 ! 1:  68178298fe rerere: fix crash in during clear
    @@ Metadata
     Author: Marcel Röthke <marcel@roethke.info>
     
      ## Commit message ##
    -    rerere: fix crash in during clear
    +    rerere: fix crash during clear
     
         When rerere_clear is called, for instance when aborting a rebase, and
         the current conflict does not have a pre or postimage recorded git
    @@ Commit message
         the number of the variant that is present is lower than the variant of
         interest.
     
    -    This patch solves this by making sure the status field is large enough
    -    to accommodate for the variant of interest so it can be accesses without
    +    Solve this by making sure the status field is large enough to
    +    accommodate for the variant of interest so it can be accessed without
         checking if it is large enough.
     
         An alternative solution would be to always check before accessing the
    @@ rerere.c: static void read_rr(struct repository *r, struct string_list *rr)
      		buf.buf[hexsz] = '\0';
      		id = new_rerere_id_hex(buf.buf);
      		id->variant = variant;
    -+		/* make sure id->collection->status has enough space
    -+		 * for the variant we are interested in */
    ++		/*
    ++		 * make sure id->collection->status has enough space
    ++		 * for the variant we are interested in
    ++		 */
     +		fit_variant(id->collection, variant);
      		string_list_insert(rr, path)->util = id;
      	}
 rerere.c | 5 +++++
 1 file changed, 5 insertions(+)
Show changes to rerere.c +5 −0
diff --git a/rerere.c b/rerere.c
index ca7e77ba68..4683d6cbb1 100644
--- a/rerere.c
+++ b/rerere.c
@@ -219,6 +219,11 @@ static void read_rr(struct repository *r, struct string_list *rr)
 		buf.buf[hexsz] = '\0';
 		id = new_rerere_id_hex(buf.buf);
 		id->variant = variant;
+		/*
+		 * make sure id->collection->status has enough space
+		 * for the variant we are interested in
+		 */
+		fit_variant(id->collection, variant);
 		string_list_insert(rr, path)->util = id;
 	}
 	strbuf_release(&buf);

-- 
2.43.2
Junio C Hamano· Feb 20, 2024, 01:22 UTC · re: Marcel Röthke · lore

Re: [PATCH v2] rerere: fix crash during clear

Marcel Röthke <marcel@roethke.info> writes:
> When rerere_clear is called, for instance when aborting a rebase, and
> the current conflict does not have a pre or postimage recorded git
> crashes with a SEGFAULT in has_rerere_resolution when accessing the
> status member of struct rerere_dir.

I had to read this twice before realizing the reason why I found it hard to grok was because of a missing comma between "recorded" and "git".

> This happens because scan_rerere_dir
> only allocates the status field in struct rerere_dir when a post or
> preimage was found.

But that is not really the root cause, no? Readers following the above text are probably wondering why the preimage was not recorded, when a conflict resulted in stopping a mergy-command and invoking rerere machinery, before rerere_clear() got called. Is that something that usually happen? How? Do we have a reproduction sequence of such a state that we can make it into a new test in t4200 where we already have tests for "git rerere clear" and its friends?

> In some cases a segfault may happen even if a post
> or preimage was recorded if it was not for the variant of interest and
> the number of the variant that is present is lower than the variant of
> interest.
Ditto.  What sequence of events would lead to such a state?

The answer *can* be ".git/rr-cache being a normal directory, the user can poke around, removing files randomly, which can create such a problematic situation", and the reproduction test *can* also be to simulate such an end-user action, but I am asking primarily because I want to make sure that we are *not* losing or failing to create necessary preimage files, causing this problem to users who do not muck with files in .git/rr-cache themselves.

Thanks.
Marcel Röthke· Feb 24, 2024, 11:25 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] rerere: fix crash during clear

On 2024-02-19 17:22:43, Junio C Hamano wrote:
Show 10 quoted lines
> Marcel Röthke <marcel@roethke.info> writes:
>
> > When rerere_clear is called, for instance when aborting a rebase, and
> > the current conflict does not have a pre or postimage recorded git
> > crashes with a SEGFAULT in has_rerere_resolution when accessing the
> > status member of struct rerere_dir.
>
> I had to read this twice before realizing the reason why I found it
> hard to grok was because of a missing comma between "recorded" and
> "git".
fixed
Show 12 quoted lines
> > This happens because scan_rerere_dir
> > only allocates the status field in struct rerere_dir when a post or
> > preimage was found.
>
> But that is not really the root cause, no?  Readers following the
> above text are probably wondering why the preimage was not recorded,
> when a conflict resulted in stopping a mergy-command and invoking
> rerere machinery, before rerere_clear() got called.  Is that
> something that usually happen?  How?  Do we have a reproduction
> sequence of such a state that we can make it into a new test in
> t4200 where we already have tests for "git rerere clear" and its
> friends?

I'm unfortunately not sure how it happened. I do have the initial state of the repository and I think I know the commands that were executed, but I could not reproduce it.

I will look into adding a test case though.
Junio C Hamano· Mar 24, 2024, 21:51 UTC · re: Marcel Röthke · lore

Re: [PATCH v2] rerere: fix crash during clear

Marcel Röthke <marcel@roethke.info> writes:
Show 19 quoted lines
> On 2024-02-19 17:22:43, Junio C Hamano wrote:
>> Marcel Röthke <marcel@roethke.info> writes:
>>
>> > When rerere_clear is called, for instance when aborting a rebase, and
>> > the current conflict does not have a pre or postimage recorded git
>> > crashes with a SEGFAULT in has_rerere_resolution when accessing the
>> > status member of struct rerere_dir.
>>
>> I had to read this twice before realizing the reason why I found it
>> hard to grok was because of a missing comma between "recorded" and
>> "git".
>
> fixed
> ...
> I'm unfortunately not sure how it happened. I do have the initial state
> of the repository and I think I know the commands that were executed,
> but I could not reproduce it.
>
> I will look into adding a test case though.
It has been about a month.  Any new development on this topic?
Thanks.
Marcel Röthke· Mar 25, 2024, 19:30 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] rerere: fix crash during clear

On 2024-03-24 14:51:17, Junio C Hamano wrote:
Show 25 quoted lines
> Marcel Röthke <marcel@roethke.info> writes:
>
> > On 2024-02-19 17:22:43, Junio C Hamano wrote:
> >> Marcel Röthke <marcel@roethke.info> writes:
> >>
> >> > When rerere_clear is called, for instance when aborting a rebase, and
> >> > the current conflict does not have a pre or postimage recorded git
> >> > crashes with a SEGFAULT in has_rerere_resolution when accessing the
> >> > status member of struct rerere_dir.
> >>
> >> I had to read this twice before realizing the reason why I found it
> >> hard to grok was because of a missing comma between "recorded" and
> >> "git".
> >
> > fixed
> > ...
> > I'm unfortunately not sure how it happened. I do have the initial state
> > of the repository and I think I know the commands that were executed,
> > but I could not reproduce it.
> >
> > I will look into adding a test case though.
>
> It has been about a month.  Any new development on this topic?
>
> Thanks.
No, I have been otherwise occupied. But I plan to get back to this soon. Probably this week, next week at the latest.
Marcel Röthke· Apr 7, 2024, 20:12 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] rerere: fix crash during clear

After reproducing this without manually corrupting the database, I noticed that the summary line is no longer accurate. Because it fixes two SEGFAULTs and one of them does not happen during clear. Can I change this in v3 or should I start a new thread?

Junio C Hamano· Apr 8, 2024, 15:18 UTC · re: Marcel Röthke · lore

Re: [PATCH v2] rerere: fix crash during clear

Marcel Röthke <marcel@roethke.info> writes:
> After reproducing this without manually corrupting the database, I
> noticed that the summary line is no longer accurate. Because it fixes
> two SEGFAULTs and one of them does not happen during clear. Can I change this
> in v3 or should I start a new thread?

Updating the title in the course of evolution of a topic/patch is perfectly normal. Please do so in v3 as continuation of the previous round.

Thanks.
Marcel Röthke· Apr 9, 2024, 12:13 UTC · re: Marcel Röthke · lore

[PATCH v3] rerere: fix crashes due to unmatched opening conflict markers

When rerere handles a conflict with an unmatched opening conflict marker in a file with other conflicts, it will fail create a preimage and also fail allocate the status member of struct rerere_dir. Currently the status member is allocated after the error handling. This will lead to a SEGFAULT when the status member is accessed during cleanup of the failed parse.

Additionally, in subsequent executions of rerere, after removing the MERGE_RR.lock manually, rerere crashes for a similar reason. MERGE_RR points to a conflict id that has no preimage, therefore the status member is not allocated and a SEGFAULT happens when trying to check if a preimage exists.

Solve this by making sure the status field is allocated correctly and add tests to prevent the bug from reoccurring.

This does not fix the root cause, failing to parse stray conflict markers, but I don't think we can do much better than recognizing it, printing an error, and moving on gracefully.

Signed-off-by: Marcel Röthke <marcel@roethke.info>
---
Interdiff against v2:
  diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
  index fb53dddf79..1e80f76860 100755
  --- a/t/t4200-rerere.sh
  +++ b/t/t4200-rerere.sh
  @@ -671,4 +671,67 @@ test_expect_success 'test simple stage 1 handling' '
   	)
   '
  +test_expect_success 'rerere does not crash with missing preimage' '
  +	git config rerere.enabled true &&
  +
  +	echo bar >test &&
  +	git add test &&
  +	git commit -m "one" &&
  +	git branch rerere_no_crash &&
  +
  +	echo foo >>test &&
  +	git add test &&
  +	git commit -m "two" &&
  +
  +	git checkout rerere_no_crash &&
  +	echo "bar" >>test &&
  +	git add test &&
  +	git commit -m "three" &&
  +
  +	test_must_fail git rebase main &&
  +	rm .git/rr-cache/*/preimage &&
  +	git rebase --abort
  +'
  +
  +test_expect_success 'rerere does not crash with unmatched conflict marker' '
  +	git config rerere.enabled true &&
  +
  +	echo bar >test &&
  +	git add test &&
  +	git commit -m "one" &&
  +	git branch rerere_no_preimage &&
  +
  +	cat >test <<-EOF &&
  +	test
  +	bar
  +	foobar
  +	EOF
  +	git add test &&
  +	git commit -m "two" &&
  +
  +	git checkout rerere_no_preimage &&
  +	echo "bar" >>test &&
  +	git add test &&
  +	git commit -m "three" &&
  +
  +	cat >test <<-EOF &&
  +	foobar
  +	bar
  +	bar
  +	EOF
  +	git add test &&
  +	git commit -m "four" &&
  +
  +	test_must_fail git rebase main &&
  +	cat >test <<-EOF &&
  +	test
  +	bar
  +	<<<<<<< HEAD
  +	foobar
  +	bar
  +	EOF
  +	git add test &&
  +	git rebase --continue
  +'
  +
   test_done
 rerere.c          |  5 ++++
 t/t4200-rerere.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)
Show changes to 2 files +68 −1

rerere.c, t/t4200-rerere.sh

diff --git a/rerere.c b/rerere.c
index ca7e77ba68..4683d6cbb1 100644
--- a/rerere.c
+++ b/rerere.c
@@ -219,6 +219,11 @@ static void read_rr(struct repository *r, struct string_list *rr)
 		buf.buf[hexsz] = '\0';
 		id = new_rerere_id_hex(buf.buf);
 		id->variant = variant;
+		/*
+		 * make sure id->collection->status has enough space
+		 * for the variant we are interested in
+		 */
+		fit_variant(id->collection, variant);
 		string_list_insert(rr, path)->util = id;
 	}
 	strbuf_release(&buf);
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index fb53dddf79..1e80f76860 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -671,4 +671,67 @@ test_expect_success 'test simple stage 1 handling' '
 	)
 '

+test_expect_success 'rerere does not crash with missing preimage' '
+	git config rerere.enabled true &&
+
+	echo bar >test &&
+	git add test &&
+	git commit -m "one" &&
+	git branch rerere_no_crash &&
+
+	echo foo >>test &&
+	git add test &&
+	git commit -m "two" &&
+
+	git checkout rerere_no_crash &&
+	echo "bar" >>test &&
+	git add test &&
+	git commit -m "three" &&
+
+	test_must_fail git rebase main &&
+	rm .git/rr-cache/*/preimage &&
+	git rebase --abort
+'
+
+test_expect_success 'rerere does not crash with unmatched conflict marker' '
+	git config rerere.enabled true &&
+
+	echo bar >test &&
+	git add test &&
+	git commit -m "one" &&
+	git branch rerere_no_preimage &&
+
+	cat >test <<-EOF &&
+	test
+	bar
+	foobar
+	EOF
+	git add test &&
+	git commit -m "two" &&
+
+	git checkout rerere_no_preimage &&
+	echo "bar" >>test &&
+	git add test &&
+	git commit -m "three" &&
+
+	cat >test <<-EOF &&
+	foobar
+	bar
+	bar
+	EOF
+	git add test &&
+	git commit -m "four" &&
+
+	test_must_fail git rebase main &&
+	cat >test <<-EOF &&
+	test
+	bar
+	<<<<<<< HEAD
+	foobar
+	bar
+	EOF
+	git add test &&
+	git rebase --continue
+'
+
 test_done
--
2.44.0
Junio C Hamano· Apr 12, 2024, 23:37 UTC · re: Marcel Röthke · lore

Re: [PATCH v3] rerere: fix crashes due to unmatched opening conflict markers

Marcel Röthke <marcel@roethke.info> writes:
Show 6 quoted lines
> When rerere handles a conflict with an unmatched opening conflict marker
> in a file with other conflicts, it will fail create a preimage and also
> fail allocate the status member of struct rerere_dir. Currently the
> status member is allocated after the error handling. This will lead to a
> SEGFAULT when the status member is accessed during cleanup of the failed
> parse.

Nicely diagnosed. Yes, such a corrupt preimage should not enter the rerere database as it is unusable for future replaying of the resolution. rerere should be prepared to deal with such an input more gracefully.

Show 8 quoted lines
> Additionally, in subsequent executions of rerere, after removing the
> MERGE_RR.lock manually, rerere crashes for a similar reason. MERGE_RR
> points to a conflict id that has no preimage, therefore the status
> member is not allocated and a SEGFAULT happens when trying to check if a
> preimage exists.
>
> Solve this by making sure the status field is allocated correctly and add
> tests to prevent the bug from reoccurring.
Thanks.
> This does not fix the root cause, failing to parse stray conflict
> markers, but I don't think we can do much better than recognizing it,
> printing an error, and moving on gracefully.

I somehow doubt that "parse stray markers" is something we _want_ to do in the first place. Is it "the root cause" that we refuse/reject such a corrupt input from entering the rerere database? To me, it seems like that the issue is that we do not do a very good job rejecting them, keeping the internal state consistent.

Show 20 quoted lines
>  rerere.c          |  5 ++++
>  t/t4200-rerere.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+)
>
> diff --git a/rerere.c b/rerere.c
> index ca7e77ba68..4683d6cbb1 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -219,6 +219,11 @@ static void read_rr(struct repository *r, struct string_list *rr)
>  		buf.buf[hexsz] = '\0';
>  		id = new_rerere_id_hex(buf.buf);
>  		id->variant = variant;
> +		/*
> +		 * make sure id->collection->status has enough space
> +		 * for the variant we are interested in
> +		 */
> +		fit_variant(id->collection, variant);
>  		string_list_insert(rr, path)->util = id;
>  	}
>  	strbuf_release(&buf);
Both the fix, and the newly added tests, look great to me.
Thanks.  Will queue.
Junio C Hamano· Apr 15, 2024, 20:15 UTC · re: Marcel Röthke · lore

Re: [PATCH v3] rerere: fix crashes due to unmatched opening conflict markers

Marcel Röthke <marcel@roethke.info> writes:
Show 6 quoted lines
> +test_expect_success 'rerere does not crash with unmatched conflict marker' '
> +	git config rerere.enabled true &&
> ...
> +	git rebase --continue
> +'
> +
This one fails, either standalone or when merged to 'seen'.

A sample CI run that failed can be seen here (you probably need to be logged in to view it):

https://github.com/git/git/actions/runs/8694652245/job/23844028985#step:5:1894
Marcel Röthke· Apr 16, 2024, 10:50 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] rerere: fix crashes due to unmatched opening conflict markers

On 2024-04-15 13:15:35, Junio C Hamano wrote:
Show 15 quoted lines
> Marcel Röthke <marcel@roethke.info> writes:
>
> > +test_expect_success 'rerere does not crash with unmatched conflict marker' '
> > +	git config rerere.enabled true &&
> > ...
> > +	git rebase --continue
> > +'
> > +
>
> This one fails, either standalone or when merged to 'seen'.
>
> A sample CI run that failed can be seen here (you probably need to
> be logged in to view it):
>
> https://github.com/git/git/actions/runs/8694652245/job/23844028985#step:5:1894

Yes, sorry that I missed that. The last git rebase command in that test needs a test_must_fail because we expect a merge conflict. Will be fixed in v4.

Marcel Röthke· Apr 16, 2024, 10:52 UTC · re: Marcel Röthke · lore

[PATCH v4] rerere: fix crashes due to unmatched opening conflict markers

When rerere handles a conflict with an unmatched opening conflict marker in a file with other conflicts, it will fail create a preimage and also fail allocate the status member of struct rerere_dir. Currently the status member is allocated after the error handling. This will lead to a SEGFAULT when the status member is accessed during cleanup of the failed parse.

Additionally, in subsequent executions of rerere, after removing the MERGE_RR.lock manually, rerere crashes for a similar reason. MERGE_RR points to a conflict id that has no preimage, therefore the status member is not allocated and a SEGFAULT happens when trying to check if a preimage exists.

Solve this by making sure the status field is allocated correctly and add tests to prevent the bug from reoccurring.

This does not fix the root cause, failing to parse stray conflict markers, but I don't think we can do much better than recognizing it, printing an error, and moving on gracefully.

Signed-off-by: Marcel Röthke <marcel@roethke.info>
---
Interdiff against v3:
  diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
  index 1e80f76860..b0a3e84984 100755
  --- a/t/t4200-rerere.sh
  +++ b/t/t4200-rerere.sh
  @@ -731,7 +731,7 @@ test_expect_success 'rerere does not crash with unmatched conflict marker' '
   	bar
   	EOF
   	git add test &&
  -	git rebase --continue
  +	test_must_fail git rebase --continue
   '
   test_done
 rerere.c          |  5 ++++
 t/t4200-rerere.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)
Show changes to 2 files +68 −1

rerere.c, t/t4200-rerere.sh

diff --git a/rerere.c b/rerere.c
index ca7e77ba68..4683d6cbb1 100644
--- a/rerere.c
+++ b/rerere.c
@@ -219,6 +219,11 @@ static void read_rr(struct repository *r, struct string_list *rr)
 		buf.buf[hexsz] = '\0';
 		id = new_rerere_id_hex(buf.buf);
 		id->variant = variant;
+		/*
+		 * make sure id->collection->status has enough space
+		 * for the variant we are interested in
+		 */
+		fit_variant(id->collection, variant);
 		string_list_insert(rr, path)->util = id;
 	}
 	strbuf_release(&buf);
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index fb53dddf79..b0a3e84984 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -671,4 +671,67 @@ test_expect_success 'test simple stage 1 handling' '
 	)
 '

+test_expect_success 'rerere does not crash with missing preimage' '
+	git config rerere.enabled true &&
+
+	echo bar >test &&
+	git add test &&
+	git commit -m "one" &&
+	git branch rerere_no_crash &&
+
+	echo foo >>test &&
+	git add test &&
+	git commit -m "two" &&
+
+	git checkout rerere_no_crash &&
+	echo "bar" >>test &&
+	git add test &&
+	git commit -m "three" &&
+
+	test_must_fail git rebase main &&
+	rm .git/rr-cache/*/preimage &&
+	git rebase --abort
+'
+
+test_expect_success 'rerere does not crash with unmatched conflict marker' '
+	git config rerere.enabled true &&
+
+	echo bar >test &&
+	git add test &&
+	git commit -m "one" &&
+	git branch rerere_no_preimage &&
+
+	cat >test <<-EOF &&
+	test
+	bar
+	foobar
+	EOF
+	git add test &&
+	git commit -m "two" &&
+
+	git checkout rerere_no_preimage &&
+	echo "bar" >>test &&
+	git add test &&
+	git commit -m "three" &&
+
+	cat >test <<-EOF &&
+	foobar
+	bar
+	bar
+	EOF
+	git add test &&
+	git commit -m "four" &&
+
+	test_must_fail git rebase main &&
+	cat >test <<-EOF &&
+	test
+	bar
+	<<<<<<< HEAD
+	foobar
+	bar
+	EOF
+	git add test &&
+	test_must_fail git rebase --continue
+'
+
 test_done
--
2.44.0

← back to recent threads