mingw: lstat: compute correct size for symlinks #2637

billziss-gh · 2020-05-27T07:44:36Z

This PR includes a single commit that fixes the mingw_lstat function to return correct size for symlinks on Windows. Prior to this commit the function returned a fixed size of 4096, which caused problems for repositories that were accessed using git from different platforms.

For example, if using both git for WSL and git for Windows the problem that I describe below could arise.

Consider a git repository accessed using git for WSL:

Now consider the same repository accessed using git for Windows:

Notice that RelNotes, which is a symbolic link, appears modified. This happened because mingw_lstat returned st_size == 4096, which then git for Windows compared to the index prepared by git for WSL and found a size mismatch.

Finally, using the commit in this PR, a patched git-status.exe reports correctly that there are no changes found.

dscho · 2020-05-27T12:13:23Z

compat/mingw.c

+	int len;
+
+	/* read reparse point data */
+	handle = CreateFileW(wpath, 0,


In our performance tests, CreateFile() was substantially slower than FindFirstFile(). This might not sound like a big thing, but when you have tens of thousands of files (or even several million files), it does matter.

Noticing that symbolic links are relatively rare, would it not make more sense to replace the MAX_LONG_PATH constant by a function that determines the length of a symbolic link (and is called only for symbolic links)?

And while we're already at that point where we want to read the length of the symlink, would it not make more sense to use the readlink() function rather than duplicating (part of) its logic?

Thank you for the quick review.

In our performance tests, CreateFile() was substantially slower than FindFirstFile().

The FindFirstFileW, FindClose sequence makes the following calls:

NtOpenFile / IoCreateFile on the parent directory of the supplied file path.

NtQueryDirectoryFile with the requested file name or mask of the supplied file path.

NtClose

The CreateFileW, DeviceIoControl, CloseHandle sequence makes the following calls:

NtCreateFile / IoCreateFile on the supplied file path.

NtFsControlFile with FSCTL_GET_REPARSE_POINT.

NtClose.

So if you have observed performance differences it is perhaps because when accessing multiple files in the same directory the file system has a better chance to cache directory contents in the FindFirstFileW sequence.

Even more importantly for our case, FindFirstFileW does not give us enough information to fill in the symlink stat. It gives us the reparse tag, but does not give us the length of the symlink. So querying the reparse point for its data and computing the length in that way becomes unavoidable.

Noticing that symbolic links are relatively rare, would it not make more sense to replace the MAX_LONG_PATH constant by a function that determines the length of a symbolic link (and is called only for symbolic links)?

I agree. But please note that this happens already: the expensive reparse point computation happens only when the if (fdata.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) condition is true.

And while we're already at that point where we want to read the length of the symlink, would it not make more sense to use the readlink() function rather than duplicating (part of) its logic?

I agree and that is how I implemented it at first. Unfortunately readlink does not return the actual reparse tag that is used in the computation of st_mode by file_attr_to_st_mode. Without that special wreadlink_len function we would have to issue both a FindFirstFileW and a readlink to properly stat a symlink.

BTW, one improvement in the submitted commit is the following:

Currently the wreadlink_len returns -1 it if encounters a reparse point that is not a symbolic link or a mount point; this causes mingw_lstat to also return -1. Perhaps such cases should be handled more gracefully (e.g. by returning the prior size of 4096 for compatibility purposes).

Without that special wreadlink_len function we would have to issue both a FindFirstFileW and a readlink to properly stat a symlink.

Well, in the current state, there is a lot of duplicated code.

The common strategy in Git's commit history is to refactor out a helper function that can be used from multiple call sites. In this case, it would be factoring out that part of readlink() that determines e.g. wbuf and len, then calling that from readlink() and from mingw_lstat().

That way, code is not duplicated, and as a consequence there is not even a possibility for one of the two copies of the code to become stale, as there is only a single copy of the code to begin with.

Understood. I will look into making the necessary changes and resubmit.

billziss-gh · 2020-05-27T20:01:34Z

I have made a couple of small updates in the commit:

I renamed the internal function wreadlink_len to get_reparse_point_info to disambiguate it from readlink.
The function returns the correct length of a symbolic link or mount point as before, but in addition it returns MAX_LONG_PATH for other reparse points (to be compatible with how git for Windows currently handles such reparse points).

Finally this commit has been signed off.

dscho · 2020-05-27T21:02:43Z

compat/mingw.c

 int mingw_lstat(const char *file_name, struct stat *buf)
 {
 	WIN32_FILE_ATTRIBUTE_DATA fdata;
-	WIN32_FIND_DATAW findbuf = { 0 };
+	DWORD reparse_tag;


You will need to initialize reparse_tag so that it can be passed to file_attr_to_st_mode() even if the path is not a reparse point.

Argh! You are correct of course.

:-) No worries. I always hope that I can help improve the patches through my reviews.

Thanks. Somehow the compiler did not catch this, but I should have been more careful.

billziss-gh · 2020-05-27T22:20:04Z

I have refactored the code as per your request:

New get_reparse_point function can be used to get a reparse point.
New get_reparse_point_link_len is used to get the reparse tag and link length for mingw_lstat (previously called wreadlink_len and later get_reparse_point_info).
The readlink function now uses get_reparse_point.

dscho · 2020-05-28T11:39:30Z

compat/mingw.c

+		return MAX_LONG_PATH; /* return value for compatibility */
+	}
+
+	len = WideCharToMultiByte(CP_UTF8, 0, normalize_ntpath(wbuf), -1, 0, 0, NULL, NULL);


I would prefer this conversion from wbuf to UTF-8 to be identical to the readlink() case. Otherwise, it will be too easy to change one and forget the other, and then we end up with inconsistent lengths again.

Granted, readlink() uses

if ((len = xwcstoutf(tmpbuf, normalize_ntpath(wbuf), MAX_LONG_PATH)) < 0)

which we do not use here because we do not really want to convert wbuf to UTF-8 at this point. But then, maybe we just shouldn't care so much and use a temporary buffer in get_reparse_point_link_len() that is simply ignored?

If we do that, we can refactor pretty much everything in readlink() (except the initial xutftowcs_long_path() call) into a new function int readlink_1(wchar_t *wpath, char *buf, size_t bufsiz, size_t *plen, DWORD *ptag). And then we do not even need a wrapper around that when calling it in mingw_lstat(), and readlink() itself will be a very minimal wrapper that only converts path to wpath and then calls return readlink_1(...);.

In other words, I would prefer this fixup squashed in:

diff --git a/compat/mingw.c b/compat/mingw.c index 92db342833c8..87f985c6e7e9 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -969,7 +969,8 @@ static int has_valid_directory_prefix(wchar_t *wfilename) return 1; } -static int get_reparse_point_link_len(const WCHAR *wpath, DWORD *ptag); +static int readlink_1(const WCHAR *wpath, char *tmpbuf, int *plen, DWORD *ptag); + int mingw_lstat(const char *file_name, struct stat *buf) { WIN32_FILE_ATTRIBUTE_DATA fdata; @@ -989,10 +990,12 @@ int mingw_lstat(const char *file_name, struct stat *buf) } if (GetFileAttributesExW(wfilename, GetFileExInfoStandard, &fdata)) { - /* for reparse points, use get_reparse_point_link_len to get the link tag and length */ + /* for reparse points, get the link tag and length */ if (fdata.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) { - link_len = get_reparse_point_link_len(wfilename, &reparse_tag); - if (link_len < 0) + char tmpbuf[MAX_LONG_PATH]; + + if (readlink_1(wfilename, tmpbuf, &link_len, + &reparse_tag) < 0) return -1; } buf->st_ino = 0; @@ -2969,10 +2972,11 @@ typedef struct _REPARSE_DATA_BUFFER { } REPARSE_DATA_BUFFER, *PREPARSE_DATA_BUFFER; #endif -static int get_reparse_point(const WCHAR *wpath, REPARSE_DATA_BUFFER *b, size_t siz, WCHAR **pwbuf) +static int readlink_1(const WCHAR *wpath, char *tmpbuf, int *plen, DWORD *ptag) { HANDLE handle; WCHAR *wbuf; + REPARSE_DATA_BUFFER *b = alloca(MAXIMUM_REPARSE_DATA_BUFFER_SIZE); DWORD dummy; /* read reparse point data */ @@ -2985,7 +2989,7 @@ static int get_reparse_point(const WCHAR *wpath, REPARSE_DATA_BUFFER *b, size_t return -1; } if (!DeviceIoControl(handle, FSCTL_GET_REPARSE_POINT, NULL, 0, b, - siz, &dummy, NULL)) { + MAXIMUM_REPARSE_DATA_BUFFER_SIZE, &dummy, NULL)) { errno = err_win_to_posix(GetLastError()); CloseHandle(handle); return -1; @@ -2993,7 +2997,7 @@ static int get_reparse_point(const WCHAR *wpath, REPARSE_DATA_BUFFER *b, size_t CloseHandle(handle); /* get target path for symlinks or mount points (aka 'junctions') */ - switch (b->ReparseTag) { + switch ((*ptag = b->ReparseTag)) { case IO_REPARSE_TAG_SYMLINK: wbuf = (WCHAR*) (((char*) b->SymbolicLinkReparseBuffer.PathBuffer) + b->SymbolicLinkReparseBuffer.SubstituteNameOffset); @@ -3007,52 +3011,28 @@ static int get_reparse_point(const WCHAR *wpath, REPARSE_DATA_BUFFER *b, size_t + b->MountPointReparseBuffer.SubstituteNameLength) = 0; break; default: - wbuf = NULL; - break; - } - - *pwbuf = wbuf; - return 0; -} - -static int get_reparse_point_link_len(const WCHAR *wpath, DWORD *ptag) -{ - WCHAR *wbuf; - REPARSE_DATA_BUFFER *b = alloca(MAXIMUM_REPARSE_DATA_BUFFER_SIZE); - int len; - - if (get_reparse_point(wpath, b, MAXIMUM_REPARSE_DATA_BUFFER_SIZE, &wbuf) < 0) + errno = EINVAL; return -1; - if (wbuf == NULL) { - *ptag = b->ReparseTag; - return MAX_LONG_PATH; /* return value for compatibility */ } - len = WideCharToMultiByte(CP_UTF8, 0, normalize_ntpath(wbuf), -1, 0, 0, NULL, NULL); - if (len) { - *ptag = b->ReparseTag; - return len - 1; - } - errno = ERANGE; - return -1; + if ((*plen = + xwcstoutf(tmpbuf, normalize_ntpath(wbuf), MAX_LONG_PATH)) < 0) + return -1; + return 0; } int readlink(const char *path, char *buf, size_t bufsiz) { - WCHAR wpath[MAX_LONG_PATH], *wbuf; - REPARSE_DATA_BUFFER *b = alloca(MAXIMUM_REPARSE_DATA_BUFFER_SIZE); + WCHAR wpath[MAX_LONG_PATH]; char tmpbuf[MAX_LONG_PATH]; int len; + DWORD tag; if (xutftowcs_long_path(wpath, path) < 0) return -1; - if (get_reparse_point(wpath, b, MAXIMUM_REPARSE_DATA_BUFFER_SIZE, &wbuf) < 0) - return -1; - if (wbuf == NULL) { - errno = EINVAL; + if (readlink_1(wpath, tmpbuf, &len, &tag) < 0) return -1; - } /* * Adapt to strange readlink() API: Copy up to bufsiz *bytes*, potentially @@ -3061,8 +3041,6 @@ int readlink(const char *path, char *buf, size_t bufsiz) * so convert to a (hopefully large enough) temporary buffer, then memcpy * the requested number of bytes (including '\0' for robustness). */ - if ((len = xwcstoutf(tmpbuf, normalize_ntpath(wbuf), MAX_LONG_PATH)) < 0) - return -1; memcpy(buf, tmpbuf, min(bufsiz, len + 1)); return min(bufsiz, len); }

I think this will work in the general case. However please note that with the fixup, lstat will fail with EINVAL in the case of a reparse point that is neither a symlink nor a mountpoint. (That was the primary reason that I refactored into the separate get_reparse_point in my earlier commit.)

I any case I am happy to include this fixup and resubmit. I will apply your fixup as a patch on top of my commit and attribute the changes in the commit message.

However please note that with the fixup, lstat will fail with EINVAL in the case of a reparse point that is neither a symlink nor a mountpoint. (That was the primary reason that I refactored into the separate get_reparse_point in my earlier commit.)

What could it be, then?

If you care very deeply about this, you can very easily add a parameter to readlink_1(), say, fail_on_unknown_tag, and only do the EINVAL thing if that parameter is non-zero, otherwise do the MAX_LONG_PATH thing.

Proposed fixup for git-for-windows#2637 Signed-off-by: Johannes Schindelin <[email protected]>

This commit fixes mingw_lstat by computing the proper size for symlinks according to POSIX. POSIX specifies that upon successful return from lstat: "the value of the st_size member shall be set to the length of the pathname contained in the symbolic link not including any terminating null byte". Prior to this commit the mingw_lstat function returned a fixed size of 4096. This caused problems in git repositories that were accessed by git for Cygwin or git for WSL. For example, doing `git reset --hard` using git for Windows would update the size of symlinks in the index to be 4096; at a later time git for Cygwin or git for WSL would find that symlinks have changed size during `git status`. Vice versa doing `git reset --hard` in git for Cygwin or git for WSL would update the size of symlinks in the index with the correct value, only for git for Windows to find incorrectly at a later time that the size had changed. Additional fixup by: Johannes Schindelin <[email protected]> Signed-off-by: Bill Zissimopoulos <[email protected]>

billziss-gh · 2020-05-28T23:42:27Z

I have pushed a new commit with your fixup applied. I have also added the fail_on_unknown_tag change.

dscho

Awesome!

In general, I do not accept new changes that late in the game (Git v2.27.0 is due Monday, with Git for Windows v2.27.0 to follow very soon thereafter), but you've been nothing but responsive and I think the patch is all the better for it.

Thank you so much for a very pleasant contribution!

mingw: lstat: compute correct size for symlinks

Git for Windows and WSL Git [now have the same idea of symbolic link's length](git-for-windows/git#2637), i.e. `git status` will no longer mark them as modified in Git for Windows after checking them out in WSL. Signed-off-by: Johannes Schindelin <[email protected]>

It is an unfortunate consequence of git-for-windows/git#2637 that tracked symbolic links will be shown as modified by `git status` but not by `git diff` when the Git index had been refreshed last with a Git for Windows version prior to v2.27.0. The good news is that this will be a one-time only cost. Signed-off-by: Johannes Schindelin <[email protected]>

billziss-gh · 2020-05-29T15:59:29Z

@dscho glad to be of help and thank you for merging this in.

mingw: lstat: compute correct size for symlinks

billziss-gh · 2020-06-01T23:11:10Z

I just updated to the latest git-for-windows and I am still having the original problem with symbolic links. Perhaps the change in the PR was inadvertently left out?

Git-for-windows repo cloned in WSL:

$ uname -a
Linux xps 4.4.0-19041-Microsoft #1-Microsoft Fri Dec 06 14:06:00 PST 2019 x86_64 x86_64 x86_64 GNU/Linux
$ git clone https://github.com/git-for-windows/git.git
Cloning into 'git'...
remote: Enumerating objects: 441232, done.
remote: Total 441232 (delta 0), reused 0 (delta 0), pack-reused 441232
Receiving objects: 100% (441232/441232), 176.09 MiB | 9.64 MiB/s, done.
Resolving deltas: 100% (319794/319794), done.
Updating files: 100% (3787/3787), done.
$ cd git
$ git status
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean

Latest status on same repo from Powershell using git-for-windows 2.27.0.windows.1:

> git --version
git version 2.27.0.windows.1
> git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   RelNotes

no changes added to commit (use "git add" and/or "git commit -a")

In #2637, we fixed a bug where symbolic links' target path sizes were recorded incorrectly in the index. The downside of this fix was that every user with tracked symbolic links in their checkouts would see them as modified in `git status`, but not in `git diff`, and only a `git add <path>` (or `git add -u`) would "fix" this. Let's do better than that: we can detect that situation and simply pretend that a symbolic link with a known bad size (or a size that just happens to be that bad size, a _very_ unlikely scenario because it would overflow our buffers due to the trailing NUL byte) means that it needs to be re-checked as if we had just checked it out. Signed-off-by: Johannes Schindelin <[email protected]>

In git-for-windows#2637, we fixed a bug where symbolic links' target path sizes were recorded incorrectly in the index. The downside of this fix was that every user with tracked symbolic links in their checkouts would see them as modified in `git status`, but not in `git diff`, and only a `git add <path>` (or `git add -u`) would "fix" this. Let's do better than that: we can detect that situation and simply pretend that a symbolic link with a known bad size (or a size that just happens to be that bad size, a _very_ unlikely scenario because it would overflow our buffers due to the trailing NUL byte) means that it needs to be re-checked as if we had just checked it out. Signed-off-by: Johannes Schindelin <[email protected]>

In #2637, we fixed a bug where symbolic links' target path sizes were recorded incorrectly in the index. The downside of this fix was that every user with tracked symbolic links in their checkouts would see them as modified in `git status`, but not in `git diff`, and only a `git add <path>` (or `git add -u`) would "fix" this. Let's do better than that: we can detect that situation and simply pretend that a symbolic link with a known bad size (or a size that just happens to be that bad size, a _very_ unlikely scenario because it would overflow our buffers due to the trailing NUL byte) means that it needs to be re-checked as if we had just checked it out. Signed-off-by: Johannes Schindelin <[email protected]>

dscho reviewed May 27, 2020

View reviewed changes

dscho reviewed May 28, 2020

View reviewed changes

dscho added a commit to dscho/git that referenced this pull request May 28, 2020

fixup! mingw: lstat: compute correct size for symlinks

a2e6991

Proposed fixup for git-for-windows#2637 Signed-off-by: Johannes Schindelin <[email protected]>

dscho approved these changes May 29, 2020

View reviewed changes

dscho merged commit 2dbd943 into git-for-windows:master May 29, 2020

git-for-windows-ci pushed a commit that referenced this pull request May 29, 2020

Merge pull request #2637 from billziss-gh/master

f2b8dce

mingw: lstat: compute correct size for symlinks

git-for-windows-ci pushed a commit that referenced this pull request May 29, 2020

Merge pull request #2637 from billziss-gh/master

b0960e1

mingw: lstat: compute correct size for symlinks

git-for-windows-ci pushed a commit that referenced this pull request May 29, 2020

Merge pull request #2637 from billziss-gh/master

4332802

mingw: lstat: compute correct size for symlinks

git-for-windows-ci pushed a commit that referenced this pull request May 29, 2020

Merge pull request #2637 from billziss-gh/master

25c876b

mingw: lstat: compute correct size for symlinks

dscho added this to the Next release milestone May 29, 2020

dscho added a commit that referenced this pull request May 31, 2020

Merge pull request #2637 from billziss-gh/master

1c380ec

mingw: lstat: compute correct size for symlinks

git-for-windows-ci pushed a commit that referenced this pull request Jun 1, 2020

Merge pull request #2637 from billziss-gh/master

0690faa

mingw: lstat: compute correct size for symlinks

git-for-windows-ci pushed a commit that referenced this pull request Jun 1, 2020

Merge pull request #2637 from billziss-gh/master

6a2bf58

mingw: lstat: compute correct size for symlinks

git-for-windows-ci pushed a commit that referenced this pull request Jun 1, 2020

Merge pull request #2637 from billziss-gh/master

6df1f5c

mingw: lstat: compute correct size for symlinks

dscho added a commit to dscho/git that referenced this pull request Jun 1, 2020

Merge pull request git-for-windows#2637 from billziss-gh/master

4fc9e19

mingw: lstat: compute correct size for symlinks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mingw: lstat: compute correct size for symlinks #2637

mingw: lstat: compute correct size for symlinks #2637

billziss-gh commented May 27, 2020

dscho May 27, 2020

dscho May 27, 2020

billziss-gh May 27, 2020 •

edited

Loading

billziss-gh May 27, 2020

dscho May 27, 2020

billziss-gh May 27, 2020

dscho May 27, 2020

billziss-gh commented May 27, 2020 •

edited

Loading

dscho May 27, 2020

billziss-gh May 27, 2020

dscho May 27, 2020

billziss-gh May 27, 2020

billziss-gh commented May 27, 2020 •

edited

Loading

dscho May 28, 2020

dscho May 28, 2020

billziss-gh May 28, 2020

dscho May 28, 2020

billziss-gh commented May 28, 2020

dscho left a comment

billziss-gh commented May 29, 2020

billziss-gh commented Jun 1, 2020

mingw: lstat: compute correct size for symlinks #2637

mingw: lstat: compute correct size for symlinks #2637

Conversation

billziss-gh commented May 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billziss-gh May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billziss-gh commented May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billziss-gh commented May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billziss-gh commented May 28, 2020

dscho left a comment

Choose a reason for hiding this comment

billziss-gh commented May 29, 2020

billziss-gh commented Jun 1, 2020

billziss-gh May 27, 2020 •

edited

Loading

billziss-gh commented May 27, 2020 •

edited

Loading

billziss-gh commented May 27, 2020 •

edited

Loading