Hi Denton,
On Thu, Sep 26, 2019 at 1:35 PM Denton Liu <liu.denton@xxxxxxxxx> 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.
