Fix the lstat() emulation on Windows
#1291
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.1291.git.1657872416216.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Eric Sunshine wrote (reply to this): On Fri, Jul 15, 2022 at 4:18 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Network shares sometimes use aggressive caching, in which case a
> just-created directory might not be immediately visible to Git.
>
> One symptom of this scenario is the following error:
>
> $ 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 in all Windows setups. 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/
>
> the following commands worked around it:
>
> 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 culprit is that `GetFileAttributesExW()` that is called from
> `mingw_lstat()` can raise an error `ERROR_PATH_NOT_FOUND`, which is
> translated to `ENOTDIR`, as opposed to `ENOENT` as expected on Linux.
>
> Therefore, when trying to read a ref, let's allow `ENOTDIR` in addition
> to `ENOENT` to indicate that said ref is missing.
>
> This fixes https://github.com/git-for-windows/git/issues/3727
>
> Signed-off-by: Pierre Garnier <pgarnier@mega.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> @@ -381,7 +381,7 @@ stat_ref:
> - if (myerr != ENOENT || skip_packed_refs)
> + if ((myerr != ENOENT && myerr != ENOTDIR) || skip_packed_refs)
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> @@ -480,7 +480,7 @@ static int load_contents(struct snapshot *snapshot)
> - if (errno == ENOENT) {
> + if (errno == ENOENT || errno == ENOTDIR) {
The first question which popped into my mind upon reading the patch
was why these changes need to be made to files-backend.c and
packed-backend.c rather than "fixing" mingw_lstat() to return ENOENT
instead of ENOTDIR. Patching mingw_lstat() seems more tractable and
less likely to lead to discovery of other code in the future which
needs to be patched in a similar way to how files-backend.c and
packed-backend.c are being patched here.
Perhaps it's a silly question and the answer is perfectly obvious to
folks directly involved in Git development on Windows, but the commit
message doesn't seem to answer it for people who don't have such
inside knowledge. |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): On Fri, Jul 15 2022, Johannes Schindelin via GitGitGadget wrote:
> From: Pierre Garnier <pgarnier@mega.com>
>
> Network shares sometimes use aggressive caching, in which case a
> just-created directory might not be immediately visible to Git.
>
> One symptom of this scenario is the following error:
>
> $ 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 in all Windows setups. 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/
>
> the following commands worked around it:
>
> 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 culprit is that `GetFileAttributesExW()` that is called from
> `mingw_lstat()` can raise an error `ERROR_PATH_NOT_FOUND`, which is
> translated to `ENOTDIR`, as opposed to `ENOENT` as expected on Linux.
>
> Therefore, when trying to read a ref, let's allow `ENOTDIR` in addition
> to `ENOENT` to indicate that said ref is missing.
>
> This fixes https://github.com/git-for-windows/git/issues/3727
This really has much wider implications, as we hard depend on POSIX
semantics in various other places. E.g. we'll the SHA-1 collision
detection sanity check (not sha1dc, the "does it exist?") would be racy
on such a system, wouldn't it?
> refs/files-backend.c | 2 +-
> refs/packed-backend.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 8db7882aacb..b2a880f62f0 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -381,7 +381,7 @@ stat_ref:
> if (lstat(path, &st) < 0) {
> int ignore_errno;
> myerr = errno;
> - if (myerr != ENOENT || skip_packed_refs)
> + if ((myerr != ENOENT && myerr != ENOTDIR) || skip_packed_refs)
> goto out;
> if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
> referent, type, &ignore_errno)) {
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 97b68377673..23d478627a7 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -480,7 +480,7 @@ static int load_contents(struct snapshot *snapshot)
>
> fd = open(snapshot->refs->path, O_RDONLY);
> if (fd < 0) {
> - if (errno == ENOENT) {
> + if (errno == ENOENT || errno == ENOTDIR) {
> /*
> * This is OK; it just means that no
> * "packed-refs" file has been written yet,
>
> base-commit: bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca
So I'm skeptical that this can work at all, but in any case wrapping
this non-POSIX hack in an #ifdef for the relevant platform is somtething
I really think we should have here, or "#ifdef NON_POSIX_FS_HACK" or
something.
You don't want to be carefully reviewing this code thinking wtf, only to
discover later that it's impossible on a well-behaved FS.
Also, NFS has similar options (which I've seen hard break git repos &
corrupt them in the past)< how do its various dangerous caching options
behave in these scenarios?
IOW if we're supporting non-POSIX behavior on platform A, are we
inadvertently making the non-POSIX behavior on platform B even worse?
Even more of a reason to wrap it in ifdefs...
But I really think the answer to this is similar to brian's FAQ patches
for git repos on "cloud mounts", I.e. document carefully that it's
likely to corrupt your repo in unexpected ways.
So I'd be much more comfortable with a workaround that stole what we do
for the *.lock spinning here, i.e. we'd detect this errno, say "wtf,
non-POSIX?" then spin for N ms, and hope to get "past the race".
That would be guaranteed not to suffer from odd corruption issues (as
the behavior wouldn't change, we'd just wait and hope to "catch up")>
Wouldn't that be narrower & better here? |
|
User |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Eric,
On Fri, 15 Jul 2022, Eric Sunshine wrote:
> On Fri, Jul 15, 2022 at 4:18 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > Network shares sometimes use aggressive caching, in which case a
> > just-created directory might not be immediately visible to Git.
> >
> > One symptom of this scenario is the following error:
> >
> > $ 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 in all Windows setups. 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/
> >
> > the following commands worked around it:
> >
> > 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 culprit is that `GetFileAttributesExW()` that is called from
> > `mingw_lstat()` can raise an error `ERROR_PATH_NOT_FOUND`, which is
> > translated to `ENOTDIR`, as opposed to `ENOENT` as expected on Linux.
> >
> > Therefore, when trying to read a ref, let's allow `ENOTDIR` in addition
> > to `ENOENT` to indicate that said ref is missing.
> >
> > This fixes https://github.com/git-for-windows/git/issues/3727
> >
> > Signed-off-by: Pierre Garnier <pgarnier@mega.com>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/refs/files-backend.c b/refs/files-backend.c
> > @@ -381,7 +381,7 @@ stat_ref:
> > - if (myerr != ENOENT || skip_packed_refs)
> > + if ((myerr != ENOENT && myerr != ENOTDIR) || skip_packed_refs)
> > diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> > @@ -480,7 +480,7 @@ static int load_contents(struct snapshot *snapshot)
> > - if (errno == ENOENT) {
> > + if (errno == ENOENT || errno == ENOTDIR) {
>
> The first question which popped into my mind upon reading the patch
> was why these changes need to be made to files-backend.c and
> packed-backend.c rather than "fixing" mingw_lstat() to return ENOENT
> instead of ENOTDIR.
I already had started crafting a mail to explain that I do not want to
take the risk of changing the code to map `ERROR_PATH_NOT_FOUND` to
`ENOENT` instead of `ENOTDIR`, as we only have one central function to map
Windows' error codes to POSIX `errno` values. It would therefore affect
many more code paths than just `mingw_lstat()`.
Can you imagine my surprise when I looked up the link to that mapping
function so that I could paste it in my reply to make my point, only to
see that that error is _already_ mapped to `ENOENT`? Look here for
yourself: https://github.com/git/git/blob/v2.37.1/compat/mingw.c#L121
So where is the bug? It is somewhere completely different:
https://github.com/git/git/blob/v2.37.1/compat/mingw.c#L847-L851
Essentially, Windows does not give us the equivalent of POSIX' `ENOTDIR`:
For something like `C:\a\b\c` we only get `ERROR_PATH_NOT_FOUND`, no
matter whether `C:\a` is missing or whether it is a file (POSIX would want
an `ENOENT` in the former and `ENOTDIR` in the latter case, and
unfortunately Git fully relies on these POSIX semantics).
In 4b0abd5c695 (mingw: let lstat() fail with errno == ENOTDIR when
appropriate, 2016-01-26), we introduced code to emulate POSIX semantics:
we laboriously look through the path components to see whether there is
anything in the way that would prevent us from creating the parent
directory.
It must be somewhere in that emulation where things go wrong, still. I
asked Pierre to debug a bit more to get to the bottom of the problem.
So: thank you for your "silly" question, it did point to the need for more
investigation.
Ciao,
Dscho |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Eric Sunshine <sunshine@sunshineco.com> writes:
>> The underlying culprit is that `GetFileAttributesExW()` that is called from
>> `mingw_lstat()` can raise an error `ERROR_PATH_NOT_FOUND`, which is
>> translated to `ENOTDIR`, as opposed to `ENOENT` as expected on Linux.
>> ...
>> @@ -480,7 +480,7 @@ static int load_contents(struct snapshot *snapshot)
>> - if (errno == ENOENT) {
>> + if (errno == ENOENT || errno == ENOTDIR) {
>
> The first question which popped into my mind upon reading the patch
> was why these changes need to be made to files-backend.c and
> packed-backend.c rather than "fixing" mingw_lstat() to return ENOENT
> instead of ENOTDIR. Patching mingw_lstat() seems more tractable and
> less likely to lead to discovery of other code in the future which
> needs to be patched in a similar way to how files-backend.c and
> packed-backend.c are being patched here.
>
> Perhaps it's a silly question and the answer is perfectly obvious to
> folks directly involved in Git development on Windows, but the commit
> message doesn't seem to answer it for people who don't have such
> inside knowledge.
FWIW, I had the same reaction. ENOTDIR does not mean an attempt to
access "test_dir/test_tag" failed because "test_dir" did not
exist---it means "test_dir" exists as something that is not a
directory (hence there is no way "test_dir/test_file" to exist).
In the example scenario in the proposed log message, a new tag
"test_dir/test_tag" is created, and (even though the proposed log
message does not explicitly say so) presumably, "test_dir" needs to
be created while doing so. A failure to access "test_dir/test_file"
due to lack of "test_dir" shouldn't report ENOTDIR but should report
ENOENT. So something is fishy.
Unless the internal implementation of the filesystem creates a
placeholder that is not a directory at "test_dir", returns the
control to the application, and does further work to turn that
placeholder object into a directory in the background, and the
application attempts to create "test_dir/test_tag" in the meantime,
racing with the filesystem, or something silly like that?
It sounds like a platform specific bug that is not specific to the
ref-files subsystem. If it can be fixed at lstat() emulation, that
would benefit other checks for ENOENT.
Having said all that.
Stepping back a bit, one situation where we would want to special
case ENOENT is when we have an optional file at known location and
it is OK for the file to be missing. We may attempt to read from
"$XDG_CONFIG_HOME/git/config" and it may fail due to ENOENT or
ENOTDIR because the user may not be using XDG config location at all
(hence $XDG_CONFIG_HOME or XDG_CONFIG_HOME/git may be missing) or
the leading directories may be there but not the file at the leaf
level. In such a use case, we should ignore ENOTDIR just like we
ignore ENOENT as an error that does not matter.
In any case, the posted patch may hide a repository corruption from
the codepath affected and cause it to silently return a bogus
answer. The first hunk touches read_ref_internal() where "path"
variable contains the path we expect to find the on-disk loose ref
file
if (lstat(path, &st) < 0) {
int ignore_errno;
myerr = errno;
if (myerr != ENOENT || skip_packed_refs)
goto out;
if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
referent, type, &ignore_errno)) {
myerr = ENOENT;
goto out;
}
ret = 0;
goto out;
}
The idea is that a ref does not have to exist as a loose ref file,
so an error from lstat() is not immediately an error. If the ref
were previously packed, then we should fall back to read it from the
packed-ref file.
So we say that if the error is *NOT* ENOENT, we jump to 'out' label
to report the failed lstat() as an error". Otherwise we proceed to
attempt to read from the packed-ref file. Is there any case where
an attempt to read from a loose ref fails with ENOTDIR and it is OK
that we can proceed without reading from the packed-refs file?
If we have a branch "js/foo" that is packed, and then if we removed
it, and then created a branch "js", the original "js/foo" should not
be in the packed-refs file, but supposed a repository is corrupt and
"js/foo" remains in the packed-refs file. Now imagine that we ask
for "js/foo". What happens?
We fail to lstat ".git/refs/heads/js/foo" due to ENOTDIR, because
the "js" branch exists loose at ".git/refs/heads/js". In the
original, because ENOTDIR is not ENOENT, we jumped to "out" to
report the error. The patched code to allow ENOTDIR will instead
happily read the stale version of "js/foo" out of the packed-refs
file.
|
dscho
force-pushed
the
enotdir-and-enoent-can-indicate-missing-refs
branch
2 times, most recently
from
5b5fb2d
to
de54661
Compare
dscho
force-pushed
the
enotdir-and-enoent-can-indicate-missing-refs
branch
from
de54661
to
06120fb
Compare
dscho
changed the title
lstat() emulation on Windows
dscho
force-pushed
the
enotdir-and-enoent-can-indicate-missing-refs
branch
from
06120fb
to
2ebe899
Compare
|
/submit |
|
Submitted as pull.1291.v2.git.1659018558989.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
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.
|
|
This branch is now known as |
|
This patch series was integrated into seen via git@50e48bf. |
|
On the Git mailing list, Eric Sunshine wrote (reply to this): On Thu, Jul 28, 2022 at 10:29 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> 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 <pgarnier@mega.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> |
|
This patch series was integrated into seen via git@b4f1c0b. |
dscho
force-pushed
the
enotdir-and-enoent-can-indicate-missing-refs
branch
from
2ebe899
to
c0fc33d
Compare
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 led 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 4b0abd5 (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 `GetFileAttributesW()` call correctly. This fixes git-for-windows#3727 Reported-by: Pierre Garnier <pgarnier@mega.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
enotdir-and-enoent-can-indicate-missing-refs
branch
from
c0fc33d
to
b4f08ee
Compare
|
/submit |
|
Submitted as pull.1291.v3.git.1659089152877.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@51c139a. |
|
There was a status update in the "New Topics" section about the branch Fix to lstat() emulation on Windows. Will merge to 'next'. source: <pull.1291.v3.git.1659089152877.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@f02082d. |
|
This patch series was integrated into next via git@1049994. |
|
There was a status update in the "Cooking" section about the branch Fix to lstat() emulation on Windows. Will merge to 'master'. source: <pull.1291.v3.git.1659089152877.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@ed9105a. |
|
This patch series was integrated into seen via git@0cec0d8. |
|
There was a status update in the "Cooking" section about the branch Fix to lstat() emulation on Windows. Will merge to 'master'. source: <pull.1291.v3.git.1659089152877.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@7b640ab. |
|
This patch series was integrated into seen via git@6c5fbd8. |
|
This patch series was integrated into master via git@6c5fbd8. |
|
This patch series was integrated into next via git@6c5fbd8. |
|
Closed via 6c5fbd8. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One particular code path in the
lstat()emulation on Windows was broken.This fixes git-for-windows#3727
Changes since v2:
Changes since v1:
cc: Eric Sunshine sunshine@sunshineco.com