Permission issue in Git in DrvFs-mounted network drives

Permission issue in Git in DrvFs-mounted network drives

[Marcos Del Sol Vives]

Under WSL1 (Windows Subsystem for Linux), when using a network share
mounted via DrvFs, Git fails to add any files to a new or an existing
repository.

The reason is that Git tries to open a temporary file as with RW
permissions but mode 0444, which causes WSL1 (or Samba, unsure who's here
to blame) to create first an file empty with the read-only DOS attribute set
that prevents any writes, and then actually trying to opening it in write
mode, which of course fails.

Seems to be a pretty common issue that nobody has yet reported officially,
judging by the amount of posts on Stackoverflow, impacting not only WSL
but also CIFS under Linux (hence why sending to this mailing list and not
the Windows-specific one):

 - https://superuser.com/questions/681196/debugging-git-repo-permissions-on-samba-share
 - https://superuser.com/questions/1450094/git-on-wsl-commands-fail-despite-permissions-seeming-fine
 - https://superuser.com/questions/1491499/use-git-on-a-shared-drive-within-wsl

As a workaround, opening the file with permissions 0600 and then using a
fchmod with the final desired mode works, which is a very small change that
should cause no issues under neither real Linux nor WSL:

--- git-2.39.5.orig/wrapper.c
+++ git-2.39.5/wrapper.c
@@ -484,9 +484,11 @@ int git_mkstemps_mode(char *pattern, int
 			v /= num_letters;
 		}
 
-		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
-		if (fd >= 0)
+		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);
+		if (fd >= 0) {
+			fchmod(fd, mode);
 			return fd;
+		}
 		/*
 		 * Fatal error (EPERM, ENOSPC etc).
 		 * It doesn't make sense to loop.

The WSL team at Microsoft has been already informed as well:
https://github.com/microsoft/WSL/issues/12051

RE: Permission issue in Git in DrvFs-mounted network drives

[rsbecker]

On September 18, 2024 12:16 PM, Marcos Del Sol Vives wrote:
>Under WSL1 (Windows Subsystem for Linux), when using a network share
>mounted via DrvFs, Git fails to add any files to a new or an existing repository.
>
>The reason is that Git tries to open a temporary file as with RW permissions but
>mode 0444, which causes WSL1 (or Samba, unsure who's here to blame) to create
>first an file empty with the read-only DOS attribute set that prevents any writes,
>and then actually trying to opening it in write mode, which of course fails.
>
>Seems to be a pretty common issue that nobody has yet reported officially, judging
>by the amount of posts on Stackoverflow, impacting not only WSL but also CIFS
>under Linux (hence why sending to this mailing list and not the Windows-specific
>one):
>
> - https://superuser.com/questions/681196/debugging-git-repo-permissions-on-
>samba-share
> - https://superuser.com/questions/1450094/git-on-wsl-commands-fail-despite-
>permissions-seeming-fine
> - https://superuser.com/questions/1491499/use-git-on-a-shared-drive-within-
>wsl
>
>As a workaround, opening the file with permissions 0600 and then using a fchmod
>with the final desired mode works, which is a very small change that should cause
>no issues under neither real Linux nor WSL:
>
>--- git-2.39.5.orig/wrapper.c
>+++ git-2.39.5/wrapper.c
>@@ -484,9 +484,11 @@ int git_mkstemps_mode(char *pattern, int
> 			v /= num_letters;
> 		}
>
>-		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
>-		if (fd >= 0)
>+		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);
>+		if (fd >= 0) {
>+			fchmod(fd, mode);
> 			return fd;
>+		}

I am not certain this is either necessary or important to platforms other than Windows.
The /tmp directory is often, and properly set with the sticky bit +t. This ensures that only
The creating user has access to the temp file regardless of create ownership or security.
I would prefer that this be put into a compat layer rather than made general change.

> 		/*
> 		 * Fatal error (EPERM, ENOSPC etc).
> 		 * It doesn't make sense to loop.
>
>The WSL team at Microsoft has been already informed as well:
>https://github.com/microsoft/WSL/issues/12051

RE: Permission issue in Git in DrvFs-mounted network drives

[Marcos Del Sol Vives]

 ---- El Wed, 18 Sep 2024 18:30:05 +0200,    escribió ---- 
 > On September 18, 2024 12:16 PM, Marcos Del Sol Vives wrote:
 > >Under WSL1 (Windows Subsystem for Linux), when using a network share
 > >mounted via DrvFs, Git fails to add any files to a new or an existing repository.
 > >
 > >The reason is that Git tries to open a temporary file as with RW permissions but
 > >mode 0444, which causes WSL1 (or Samba, unsure who's here to blame) to create
 > >first an file empty with the read-only DOS attribute set that prevents any writes,
 > >and then actually trying to opening it in write mode, which of course fails.
 > >
 > >Seems to be a pretty common issue that nobody has yet reported officially, judging
 > >by the amount of posts on Stackoverflow, impacting not only WSL but also CIFS
 > >under Linux (hence why sending to this mailing list and not the Windows-specific
 > >one):
 > >
 > > - https://superuser.com/questions/681196/debugging-git-repo-permissions-on-
 > >samba-share
 > > - https://superuser.com/questions/1450094/git-on-wsl-commands-fail-despite-
 > >permissions-seeming-fine
 > > - https://superuser.com/questions/1491499/use-git-on-a-shared-drive-within-
 > >wsl
 > >
 > >As a workaround, opening the file with permissions 0600 and then using a fchmod
 > >with the final desired mode works, which is a very small change that should cause
 > >no issues under neither real Linux nor WSL:
 > >
 > >--- git-2.39.5.orig/wrapper.c
 > >+++ git-2.39.5/wrapper.c
 > >@@ -484,9 +484,11 @@ int git_mkstemps_mode(char *pattern, int
 > >             v /= num_letters;
 > >         }
 > >
 > >-        fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
 > >-        if (fd >= 0)
 > >+        fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);
 > >+        if (fd >= 0) {
 > >+            fchmod(fd, mode);
 > >             return fd;
 > >+        }
 > 
 > I am not certain this is either necessary or important to platforms other than Windows.
 > The /tmp directory is often, and properly set with the sticky bit +t. This ensures that only
 > The creating user has access to the temp file regardless of create ownership or security.
 > I would prefer that this be put into a compat layer rather than made general change.
 
The temporary file is created in .git/objects/XX/tmp_obj_YYYY, not in /tmp.
As shown in the first StackExchange link, this also impacts Linux machines.

 > >         /*
 > >          * Fatal error (EPERM, ENOSPC etc).
 > >          * It doesn't make sense to loop.
 > >
 > >The WSL team at Microsoft has been already informed as well:
 > >https://github.com/microsoft/WSL/issues/12051
 > 
 > 

Re: Permission issue in Git in DrvFs-mounted network drives

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2612 bytes --]

On 2024-09-18 at 16:16:26, Marcos Del Sol Vives wrote:
> Under WSL1 (Windows Subsystem for Linux), when using a network share
> mounted via DrvFs, Git fails to add any files to a new or an existing
> repository.
> 
> The reason is that Git tries to open a temporary file as with RW
> permissions but mode 0444, which causes WSL1 (or Samba, unsure who's here
> to blame) to create first an file empty with the read-only DOS attribute set
> that prevents any writes, and then actually trying to opening it in write
> mode, which of course fails.
> 
> Seems to be a pretty common issue that nobody has yet reported officially,
> judging by the amount of posts on Stackoverflow, impacting not only WSL
> but also CIFS under Linux (hence why sending to this mailing list and not
> the Windows-specific one):
> 
>  - https://superuser.com/questions/681196/debugging-git-repo-permissions-on-samba-share
>  - https://superuser.com/questions/1450094/git-on-wsl-commands-fail-despite-permissions-seeming-fine
>  - https://superuser.com/questions/1491499/use-git-on-a-shared-drive-within-wsl
> 
> As a workaround, opening the file with permissions 0600 and then using a
> fchmod with the final desired mode works, which is a very small change that
> should cause no issues under neither real Linux nor WSL:

This behaviour also occurs on some broken NFS mounts.  It is, however,
explicitly permitted by POSIX to open a file 0444 with O_WRONLY or
O_RDWR, and we've traditionally not fixed it for that reason.  I would
say that WSL1 behaviour is incorrect here.  Here's what POSIX says[0]:

  The argument following the oflag argument does not affect whether the
  file is open for reading, writing, or for both.

That is, POSIX forbids failing to open a file for writing because the
permissions are 0444, and as such this is not POSIX compliant.  Note
that the operation must also be atomic; that is, the server can't
emulate this by setting the permissions to 0600 and then doing an fchmod
internally unless nobody can ever detect the intermediate state.

I think we also probably have a few other codepaths that are affected in
the same way and this is maybe just the most noticeable.

Other folks may feel differently about fixing this, but I would suggest
against it.

> The WSL team at Microsoft has been already informed as well:
> https://github.com/microsoft/WSL/issues/12051

I've provided some comments in the issue there.

[0] https://pubs.opengroup.org/onlinepubs/9799919799/functions/open.html
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: Permission issue in Git in DrvFs-mounted network drives

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> Other folks may feel differently about fixing this, but I would suggest
> against it.
>
>> The WSL team at Microsoft has been already informed as well:
>> https://github.com/microsoft/WSL/issues/12051
>
> I've provided some comments in the issue there.
>
> [0] https://pubs.opengroup.org/onlinepubs/9799919799/functions/open.html

It depends on how hard it would be for filesystem providers to fix
their ware, but (1) leaving our code to require open semantics of
POSIX may serve as an incentive to encourage them to do so, and (2)
I am not fundamentally opposed to have an option to help users of
such flawed filesystem, provided if such a change is done cleanly.

Thanks.

Re: Permission issue in Git in DrvFs-mounted network drives

[Marcos Del Sol Vives]

---- El Fri, 20 Sep 2024 17:51:57 +0200,  brian m. carlson  escribió ---- 
> That is, POSIX forbids failing to open a file for writing because the
> permissions are 0444, and as such this is not POSIX compliant.  Note
> that the operation must also be atomic; that is, the server can't
> emulate this by setting the permissions to 0600 and then doing an fchmod
> internally unless nobody can ever detect the intermediate state.

I'm not sure why atomicity would be an issue here?

The file already exists between the open and the fchmod, so another
concurrent thread would be unable to open it regardless due to O_EXCL and
O_CREAT.

In fact, Git is *already* using this method of opening with 0600 for
writing and then chmoding to 0444 when writing pack files: in
builtin/index-pack.c, method open_pack_file opens with 0600 to write, and
then chmods to 444 in rename_tmp_packfile.

> Other folks may feel differently about fixing this, but I would suggest
> against it.

I understand that it's a workaround required due to non-POSIX compliance,
but considering it should have no impact on any other OS and will actually
as you said make buggy NFS mount work, IMHO it'd be still nice to have
the changes made.

---- El Fri, 20 Sep 2024 20:15:42 +0200,  Junio C Hamano  escribió ---- 
> It depends on how hard it would be for filesystem providers to fix
> their ware, but (1) leaving our code to require open semantics of
> POSIX may serve as an incentive to encourage them to do so, and (2)
> I am not fundamentally opposed to have an option to help users of
> such flawed filesystem, provided if such a change is done cleanly.

You mean adding a Git setting to, for example, disable using 444 mode
altogether? Or do it in two steps (open + fchmod)?

Re: Permission issue in Git in DrvFs-mounted network drives

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1348 bytes --]

On 2024-09-20 at 18:36:21, Marcos Del Sol Vives wrote:
> ---- El Fri, 20 Sep 2024 17:51:57 +0200,  brian m. carlson  escribió ----
> > Other folks may feel differently about fixing this, but I would suggest
> > against it.
> 
> I understand that it's a workaround required due to non-POSIX compliance,
> but considering it should have no impact on any other OS and will actually
> as you said make buggy NFS mount work, IMHO it'd be still nice to have
> the changes made.

Git could, in theory, accept a patch here.  However, you're also going
to have lots of other software that breaks in this case, not just Git.
So it's less useful to patch Git and hundreds of other packages on Linux
distributions that have relied on the POSIX standard and more useful to
fix your OS (or maybe switch to WSL2, if that doesn't have the problem).
Most Linux distros will generally not be interested in fixing this class
of problem, in my experience.

In addition, chmod doesn't always work under WSL.  I believe it _does_
work if the drive is mounted with metadata, but some people don't have
that enabled and I don't know if it works for all drives.  For those
people, the current code will work, since it doesn't call chmod or
fchmod, but it will fail with your patch.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: Permission issue in Git in DrvFs-mounted network drives

[Marcos Del Sol Vives]

---- El Fri, 20 Sep 2024 22:43:31 +0200,  brian m. carlson  escribió ---- 
> Git could, in theory, accept a patch here.  However, you're also going
> to have lots of other software that breaks in this case, not just Git.
> So it's less useful to patch Git and hundreds of other packages on Linux
> distributions that have relied on the POSIX standard and more useful to
> fix your OS (or maybe switch to WSL2, if that doesn't have the problem).
> Most Linux distros will generally not be interested in fixing this class
> of problem, in my experience.

There is not a significant amount of software that opens files for writing
in read-only mode. Despite using WSL extensively for development, so far
Git has been the only that failed to work.

WSL2 has poor support for accessing local devices (such as serial ports,
something really useful for embedded development like the Proxmark project
where I contribute to) or pipes (which I use to sign commits using a
TPM-backed SSH key), so it's not really an option.

> In addition, chmod doesn't always work under WSL.  I believe it _does_
> work if the drive is mounted with metadata, but some people don't have
> that enabled and I don't know if it works for all drives.  For those
> people, the current code will work, since it doesn't call chmod or
> fchmod, but it will fail with your patch.

I can guarantee you it'll work under WSL, even without metadata, because
I've patched Git on my WSL1 Debian machine by doing the aforementioned
trick and it totally works. Metadata is not even supported on anything but
local NTFS drives.

WSL1, when mounted without metadata, chmod and fchmod calls will not fail
but would instead just ignore the call, *except* when setting mode 444,
400 or any other read-only mode where it will set the FAT/NTFS read-only
attribute:

marcos@desk:/mnt/nas/marcos$ mount | grep nas
\\192.168.0.245\marcos on /mnt/nas/marcos type drvfs (rw,relatime,uid=1000,gid=1000,case=off)
marcos@desk:/mnt/nas/marcos$ touch readonly
marcos@desk:/mnt/nas/marcos$ ls -l readonly
-rwxrwxrwx 1 marcos marcos 0 Sep 21 00:33 readonly
marcos@desk:/mnt/nas/marcos$ chmod 444 readonly
marcos@desk:/mnt/nas/marcos$ ls -l readonly
-r-xr-xr-x 1 marcos marcos 0 Sep 21 00:33 readonly

That is exactly what is causing the issue - two SMB requests are being
issued: one to create the file in read-only mode, and then another to
open it.

Re: Permission issue in Git in DrvFs-mounted network drives

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3264 bytes --]

On 2024-09-20 at 22:39:51, Marcos Del Sol Vives wrote:
> ---- El Fri, 20 Sep 2024 22:43:31 +0200,  brian m. carlson  escribió ---- 
> There is not a significant amount of software that opens files for writing
> in read-only mode. Despite using WSL extensively for development, so far
> Git has been the only that failed to work.

I have been using Debian for many years and have contributed to several
of the packages.  I assure you that using what Git is doing is not, in
fact, uncommon.  It may be that you haven't run into it, but it is
definitely not rare.

For example, I just found an example of doing exactly the same thing in
zsh, which is a major project that people use across distros.  There's
apparently another in the elm MUA.  I found yet another one in some m4
(autoconf) files used in building Emacs.  And there are a lot of other
examples you can find by searching GitHub's code search for "open
O_WRONLY 0444".

Perhaps you don't use any of that software, but they're all substantial
projects that are in common use across Linux distros and Unix systems in
general.  I am a zsh user, for instance.

> > In addition, chmod doesn't always work under WSL.  I believe it _does_
> > work if the drive is mounted with metadata, but some people don't have
> > that enabled and I don't know if it works for all drives.  For those
> > people, the current code will work, since it doesn't call chmod or
> > fchmod, but it will fail with your patch.
> 
> I can guarantee you it'll work under WSL, even without metadata, because
> I've patched Git on my WSL1 Debian machine by doing the aforementioned
> trick and it totally works. Metadata is not even supported on anything but
> local NTFS drives.

I have actually answered questions about chmod failing to work in some
cases on WSL, which causes operations to fail.  Here's a different
question on StackOverflow that demonstrates a particular problem that's
common where chmod fails to work in Git on certain WSL drives[0].  The
error looks like this:

  Cloning into '<repo>'...
  error: chmod on /mnt/c/Users/User/Code/<repo>/.git/config.lock failed: Operation not permitted
  fatal: could not set 'core.filemode' to 'false'

If this situation occurs, your patch will actually leave the file
writable, which we don't want, since the fchmod will silently fail.

It may be that WSL has changed since that was written, but I've seen
reports of that happening relatively recently, and, in any event, we
don't want to require that people use the absolute latest WSL to get
things working.

Again, maybe Junio will accept a patch here, but I don't think the
current one is going to work correctly, even on WSL.  It may not be
obvious that it's broken because the patch ignores fchmod's exit status.
If you want to submit a patch that uses an option to either use the old
behaviour or one that does an fchmod after the fact, that would probably
be accepted, as long as you added tests (including the read-only bit)
for both cases and sufficient documentation.

[0] https://askubuntu.com/questions/1115564/wsl-ubuntu-distro-how-to-solve-operation-not-permitted-on-cloning-repository
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]