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

[Junio C Hamano <gitster@pobox.com>]

Marcel Röthke <marcel@roethke.info> writes:

> 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.

> 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.

>  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.