Re: [PATCH v2 6/8] dir: fix checks on common prefix directory

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Elijah,

On Mon, 16 Dec 2019, Elijah Newren wrote:

> On Mon, Dec 16, 2019 at 5:51 AM Elijah Newren <newren@gmail.com> wrote:
> >
> > On Sun, Dec 15, 2019 at 2:29 AM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > >
> > > Hi Elijah,
> > >
> > > I have not had time to dive deeply into this, but I know that it _does_
> > > cause a ton of segmentation faults in the `shears/pu` branch (where all of
> > > Git for Windows' patches are rebased on top of `pu`):
> >
> > Weird.  If it's going to cause segmentation faults at all, it would
> > certainly do it all over the place, but I tested the patches on the
> > major platforms using your Azure Pipelines setup on git.git so it
> > should be good on all the platforms.  Did your shears/pu branch make
> > some other changes to the setup?

Not really.

> > > On Tue, 10 Dec 2019, Elijah Newren via GitGitGadget wrote:
> > >
> > > > diff --git a/dir.c b/dir.c
> > > > index 645b44ea64..9c71a9ac21 100644
> > > > --- a/dir.c
> > > > +++ b/dir.c
> > > > @@ -2102,37 +2102,69 @@ static int treat_leading_path(struct dir_struct *dir,
> > > >                             const struct pathspec *pathspec)
> > > >  {
> > > >       struct strbuf sb = STRBUF_INIT;
> > > > -     int baselen, rc = 0;
> > > > +     int prevlen, baselen;
> > > >       const char *cp;
> > > > +     struct cached_dir cdir;
> > > > +     struct dirent de;
> > > > +     enum path_treatment state = path_none;
> > > > +
> > > > +     /*
> > > > +      * For each directory component of path, we are going to check whether
> > > > +      * that path is relevant given the pathspec.  For example, if path is
> > > > +      *    foo/bar/baz/
> > > > +      * then we will ask treat_path() whether we should go into foo, then
> > > > +      * whether we should go into bar, then whether baz is relevant.
> > > > +      * Checking each is important because e.g. if path is
> > > > +      *    .git/info/
> > > > +      * then we need to check .git to know we shouldn't traverse it.
> > > > +      * If the return from treat_path() is:
> > > > +      *    * path_none, for any path, we return false.
> > > > +      *    * path_recurse, for all path components, we return true
> > > > +      *    * <anything else> for some intermediate component, we make sure
> > > > +      *        to add that path to the relevant list but return false
> > > > +      *        signifying that we shouldn't recurse into it.
> > > > +      */
> > > >
> > > >       while (len && path[len - 1] == '/')
> > > >               len--;
> > > >       if (!len)
> > > >               return 1;
> > > > +
> > > > +     memset(&cdir, 0, sizeof(cdir));
> > > > +     memset(&de, 0, sizeof(de));
> > > > +     cdir.de = &de;
> > > > +     de.d_type = DT_DIR;
> > >
> > > So here, `de` is zeroed out, and therefore `de.d_name` is `NULL`.
> >
> > Um, yeah...didn't I have an allocation of de.d_name here?  It will
> > always have a subset of path copied into it, so an allocation of len+1
> > is plenty long enough.
>
> Actually, it looks like I looked up the definition of dirent
> previously and forgot by the time you emailed.  On linux, from
> /usr/include/bits/dirent.h:
>
> struct dirent
>   {
>     ....
>     unsigned char d_type;
>     char d_name[256];           /* We must not include limits.h! */
>   };
>
> and from compat/win32/dirent.h defines it as:
>
> struct dirent {
>         unsigned char d_type;      /* file type to prevent lstat after
> readdir */
>         char d_name[MAX_PATH * 3]; /* file name (* 3 for UTF-8 conversion) */
> };
>
> and 'man dirent' on Mac OS X says it's defined as:
>
> struct dirent {
>         ...
>         _uint8_t d_type;
>         _unit8_t d_namlen;   /* length of string in d_name */
>         char    d_name[255+1];  /* name must be no longer than this */
> }
>
> so, allocating it would be incorrect and my memset would just fill
> d_name with nul characters.
>
>
> But the raises the question...what kind of segfaults are you getting?
> Can you link to any builds or post any stack traces?  Can I duplicate
> with some copy of git-for-windows on linux?

If you care to look at our very own `compat/win32/dirent.h`, you will see
this:

struct dirent {
        unsigned char d_type; /* file type to prevent lstat after readdir */
        char *d_name;         /* file name */
};

And looking at
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/dirent.h.html, I
do not see any guarantee of that `[256]` at all:

The <dirent.h> header shall [...] define the structure dirent which shall
include the following members:

[XSI][Option Start]
ino_t  d_ino       File serial number.
[Option End]
char   d_name[]    Filename string of entry.

You will notice that not even `d_type` is guaranteed.

Ciao,
Dscho