| Message ID | 19205ebb4b9.c2a2da5a2387912.3559118454287459572@orca.pet (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Permission issue in Git in DrvFs-mounted network drives | expand |
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
---- 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 > >
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" <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.
---- 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)?
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.
---- 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.
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
--- 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.