# git-core: try_to_follow_renames(): git killed by SIGSEGV 6 messages from 2020-02-27 to 2020-03-10. Participants: Ondrej Pohorelsky, Alexandr Miloslavskiy, SZEDER Gábor, Konstantin Tokarev. Thread: https://gitlist.dev/t/52900 ## Ondrej Pohorelsky, 2020-02-27 12:56 Subject: git-core: try_to_follow_renames(): git killed by SIGSEGV Message-ID: URL: https://gitlist.dev/e/CA%2BB51BFFvn9puia8%2BkheeWkDfOQ7RYHTcGa74M5aeiTd8-QJXA%40mail.gmail.com ``` Hi, there is a SIGSEGV appearing in Fedora[0] with Git 2.24.1 This bug started to appear after update to Git 2.24.1. Bug reporter said that Git crashed on him while running VS Code with Git Lens extension[1] I have tried to reproduce this bug with my own compiled Git with debug flags, but sadly SIGSEGV never appeared. To me it seems like there is a problem in commit a2bb801f6a[2] which changes move_diff_queue() function. This function calls diff_tree_oid() that calls try_to_follow_renames(). In the last two functions there are no arguments checks. Best regards, Ondřej Pohořelský [0] https://retrace.fedoraproject.org/faf/problems/bthash/?bth=25aa7d7267ab5de548ffca337115cb68f7b65105 [1] https://bugzilla.redhat.com/show_bug.cgi?id=1791810 [2] https://git.kernel.org/pub/scm/git/git.git/commit/?id=a2bb801f6a430f6049e5c9729a8f3bf9097d9b34 ``` ## Alexandr Miloslavskiy, 2020-03-06 14:44 Subject: Re: git-core: try_to_follow_renames(): git killed by SIGSEGV Message-ID: <3c722d21-ee57-7d20-81fb-0399f02f1bc7@syntevo.com> URL: https://gitlist.dev/e/3c722d21-ee57-7d20-81fb-0399f02f1bc7%40syntevo.com In-Reply-To: ``` Since I like studying crashes and noone else replied, I decided to have a look. The problem is easy to reproduce with this (replace 1.c with any file): git log --follow -L 1,1:1.c -- 1.c It occurs because `opt->pathspec.items` gets cleaned here: clear_pathspec queue_diffs /* must look at the full tree diff to detect renames */ clear_pathspec(&opt->pathspec); DIFF_QUEUE_CLEAR(&diff_queued_diff); process_ranges_ordinary_commit process_ranges_arbitrary_commit line_log_filter prepare_revision_walk cmd_log_walk cmd_log And on next iteration it crashes in 'try_to_follow_renames' on this line: diff_opts.single_follow = opt->pathspec.items[0].match; I think that bug comes from commit: a2bb801f by SZEDER Gábor, 2019-08-21 13:04:24 line-log: avoid unnecessary full tree diffs @szeder could you please look into that? On 27.02.2020 13:56, Ondrej Pohorelsky wrote: > Hi, > > there is a SIGSEGV appearing in Fedora[0] with Git 2.24.1 > > This bug started to appear after update to Git 2.24.1. > Bug reporter said that Git crashed on him while running VS Code with > Git Lens extension[1] > I have tried to reproduce this bug with my own compiled Git with debug > flags, but sadly SIGSEGV never appeared. > > To me it seems like there is a problem in commit a2bb801f6a[2] which > changes move_diff_queue() function. This function calls > diff_tree_oid() that calls try_to_follow_renames(). In the last two > functions there are no arguments checks. > > Best regards, > Ondřej Pohořelský > > [0] https://retrace.fedoraproject.org/faf/problems/bthash/?bth=25aa7d7267ab5de548ffca337115cb68f7b65105 > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1791810 > [2] https://git.kernel.org/pub/scm/git/git.git/commit/?id=a2bb801f6a430f6049e5c9729a8f3bf9097d9b34 > ``` ## Ondrej Pohorelsky, 2020-03-10 12:07 Subject: Re: git-core: try_to_follow_renames(): git killed by SIGSEGV Message-ID: URL: https://gitlist.dev/e/CA%2BB51BErxp0Nzcc%3D1Ress4tbDmKYmcCqUyC-fmD%2B8O8KdaZLZQ%40mail.gmail.com In-Reply-To: <3c722d21-ee57-7d20-81fb-0399f02f1bc7@syntevo.com> ``` Thank you for your further analyzation and explanation. I would love to make a patch for this bug, but sadly I'm not fully aware of what is going on in this functions and how they are affecting other git functionality. I hope @szeder can look into this bug and provide more explanation as he knows this code best. Best regards, Ondřej Pohořelský On Fri, Mar 6, 2020 at 3:58 PM Alexandr Miloslavskiy wrote: > > Since I like studying crashes and noone else replied, I decided to have > a look. > > The problem is easy to reproduce with this (replace 1.c with any file): > git log --follow -L 1,1:1.c -- 1.c > > It occurs because `opt->pathspec.items` gets cleaned here: > clear_pathspec > queue_diffs > /* must look at the full tree diff to detect renames */ > clear_pathspec(&opt->pathspec); > DIFF_QUEUE_CLEAR(&diff_queued_diff); > process_ranges_ordinary_commit > process_ranges_arbitrary_commit > line_log_filter > prepare_revision_walk > cmd_log_walk > cmd_log > > And on next iteration it crashes in 'try_to_follow_renames' on this line: > diff_opts.single_follow = opt->pathspec.items[0].match; > > I think that bug comes from commit: > a2bb801f by SZEDER Gábor, 2019-08-21 13:04:24 > line-log: avoid unnecessary full tree diffs > > @szeder could you please look into that? > > On 27.02.2020 13:56, Ondrej Pohorelsky wrote: > > Hi, > > > > there is a SIGSEGV appearing in Fedora[0] with Git 2.24.1 > > > > This bug started to appear after update to Git 2.24.1. > > Bug reporter said that Git crashed on him while running VS Code with > > Git Lens extension[1] > > I have tried to reproduce this bug with my own compiled Git with debug > > flags, but sadly SIGSEGV never appeared. > > > > To me it seems like there is a problem in commit a2bb801f6a[2] which > > changes move_diff_queue() function. This function calls > > diff_tree_oid() that calls try_to_follow_renames(). In the last two > > functions there are no arguments checks. > > > > Best regards, > > Ondřej Pohořelský > > > > [0] https://retrace.fedoraproject.org/faf/problems/bthash/?bth=25aa7d7267ab5de548ffca337115cb68f7b65105 > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1791810 > > [2] https://git.kernel.org/pub/scm/git/git.git/commit/?id=a2bb801f6a430f6049e5c9729a8f3bf9097d9b34 > > > ``` ## SZEDER Gábor, 2020-03-10 16:57 Subject: Re: git-core: try_to_follow_renames(): git killed by SIGSEGV Message-ID: <20200310165723.GB3122@szeder.dev> URL: https://gitlist.dev/e/20200310165723.GB3122%40szeder.dev In-Reply-To: <3c722d21-ee57-7d20-81fb-0399f02f1bc7@syntevo.com> ``` On Fri, Mar 06, 2020 at 03:44:34PM +0100, Alexandr Miloslavskiy wrote: > Since I like studying crashes and noone else replied, I decided to have a > look. > > The problem is easy to reproduce with this (replace 1.c with any file): > git log --follow -L 1,1:1.c -- 1.c Don't do this. In particular: - Don't use line-level log with a pathspec, because the documentation of 'git log -L' explicitly told you not to do so ("You may not give any pathspec limiters."). This should have errored out since the beginning, but, unfortunately, has never been enforced. - Don't use '-L' with '--follow'. On one hand, line-level log on its own already follows file renames, even multiple files at once, there is no need for an additional '--follow' (which can only follow one file). OTOH, you shouldn't be able to use '-L' and '--follow' together, because the former forbids a pathspec, while the latter requires one. In any case, '--follow' has always been an ugly hack on top of the revision walking machinery, while line-level log is a rather poorly integrated bolt-on. They simply weren't designed to work together, as evidenced by their contradicting requirements about the pathspec. > It occurs because `opt->pathspec.items` gets cleaned here: > clear_pathspec > queue_diffs > /* must look at the full tree diff to detect renames */ > clear_pathspec(&opt->pathspec); > DIFF_QUEUE_CLEAR(&diff_queued_diff); > process_ranges_ordinary_commit > process_ranges_arbitrary_commit > line_log_filter > prepare_revision_walk > cmd_log_walk > cmd_log > > And on next iteration it crashes in 'try_to_follow_renames' on this line: > diff_opts.single_follow = opt->pathspec.items[0].match; > > I think that bug comes from commit: > a2bb801f by SZEDER Gábor, 2019-08-21 13:04:24 > line-log: avoid unnecessary full tree diffs > > @szeder could you please look into that? > > On 27.02.2020 13:56, Ondrej Pohorelsky wrote: > >Hi, > > > >there is a SIGSEGV appearing in Fedora[0] with Git 2.24.1 > > > >This bug started to appear after update to Git 2.24.1. > >Bug reporter said that Git crashed on him while running VS Code with > >Git Lens extension[1] > >I have tried to reproduce this bug with my own compiled Git with debug > >flags, but sadly SIGSEGV never appeared. > > > >To me it seems like there is a problem in commit a2bb801f6a[2] which > >changes move_diff_queue() function. This function calls > >diff_tree_oid() that calls try_to_follow_renames(). In the last two > >functions there are no arguments checks. > > > >Best regards, > >Ondřej Pohořelský > > > >[0] https://retrace.fedoraproject.org/faf/problems/bthash/?bth=25aa7d7267ab5de548ffca337115cb68f7b65105 > >[1] https://bugzilla.redhat.com/show_bug.cgi?id=1791810 > >[2] https://git.kernel.org/pub/scm/git/git.git/commit/?id=a2bb801f6a430f6049e5c9729a8f3bf9097d9b34 > > > ``` ## Konstantin Tokarev, 2020-03-10 17:30 Subject: Re: git-core: try_to_follow_renames(): git killed by SIGSEGV Message-ID: <347951583861239@vla5-dcf36e533bf7.qloud-c.yandex.net> URL: https://gitlist.dev/e/347951583861239%40vla5-dcf36e533bf7.qloud-c.yandex.net In-Reply-To: <20200310165723.GB3122@szeder.dev> ``` 10.03.2020, 19:57, "SZEDER Gábor" : > On Fri, Mar 06, 2020 at 03:44:34PM +0100, Alexandr Miloslavskiy wrote: >>  Since I like studying crashes and noone else replied, I decided to have a >>  look. >> >>  The problem is easy to reproduce with this (replace 1.c with any file): >>    git log --follow -L 1,1:1.c -- 1.c > > Don't do this. In particular: > >   - Don't use line-level log with a pathspec, because the >     documentation of 'git log -L' explicitly told you not to do so >     ("You may not give any pathspec limiters."). This should have >     errored out since the beginning, but, unfortunately, has never >     been enforced. > >   - Don't use '-L' with '--follow'. On one hand, line-level log on >     its own already follows file renames, even multiple files at once, >     there is no need for an additional '--follow' (which can only >     follow one file). OTOH, you shouldn't be able to use '-L' and >     '--follow' together, because the former forbids a pathspec, while >     the latter requires one. > > In any case, '--follow' has always been an ugly hack on top of the > revision walking machinery, while line-level log is a rather poorly > integrated bolt-on. They simply weren't designed to work together, as > evidenced by their contradicting requirements about the pathspec. This kind of explains issue with --follow and --full-diff which I've reported recently and got completely ignored. Are there any plans to integrate --follow better with other tools? > >>  It occurs because `opt->pathspec.items` gets cleaned here: >>      clear_pathspec >>      queue_diffs >>          /* must look at the full tree diff to detect renames */ >>          clear_pathspec(&opt->pathspec); >>          DIFF_QUEUE_CLEAR(&diff_queued_diff); >>      process_ranges_ordinary_commit >>      process_ranges_arbitrary_commit >>      line_log_filter >>      prepare_revision_walk >>      cmd_log_walk >>      cmd_log >> >>  And on next iteration it crashes in 'try_to_follow_renames' on this line: >>      diff_opts.single_follow = opt->pathspec.items[0].match; >> >>  I think that bug comes from commit: >>      a2bb801f by SZEDER Gábor, 2019-08-21 13:04:24 >>      line-log: avoid unnecessary full tree diffs >> >>  @szeder could you please look into that? >> >>  On 27.02.2020 13:56, Ondrej Pohorelsky wrote: >>  >Hi, >>  > >>  >there is a SIGSEGV appearing in Fedora[0] with Git 2.24.1 >>  > >>  >This bug started to appear after update to Git 2.24.1. >>  >Bug reporter said that Git crashed on him while running VS Code with >>  >Git Lens extension[1] >>  >I have tried to reproduce this bug with my own compiled Git with debug >>  >flags, but sadly SIGSEGV never appeared. >>  > >>  >To me it seems like there is a problem in commit a2bb801f6a[2] which >>  >changes move_diff_queue() function. This function calls >>  >diff_tree_oid() that calls try_to_follow_renames(). In the last two >>  >functions there are no arguments checks. >>  > >>  >Best regards, >>  >Ondřej Pohořelský >>  > >>  >[0] https://retrace.fedoraproject.org/faf/problems/bthash/?bth=25aa7d7267ab5de548ffca337115cb68f7b65105 >>  >[1] https://bugzilla.redhat.com/show_bug.cgi?id=1791810 >>  >[2] https://git.kernel.org/pub/scm/git/git.git/commit/?id=a2bb801f6a430f6049e5c9729a8f3bf9097d9b34 >>  > -- Regards, Konstantin ``` ## Alexandr Miloslavskiy, 2020-03-10 17:35 Subject: Re: git-core: try_to_follow_renames(): git killed by SIGSEGV Message-ID: <28747ec8-0049-b459-0100-420252ca0225@syntevo.com> URL: https://gitlist.dev/e/28747ec8-0049-b459-0100-420252ca0225%40syntevo.com In-Reply-To: <20200310165723.GB3122@szeder.dev> ``` On 10.03.2020 17:57, SZEDER Gábor wrote: > Don't do this. In particular: To clarify, I don't do this, I merely investigated a crash report from someone else. The reporter (Ondrej) seems to be a crash db maintainer, so he also doesn't do this. Finally, the user behind the report also doesn't do this, as he merely uses VS Code. Therefore, "don't do" part is in fact addressed at VS Code. > This should have errored out since the beginning To me it sounds like a perfect plan to avoid the crash. ```