From: Junio C Hamano <gitster@pobox.com>
To: "Marcel Röthke" <marcel@roethke.info>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v3] rerere: fix crashes due to unmatched opening conflict markers
Date: Fri, 12 Apr 2024 16:37:58 -0700 [thread overview]
Message-ID: <xmqqr0fai23t.fsf@gitster.g> (raw)
In-Reply-To: <20240409121708.131542-2-marcel@roethke.info> ("Marcel Röthke"'s message of "Tue, 9 Apr 2024 14:13:51 +0200")
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.
next prev parent reply other threads:[~2024-04-12 23:38 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-18 11:49 [PATCH] rerere: fix crash in during clear Marcel Röthke
2024-02-18 13:02 ` Kristoffer Haugsbakk
2024-02-18 19:38 ` Marcel Röthke
2024-02-18 19:46 ` [PATCH v2] rerere: fix crash " Marcel Röthke
2024-02-20 1:22 ` Junio C Hamano
2024-02-24 11:25 ` Marcel Röthke
2024-03-24 21:51 ` Junio C Hamano
2024-03-25 19:30 ` Marcel Röthke
2024-04-07 20:12 ` Marcel Röthke
2024-04-08 15:18 ` Junio C Hamano
2024-04-09 12:13 ` [PATCH v3] rerere: fix crashes due to unmatched opening conflict markers Marcel Röthke
2024-04-12 23:37 ` Junio C Hamano [this message]
2024-04-15 20:15 ` Junio C Hamano
2024-04-16 10:50 ` Marcel Röthke
2024-04-16 10:52 ` [PATCH v4] " Marcel Röthke
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqr0fai23t.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=marcel@roethke.info \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).