| Message ID | 20240409121708.131542-2-marcel@roethke.info (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] rerere: fix crashes due to unmatched opening conflict markers | expand |
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.
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
On 2024-04-15 13:15:35, Junio C Hamano wrote: > 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.
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
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(+) -- 2.44.0