git-core: try_to_follow_renames(): git killed by SIGSEGV

git-core: try_to_follow_renames(): git killed by SIGSEGV

[Ondrej Pohorelsky]

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

Re: git-core: try_to_follow_renames(): git killed by SIGSEGV

[Alexandr Miloslavskiy]

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
> 

Re: git-core: try_to_follow_renames(): git killed by SIGSEGV

[Ondrej Pohorelsky]

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
<alexandr.miloslavskiy@syntevo.com> 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
> >
>

Re: git-core: try_to_follow_renames(): git killed by SIGSEGV

[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.

> 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
> >
> 

Re: git-core: try_to_follow_renames(): git killed by SIGSEGV

[Konstantin Tokarev]

10.03.2020, 19:57, "SZEDER Gábor" <szeder.dev@gmail.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.

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

Re: git-core: try_to_follow_renames(): git killed by SIGSEGV

[Alexandr Miloslavskiy]

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.