[v2] lstat(mingw): correctly detect ENOTDIR scenarios

Commit Message

Johannes Schindelin July 28, 2022, 2:29 p.m. UTC

From: Johannes Schindelin <[e-mail]>

Files' attributes can indicate more than just whether they are files or
directories. It was reported in Git for Windows that on certain network
shares, this let to a nasty problem trying to create tags:

	$ git tag -a -m "automatic tag creation"  test_dir/test_tag
	fatal: cannot lock ref 'refs/tags/test_dir/test_tag': unable to resolve reference 'refs/tags/test_dir/test_tag': Not a directory

Note: This does not necessarily happen with all types of network shares.
One setup where it _did_ happen is a Windows Server 2019 VM, and as
hinted in

	http://woshub.com/slow-network-shared-folder-refresh-windows-server/

in the indicated instance the following commands worked around the bug:

	Set-SmbClientConfiguration -DirectoryCacheLifetime 0
	Set-SmbClientConfiguration -FileInfoCacheLifetime 0
	Set-SmbClientConfiguration -FileNotFoundCacheLifetime 0

This would impact performance negatively, though, as it essentially
turns off all caching, therefore we do not want to require users to do
that just to be able to use Git on Windows.

The underlying bug is in the code added in 4b0abd5c695 (mingw: let
lstat() fail with errno == ENOTDIR when appropriate, 2016-01-26) that
emulates the POSIX behavior where `lstat()` should return `ENOENT` if
the file or directory simply does not exist but could be created, and
`ENOTDIR` if there is no file or directory nor could there be because a
leading path already exists and is not a directory.

In that code, the return value of `GetFileAttributesW()` is interpreted
as an enum value, not as a bit field, so that a perfectly fine leading
directory can be misdetected as "not a directory".

As a consequence, the `read_refs_internal()` function would return
`ENOTDIR`, suggesting not only that the tag in the `git tag` invocation
above does not exist, but that it cannot even be created.

Let's fix the code so that it interprets the return value of the
`GetFileAtrtibutesW()` call correctly.

This fixes https://github.com/git-for-windows/git/issues/3727

Reported-by: Pierre Garnier <[e-mail]>
Signed-off-by: Johannes Schindelin <[e-mail]>
---
    Fix the lstat() emulation on Windows
    
    One particular code path in the lstat() emulation on Windows was broken.
    
    This fixes https://github.com/git-for-windows/git/issues/3727
    
    Changes since v1:
    
     * Thanks to Eric's excellent review, the reporter and I dug deeper and
       figured out the real bug (and fix).

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1291%2Fdscho%2Fenotdir-and-enoent-can-indicate-missing-refs-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1291/dscho/enotdir-and-enoent-can-indicate-missing-refs-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1291

Range-diff vs v1:

 1:  c3d51a755ba < -:  ----------- refs: work around network caching on Windows
 -:  ----------- > 1:  2ebe899736e lstat(mingw): correctly detect ENOTDIR scenarios


 compat/mingw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


base-commit: 4b0abd5c695c87bf600e57b6a5c7d6844707d34c

Comments

Junio C Hamano July 28, 2022, 5:37 p.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> In that code, the return value of `GetFileAttributesW()` is interpreted
> as an enum value, not as a bit field, so that a perfectly fine leading
> directory can be misdetected as "not a directory".

Nicely analyzed.  So after all ENOTDIR was a buggy response and by
fixing it we help all callers of lstat(), as pointed out in the
earlier review.

>      * Thanks to Eric's excellent review, the reporter and I dug deeper and
>        figured out the real bug (and fix).

Yeah, thanks all.

Will queue.

Eric Sunshine July 28, 2022, 11:27 p.m. UTC | #2

On Thu, Jul 28, 2022 at 10:29 AM Johannes Schindelin via GitGitGadget
<[e-mail]> wrote:
> Files' attributes can indicate more than just whether they are files or
> directories. It was reported in Git for Windows that on certain network
> shares, this let to a nasty problem trying to create tags:

s/let/led/

(not worth a re-roll)

>         $ git tag -a -m "automatic tag creation"  test_dir/test_tag
>         fatal: cannot lock ref 'refs/tags/test_dir/test_tag': unable to resolve reference 'refs/tags/test_dir/test_tag': Not a directory
>
> Note: This does not necessarily happen with all types of network shares.
> One setup where it _did_ happen is a Windows Server 2019 VM, and as
> hinted in
>
>         http://woshub.com/slow-network-shared-folder-refresh-windows-server/
>
> in the indicated instance the following commands worked around the bug:
>
>         Set-SmbClientConfiguration -DirectoryCacheLifetime 0
>         Set-SmbClientConfiguration -FileInfoCacheLifetime 0
>         Set-SmbClientConfiguration -FileNotFoundCacheLifetime 0
>
> This would impact performance negatively, though, as it essentially
> turns off all caching, therefore we do not want to require users to do
> that just to be able to use Git on Windows.
>
> The underlying bug is in the code added in 4b0abd5c695 (mingw: let
> lstat() fail with errno == ENOTDIR when appropriate, 2016-01-26) that
> emulates the POSIX behavior where `lstat()` should return `ENOENT` if
> the file or directory simply does not exist but could be created, and
> `ENOTDIR` if there is no file or directory nor could there be because a
> leading path already exists and is not a directory.
>
> In that code, the return value of `GetFileAttributesW()` is interpreted
> as an enum value, not as a bit field, so that a perfectly fine leading
> directory can be misdetected as "not a directory".
>
> As a consequence, the `read_refs_internal()` function would return
> `ENOTDIR`, suggesting not only that the tag in the `git tag` invocation
> above does not exist, but that it cannot even be created.
>
> Let's fix the code so that it interprets the return value of the
> `GetFileAtrtibutesW()` call correctly.
>
> This fixes https://github.com/git-for-windows/git/issues/3727
>
> Reported-by: Pierre Garnier <[e-mail]>
> Signed-off-by: Johannes Schindelin <[e-mail]>

Patch

diff --git a/compat/mingw.c b/compat/mingw.c
index 545e952a588..3b85bb02536 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -471,8 +471,8 @@  static int has_valid_directory_prefix(wchar_t *wfilename)
 		wfilename[n] = L'\0';
 		attributes = GetFileAttributesW(wfilename);
 		wfilename[n] = c;
-		if (attributes == FILE_ATTRIBUTE_DIRECTORY ||
-				attributes == FILE_ATTRIBUTE_DEVICE)
+		if (attributes &
+		    (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_DEVICE))
 			return 1;
 		if (attributes == INVALID_FILE_ATTRIBUTES)
 			switch (GetLastError()) {