git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Elijah Newren <newren@gmail.com>
Date
Sep 27, 2019, 00:12 UTC
Message-ID
<CABPp-BGWG-HExZSwzOL75-9Oi0Cs1c8dc6Hq9TotgpQa0XmNvg@mail.gmail.com>
In-Reply-To
<20190926203509.GA61514@generichostname>
Hi Denton,
On Thu, Sep 26, 2019 at 1:35 PM Denton Liu <liu.denton@gmail.com> wrote:
Show 67 quoted lines
>
> 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.

Previous: Denton LiuNext: SZEDER Gábor
Message 51 of 73 in “t7300-clean: demonstrate deleting nested repo with an ignored file breakage”
  1. t7300-clean: demonstrate deleting nested repo with an ignored file breakageSZEDER Gábor, Aug 25, 2019
  2. SZEDER GáborAug 25, 2019
  3. Philip OakleyAug 25, 2019
  4. SZEDER GáborAug 26, 2019
  5. 00/12 Fix some git clean issuesElijah Newren, Sep 5, 2019
  6. 01/12 t7300: Add some testcases showing failure to clean specified pathspecsElijah Newren, Sep 5, 2019
  7. 02/12 dir: fix typo in commentElijah Newren, Sep 5, 2019
  8. 04/12 dir: Directories should be checked for matching pathspecs tooElijah Newren, Sep 5, 2019
  9. 03/12 dir: fix off-by-one error in match_pathspec_itemElijah Newren, Sep 5, 2019
  10. 05/12 dir: Make the DO_MATCH_SUBMODULE code reusable for a non-submodule caseElijah Newren, Sep 5, 2019
  11. 07/12 dir: add commentary explaining match_pathspec_item's return valueElijah Newren, Sep 5, 2019
  12. 06/12 dir: If our pathspec might match files under a dir, recurse into itElijah Newren, Sep 5, 2019
  13. 10/12 clean: avoid removing untracked files in a nested git repositoryElijah Newren, Sep 5, 2019
  14. SZEDER GáborSep 5, 2019
  15. 09/12 clean: disambiguate the definition of -dElijah Newren, Sep 5, 2019
  16. 11/12 clean: rewrap overly long lineElijah Newren, Sep 5, 2019
  17. 12/12 clean: fix theoretical path corruptionElijah Newren, Sep 5, 2019
  18. SZEDER GáborSep 5, 2019
  19. Elijah NewrenSep 7, 2019
  20. 08/12 git-clean.txt: do not claim we will delete files with -n/--dry-runElijah Newren, Sep 5, 2019
  21. SZEDER GáborSep 5, 2019
  22. Elijah NewrenSep 7, 2019
  23. 00/12 Fix some git clean issuesElijah Newren, Sep 12, 2019
  24. 01/12 t7300: add testcases showing failure to clean specified pathspecsElijah Newren, Sep 12, 2019
  25. Junio C HamanoSep 13, 2019
  26. Elijah NewrenSep 13, 2019
  27. Junio C HamanoSep 13, 2019
  28. 03/12 dir: fix off-by-one error in match_pathspec_itemElijah Newren, Sep 12, 2019
  29. Junio C HamanoSep 13, 2019
  30. 02/12 dir: fix typo in commentElijah Newren, Sep 12, 2019
  31. 05/12 dir: make the DO_MATCH_SUBMODULE code reusable for a non-submodule caseElijah Newren, Sep 12, 2019
  32. 04/12 dir: also check directories for matching pathspecsElijah Newren, Sep 12, 2019
  33. 06/12 dir: if our pathspec might match files under a dir, recurse into itElijah Newren, Sep 12, 2019
  34. Junio C HamanoSep 13, 2019
  35. 07/12 dir: add commentary explaining match_pathspec_item's return valueElijah Newren, Sep 12, 2019
  36. Junio C HamanoSep 13, 2019
  37. 09/12 clean: disambiguate the definition of -dElijah Newren, Sep 12, 2019
  38. 10/12 clean: avoid removing untracked files in a nested git repositoryElijah Newren, Sep 12, 2019
  39. 11/12 clean: rewrap overly long lineElijah Newren, Sep 12, 2019
  40. 08/12 git-clean.txt: do not claim we will delete files with -n/--dry-runElijah Newren, Sep 12, 2019
  41. 12/12 clean: fix theoretical path corruptionElijah Newren, Sep 12, 2019
  42. 00/12 Fix some git clean issuesElijah Newren, Sep 17, 2019
  43. 02/12 dir: fix typo in commentElijah Newren, Sep 17, 2019
  44. 01/12 t7300: add testcases showing failure to clean specified pathspecsElijah Newren, Sep 17, 2019
  45. 03/12 dir: fix off-by-one error in match_pathspec_itemElijah Newren, Sep 17, 2019
  46. 04/12 dir: also check directories for matching pathspecsElijah Newren, Sep 17, 2019
  47. [BUG] git is segfaulting, was [PATCH v4 04/12] dir: also check directories for matching pathspecsDenton Liu, Sep 25, 2019
  48. Elijah NewrenSep 25, 2019
  49. Denton LiuSep 25, 2019
  50. Denton LiuSep 26, 2019
  51. Elijah NewrenSep 27, 2019
  52. SZEDER GáborSep 27, 2019
  53. SZEDER GáborSep 27, 2019
  54. Denton LiuSep 27, 2019
  55. dir: special case check for the possibility that pathspec is NULLElijah Newren, Sep 30, 2019
  56. Denton LiuSep 30, 2019
  57. Elijah NewrenOct 1, 2019
  58. dir: special case check for the possibility that pathspec is NULLElijah Newren, Oct 1, 2019
  59. Denton LiuOct 1, 2019
  60. Elijah NewrenOct 1, 2019
  61. dir: special case check for the possibility that pathspec is NULLElijah Newren, Oct 1, 2019
  62. Denton LiuOct 1, 2019
  63. Elijah NewrenOct 1, 2019
  64. Elijah NewrenOct 2, 2019
  65. SZEDER GáborOct 7, 2019
  66. 08/12 git-clean.txt: do not claim we will delete files with -n/--dry-runElijah Newren, Sep 17, 2019
  67. 11/12 clean: rewrap overly long lineElijah Newren, Sep 17, 2019
  68. 12/12 clean: fix theoretical path corruptionElijah Newren, Sep 17, 2019
  69. 09/12 clean: disambiguate the definition of -dElijah Newren, Sep 17, 2019
  70. 10/12 clean: avoid removing untracked files in a nested git repositoryElijah Newren, Sep 17, 2019
  71. 07/12 dir: add commentary explaining match_pathspec_item's return valueElijah Newren, Sep 17, 2019
  72. 06/12 dir: if our pathspec might match files under a dir, recurse into itElijah Newren, Sep 17, 2019
  73. 05/12 dir: make the DO_MATCH_SUBMODULE code reusable for a non-submodule caseElijah Newren, Sep 17, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.