Marcel Röthke <marcel@xxxxxxxxxxxx> 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.
