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

On Mon, Dec 16, 2019 at 5:51 AM Elijah Newren [off-list ref] wrote:

On Sun, Dec 15, 2019 at 2:29 AM Johannes Schindelin
[off-list ref] wrote:

quoted

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?

quoted

On Tue, 10 Dec 2019, Elijah Newren via GitGitGadget wrote:

quoted

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?