{"thread":{"id":"56470","subject":"Segfaults in git rebase --continue and git rerere","startedAt":"2021-09-09T01:18:08Z","lastAt":"2021-09-09T01:57:47Z","messageCount":2,"participants":["brian m. carlson","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"435196","messageId":"YTlgyHnD3fFWwLu3@camp.crustytoothpaste.net","threadId":"56470","inReplyTo":null,"subject":"Segfaults in git rebase --continue and git rerere","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-09-09T01:18:00Z","receivedAt":"2021-09-09T01:18:08Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"I'm having a bit of a problem with segfaults using\n2.33.0.252.g9b09ab0cd71.  I was in the process of running \"git rebase\n--continue\" with that version to resolve some conflicts in a project I'm\nworking on.  At that point, it segfaulted, and I got this (apologies for\nthe French):\n\n  error: impossible d'analyser la section en conflit dans 'scutiger-lfs/src/bin/git-lfs-transfer.rs'\n  error: impossible d'analyser la section en conflit dans 'scutiger-lfs/src/bin/git-lfs-transfer.rs'\n  Pré-image enregistrée pour 'scutiger-lfs/src/bin/git-lfs-transfer.rs'\n  [HEAD détachée 5134185] scutiger-lfs: move BatchItem, Mode, and Oid to library\n   2 files changed, 74 insertions(+), 9 deletions(-)\n  Fusion automatique de scutiger-lfs/src/processor.rs\n  Fusion automatique de scutiger-lfs/src/bin/git-lfs-transfer.rs\n  CONFLIT (contenu) : Conflit de fusion dans scutiger-lfs/src/bin/git-lfs-transfer.rs\n  error: impossible d'appliquer 2cd1615... scutiger-lfs: move download and verify actions into backend\n  Resolve all conflicts manually, mark them as resolved with\n  \"git add/rm <conflicted_files>\", then run \"git rebase --continue\".\n  You can instead skip this commit: run \"git rebase --skip\".\n  To abort and get back to the state before \"git rebase\", run \"git rebase --abort\".\n  error: impossible d'analyser la section en conflit dans 'scutiger-lfs/src/bin/git-lfs-transfer.rs'\n  zsh: segmentation fault  git rebase --continue\n\nI can provide a tarball of the broken repo upon request.  It's 108 MB,\nso you'll need to provide some place for me to drop it.\n\nAt that point, I needed to remove .git/MERGE_RR.lock (which is empty),\nand I ran \"git rebase --abort\" (because I realized my resolution was\nbad).  Upon which, I received a second segfault (traceback from tip of\nnext):\n\n  #0  0x00000000005b6e6b in has_rerere_resolution (id=0x229b3a0) at rerere.c:162\n  162             return ((id->collection->status[variant] & both) == both);\n  \n  #0  0x00000000005b6e6b in has_rerere_resolution (id=0x229b3a0) at rerere.c:162\n          both = 3\n          variant = 0\n  #1  rerere_clear (r=0x722880 <the_repo>, merge_rr=merge_rr@entry=0x7ffe8c703810) at rerere.c:1239\n          id = 0x229b3a0\n          i = 0\n\nIt appears to be because id->collection->status here is NULL.  It's\nunclear to me why that's the case, but it does appear to be linked to my\n.git/MERGE_RR file, which looks like this (xxd -g1):\n\n  00000000: 30 34 39 62 31 34 32 39 34 65 64 30 63 30 65 66  049b14294ed0c0ef\n  00000010: 62 65 39 32 66 34 66 64 33 31 31 63 37 63 34 30  be92f4fd311c7c40\n  00000020: 39 30 39 34 65 63 64 65 09 73 63 75 74 69 67 65  9094ecde.scutige\n  00000030: 72 2d 6c 66 73 2f 73 72 63 2f 62 69 6e 2f 67 69  r-lfs/src/bin/gi\n  00000040: 74 2d 6c 66 73 2d 74 72 61 6e 73 66 65 72 2e 72  t-lfs-transfer.r\n  00000050: 73 00                                            s.\n\nThe corresponding directory under .git/rr-cache is empty.  This\nspecifically seems to be the problem, and I've included a snippet of a\ntest below that causes the same segfault.  My guess is that the original\nsegfault left the MERGE_RR file present but the files in the rr-cache\ndirectory absent.\n\nSince rerere isn't a strong suit of mine, I'm not sending a patch, but I\nam including a failing test[0] which indicates the problem (and to which\nanyone is welcome to add my sign-off) in hopes that someone more\nfamiliar with this subsystem can figure out what's going on.\n\n----- %< -----\ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex 9f8c76dffb..c44dfe248a 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -623,6 +623,7 @@ test_expect_success 'rerere with inner conflict markers' '\n \tgit commit -q -m \"will solve conflicts later\" &&\n \ttest_must_fail git merge A &&\n \n+\tcp .git/MERGE_RR merge_rr &&\n \techo \"resolved\" >test &&\n \tgit add test &&\n \tgit commit -q -m \"solved conflict\" &&\n@@ -645,6 +646,13 @@ test_expect_success 'rerere with inner conflict markers' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rerere clear does not segfault with bad data' '\n+\tres_id=$($PERL_PATH -nF\"\\t\" -e \"print \\$F[0]\" merge_rr) &&\n+\tcp merge_rr .git/MERGE_RR &&\n+\trm -f .git/rr-cache/$res_id/* &&\n+\tgit rerere clear\n+'\n+\n test_expect_success 'setup simple stage 1 handling' '\n \ttest_create_repo stage_1_handling &&\n \t(\n----- %< -----\n\nAs an aside, I generally find Git's codebase to be of exceptionally good\nquality for a C program, and so seeing two segfaults back to back led me\nto downgrade my recently upgraded version of glibc to see if somehow\nthat was the problem.  Unfortunately, that was not the case, and we just\nhave two separate bugs here.\n\n[0] The test uses perl because the MERGE_RR file contains a NUL byte and\ntherefore cannot be used with standard POSIX utilities.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"435199","messageId":"87tuiufr3c.fsf@evledraar.gmail.com","threadId":"56470","inReplyTo":"YTlgyHnD3fFWwLu3@camp.crustytoothpaste.net","subject":"Re: Segfaults in git rebase --continue and git rerere","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-09T01:48:38Z","receivedAt":"2021-09-09T01:57:47Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 09 2021, brian m. carlson wrote:\n\n> [[PGP Signed Part:Undecided]]\n> I'm having a bit of a problem with segfaults using\n> 2.33.0.252.g9b09ab0cd71.  I was in the process of running \"git rebase\n> --continue\" with that version to resolve some conflicts in a project I'm\n> working on.  At that point, it segfaulted, and I got this (apologies for\n> the French):\n>\n>   error: impossible d'analyser la section en conflit dans 'scutiger-lfs/src/bin/git-lfs-transfer.rs'\n>   error: impossible d'analyser la section en conflit dans 'scutiger-lfs/src/bin/git-lfs-transfer.rs'\n>   Pré-image enregistrée pour 'scutiger-lfs/src/bin/git-lfs-transfer.rs'\n>   [HEAD détachée 5134185] scutiger-lfs: move BatchItem, Mode, and Oid to library\n>    2 files changed, 74 insertions(+), 9 deletions(-)\n>   Fusion automatique de scutiger-lfs/src/processor.rs\n>   Fusion automatique de scutiger-lfs/src/bin/git-lfs-transfer.rs\n>   CONFLIT (contenu) : Conflit de fusion dans scutiger-lfs/src/bin/git-lfs-transfer.rs\n>   error: impossible d'appliquer 2cd1615... scutiger-lfs: move download and verify actions into backend\n>   Resolve all conflicts manually, mark them as resolved with\n>   \"git add/rm <conflicted_files>\", then run \"git rebase --continue\".\n>   You can instead skip this commit: run \"git rebase --skip\".\n>   To abort and get back to the state before \"git rebase\", run \"git rebase --abort\".\n>   error: impossible d'analyser la section en conflit dans 'scutiger-lfs/src/bin/git-lfs-transfer.rs'\n>   zsh: segmentation fault  git rebase --continue\n>\n> I can provide a tarball of the broken repo upon request.  It's 108 MB,\n> so you'll need to provide some place for me to drop it.\n>\n> At that point, I needed to remove .git/MERGE_RR.lock (which is empty),\n> and I ran \"git rebase --abort\" (because I realized my resolution was\n> bad).  Upon which, I received a second segfault (traceback from tip of\n> next):\n>\n>   #0  0x00000000005b6e6b in has_rerere_resolution (id=0x229b3a0) at rerere.c:162\n>   162             return ((id->collection->status[variant] & both) == both);\n>   \n>   #0  0x00000000005b6e6b in has_rerere_resolution (id=0x229b3a0) at rerere.c:162\n>           both = 3\n>           variant = 0\n>   #1  rerere_clear (r=0x722880 <the_repo>, merge_rr=merge_rr@entry=0x7ffe8c703810) at rerere.c:1239\n>           id = 0x229b3a0\n>           i = 0\n>\n> It appears to be because id->collection->status here is NULL.  It's\n> unclear to me why that's the case, but it does appear to be linked to my\n> .git/MERGE_RR file, which looks like this (xxd -g1):\n>\n>   00000000: 30 34 39 62 31 34 32 39 34 65 64 30 63 30 65 66  049b14294ed0c0ef\n>   00000010: 62 65 39 32 66 34 66 64 33 31 31 63 37 63 34 30  be92f4fd311c7c40\n>   00000020: 39 30 39 34 65 63 64 65 09 73 63 75 74 69 67 65  9094ecde.scutige\n>   00000030: 72 2d 6c 66 73 2f 73 72 63 2f 62 69 6e 2f 67 69  r-lfs/src/bin/gi\n>   00000040: 74 2d 6c 66 73 2d 74 72 61 6e 73 66 65 72 2e 72  t-lfs-transfer.r\n>   00000050: 73 00                                            s.\n>\n> The corresponding directory under .git/rr-cache is empty.  This\n> specifically seems to be the problem, and I've included a snippet of a\n> test below that causes the same segfault.  My guess is that the original\n> segfault left the MERGE_RR file present but the files in the rr-cache\n> directory absent.\n>\n> Since rerere isn't a strong suit of mine, I'm not sending a patch, but I\n> am including a failing test[0] which indicates the problem (and to which\n> anyone is welcome to add my sign-off) in hopes that someone more\n> familiar with this subsystem can figure out what's going on.\n\nI haven't taken time to familiarize myself with it, but I have one\nsegfault fix for it already[1] which needs a test, I tried and this is\ncompletely unrelated.\n\nThe \"fix\" for this segfault you're reporting could be, this makes your\ntest pass, along with the rest of the test suite:\n\ndiff --git a/rerere.c b/rerere.c\nindex d83d58df4fb..cbad6a89ebc 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -157,6 +157,9 @@ static int has_rerere_resolution(const struct rerere_id *id)\n \tconst int both = RR_HAS_POSTIMAGE|RR_HAS_PREIMAGE;\n \tint variant = id->variant;\n \n+\tif (variant == id->collection->status_nr)\n+\t\treturn 1;\n+\n \tif (variant < 0)\n \t\treturn 0;\n \treturn ((id->collection->status[variant] & both) == both);\n\nBut I have no idea if that's sensible. I.e. as you found the immediate\ncause here is that our \"id\" is being looked up in\n\"id->collection->status\" where that's NULL, there's similar checks\nelsewhere for \"id->collection->status_nr\", but if that's correct here I\ndon't know, nor what the deeper logic error is of how the two went out\nof sync.\n\n1. https://lore.kernel.org/git/87v96p4w3f.fsf@evledraar.gmail.com/\n\n> ----- %< -----\n> diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\n> index 9f8c76dffb..c44dfe248a 100755\n> --- a/t/t4200-rerere.sh\n> +++ b/t/t4200-rerere.sh\n> @@ -623,6 +623,7 @@ test_expect_success 'rerere with inner conflict markers' '\n>  \tgit commit -q -m \"will solve conflicts later\" &&\n>  \ttest_must_fail git merge A &&\n>  \n> +\tcp .git/MERGE_RR merge_rr &&\n>  \techo \"resolved\" >test &&\n>  \tgit add test &&\n>  \tgit commit -q -m \"solved conflict\" &&\n> @@ -645,6 +646,13 @@ test_expect_success 'rerere with inner conflict markers' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'rerere clear does not segfault with bad data' '\n> +\tres_id=$($PERL_PATH -nF\"\\t\" -e \"print \\$F[0]\" merge_rr) &&\n> +\tcp merge_rr .git/MERGE_RR &&\n> +\trm -f .git/rr-cache/$res_id/* &&\n> +\tgit rerere clear\n> +'\n> +\n>  test_expect_success 'setup simple stage 1 handling' '\n>  \ttest_create_repo stage_1_handling &&\n>  \t(\n> ----- %< -----\n>\n> As an aside, I generally find Git's codebase to be of exceptionally good\n> quality for a C program, and so seeing two segfaults back to back led me\n> to downgrade my recently upgraded version of glibc to see if somehow\n> that was the problem.  Unfortunately, that was not the case, and we just\n> have two separate bugs here.\n>\n> [0] The test uses perl because the MERGE_RR file contains a NUL byte and\n> therefore cannot be used with standard POSIX utilities.\n\n(Just a side-note, I think the use of perl here is fine)\n\nI haven't tried, but I think you can probably do that res_id assignment\nwith \"git grep\" and -o, see t7816-grep-binary-pattern.sh for how we can\nsupport greps with \\0 in them with -f.\n"}]}