Re: [BUG] git is segfaulting, was [PATCH v4 04/12] dir: also check directories for matching pathspecs

[Elijah Newren <newren@gmail.com>]

Hi Denton,

On Thu, Sep 26, 2019 at 1:35 PM Denton Liu <liu.denton@gmail.com> wrote:
>
> On Wed, Sep 25, 2019 at 02:55:30PM -0700, Denton Liu wrote:
> > Looks correct to me. I don't see why this wouldn't reproduce. I'll send
> > you more information if I figure anything else out.
>
> I looked into it a little more and I think I know why it's being
> triggered.
>
> When we checkout 'todo' from 'master', since they're completely
> different trees, all of git's source files need to be removed. As a
> result, the checkout process at some point invokes check_ok_to_remove().
>
> This kicks off the following call chain:
>
>         check_ok_to_remove()
>         verify_clean_subdirectory()
>         read_directory()
>         read_directory_recursive() (this is called recursively, of course)
>         match_pathspec()
>         do_match_pathspec()
>
> Where we segfault in do_match_pathspec() because ps is NULL:
>
>         GUARD_PATHSPEC(ps,
>                            PATHSPEC_FROMTOP |
>                            PATHSPEC_MAXDEPTH |
>                            PATHSPEC_LITERAL |
>                            PATHSPEC_GLOB |
>                            PATHSPEC_ICASE |
>                            PATHSPEC_EXCLUDE |
>                            PATHSPEC_ATTR);
>
> So why is ps == NULL? In verify_clean_subdirectory(), we call
> read_directory() like this:
>
>         i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
>
> where we explictly pass in a NULL and it is handed down the callstack. I
> guess this means that we should be expecting that pathspecs can be NULL
> in this path. So I've applied the patch at the bottom and it fixes the
> problem.
>
> I was wondering if we should stick a
>
>         if (!ps)
>                 BUG("ps is NULL");
>
> into do_match_pathspec(), though, so we can avoid these situations in
> the future.
>
> Also, I'm still not sure why the issue wasn't reproducible on your
> side... I'm not too familiar with this area of the code, though.
>
> -- >8 --
> diff --git a/dir.c b/dir.c
> index 76a3c3894b..b7a6de58c6 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1952,7 +1952,7 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,
>                         if (subdir_state > dir_state)
>                                 dir_state = subdir_state;
>
> -                       if (!match_pathspec(istate, pathspec, path.buf, path.len,
> +                       if (pathspec && !match_pathspec(istate, pathspec, path.buf, path.len,
>                                             0 /* prefix */, NULL,
>                                             0 /* do NOT special case dirs */))
>                                 state = path_none;

The patch makes sense...but I'd really like to add a test, and
understand it better so I can check to see if there are any other bad
codepaths.  Sadly, I still have no idea how to reproduce the bug.  I
can put

    char *oopsies = NULL;
    printf("oopsies = %s\n", oopsies);

at the beginning of check_ok_to_remove() to verify that function is
never called and run the steps you gave with no problem.  However, I
do notice that your reproduction steps involve 'master' which may have
local changes for you that I don't have.  Is there any chance you can
reproduce this using a commit id that is already upstream instead of
'master'?  I've been poking around unpack-trees.c for a bit but I'm
having a hard time reversing out of it what's different about our
setups and how to trigger.