Page MenuHomePhabricator

Windows may have irregular behaviors in unusual cases when copying a file to preserve attributes/permissions
Open, WishlistPublic

Description

See https://discourse.phabricator-community.org/t/filesystem-copyfile-fails-on-windows-due-to-incompatibility-between-copy-and-bypass-shell/4161.

Filesystem::copyFile() currently invokes copy as a subprocess, but this is a shell command and no longer works now that we use bypass_shell. See T13209.

(I haven't reproduced this yet, but the report sounds pretty credible.)

The suggested fix (use cmd /c copy ...) is an improvement, but using PHP copy() or a manual implementation might be preferable.

See PHI2036.

Event Timeline

epriestley triaged this task as Normal priority.Aug 11 2020, 5:02 PM
epriestley created this task.

D21643 "fixes" this by using copy(). However:

Filesystem::copyFile() has only one callsite, in lint, and it is only used to copy file permissions before writing a corrected file. On non-Windows systems, the implementation is cp -p, with -p ("preserve a bunch of file attributes") as the critical piece.

It's not clear to me if copy() copies all permissions/attributes (or if this is even meaningful on Windows). It seems like it works in Git bash for me, but I think Git bash is just using a filename heuristic on NTFS since NTFS doesn't support an executable bit, e.g. see here:

https://stackoverflow.com/questions/25730041/updating-file-permissions-with-git-bash-on-windows-7

So, likely:

  • Git +x must be managed explicitly in Windows on NTFS (git update-index --chmod=+x scripts/script.sh).
  • Since we're always eventually writing a file with the same path and name, we get the right result "for free" (on Windows + NTFS) no matter what, even if the intermediate file doesn't actually have the right permissions in NTFS and/or Git and/or Git Bash (MINGW), unless lint is changing +x on the file, which it currently never can.
  • Unclear what happens if you have a Windows drive with a filesystem other than NTFS. I don't have any on hand and am not ambitious enough to format a disk to test this.

Upshot:

  • Windows with non-NTFS filesystems might lose +x when a +x file is modified by lint, but this is a lot of work to test and might affect zero or nearly-zero users even if it does happen.
  • Linters which add or remove +x (these currently do not exist) likely don't work on NTFS (and can't work naively: they must execute raw git commands).
  • Filesystem::copyFile() could be removed, and the single callsite could be replaced with Filesystem::createNewFileWithSamePermissionsAs($src, $dst). This would, in theory, improve performance slightly when linting very large files (by omitting an unnecessary copy).
  • copy() on Windows might also drop other permissions or otherwise produce an imperfect copy -- I'm not a Wizard Who Knows A Lot About Windows Filesystems -- but I couldn't find anything by poking at it for a bit.
epriestley renamed this task from Filesystem class invokes "copy" command on Windows, which fails after "bypass_shell" changes to Windows may have irregular behaviors in unusual cases when copying a file to preserve attributes/permissions.Mar 22 2021, 7:00 PM
epriestley lowered the priority of this task from Normal to Wishlist.