libuv
libuv copied to clipboard
libuv
fs: bring back macos-specific copyfile(3)
copyfile(3) does not always do its own open according to permission rules. We can solve that by guarding it with one of our own opens. This way we can keep the cool clonefile thing for huge node_modules and still have sensible results.
MacOS does not have faccess(2) in case you are wondering why I did this. There is only access(2) with the "real UID" shenanigans that will bite us in the future.
This PR partially reverts #2233. Tests in the said PR are kept, and they should pass.
We need to do an open first per nodejs/node#26936. Amending.
For the record, this one is possibly due to this part in copyfile: if(chmod(s->dst, (s->sb.st_mode | S_IWUSR) & ~S_IFMT) == 0). Absolute unit.
For clonefileat it should fail correctly, since COPYFILE_UNLINK is not default and the syscall always fails when dest exists
fstat is not the best idea because ACL is a thing on macOS. The correct way to do so is via the access(2) family of APIs, but macOS does not have a faccess(2) / faccessat(2) that does EUID instead of the UID with AT_EACCESS. A quick Google search I did yesterday finds CarbonStdCLib.o, apparently a C library polyfill, doing something similar for faccess.
Unlinking is what the OS X cp(1) command does for -c anyways, so it kind of is idiomatic in this use.
Gonna fix the branch thing later today (UTC+8).
Hmm, looks like there is faccessat(2) on macOS >= 10.10. What's the OS support policy for libuv and nodejs? (Looks like I can do that for node, which only supports macOS >= 10.11.)
A good-ish polyfill for our desired functionality is found in gnulib euidaccess(3) (GPL!!). It gives two implementations: the correct but non-reentrant one using a setre{u,g}id-access-setre{u,g}id pattern and the traditional stat thing which is incorrect with ACL. So which one is more evil here, changing a global state or being incorrect in a case not yet tested?
Since faccessat(2) is a syscall, I can possibly get away with doing a version check like the clone part and syscall(2) the whole thing away on 10.10+. I still need to pick a way to polyfill this feature for [10.7, 10.10), so please do give your suggestion on that. It should look like:
#if defined(__APPLE__) && !TARGET_OS_IPHONE
/** @returns 1: writeable. 2: does not exist. */
static int fs__mac_canwrite(const char* path, int darwin_kern_maj) {
int res = 0;
// 10.7+
if (darwin_kern_maj >= 11) {
// TODO: rewrite to use syscall(2) and sys/syscall.h!
res = faccessat(AT_FDCWD, path, W_OK | R_OK, AT_EACCESS) == 0;
goto test_exist;
}
/* polyfill here */
test_exist: // should be reused by stat or access polyfills
if (res == 0 && errorno == ENOENT)
return 2;
return res;
}
static bool fs__mac_can_copy(const char* path, int darwin_kern_maj, int excl) {
int res;
res = fs__mac_canwrite(path, darwin_kern_maj);
if (res != 2)
return ! excl;
// Apple manpage says their dirname does not modify the input. Trust it for now.
return !! fs__mac_canwrite(dirname(path), darwin_kern_maj);
}
#endif
I presume the gnulib implementation will be GPL and therefore cannot be used in libuv.
Yeah, someone will need to wipe their brain with only the phrase "implement an access check based on stat() or setuid and access(2)" left if they decided to implement it here. Should possibly add a click warning on the gnulib link.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
![stale[bot] avatar](https://avatars.githubusercontent.com/in/1724?v=4 "Published by a pub.dev verified publisher"){kind=small}
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
With this change, the macOS implementation has more overlap with the Unix implementation than it previously did. The overlap is more prone to getting out of sync - for example, the Unix implementation no longer opens the destination with O_TRUNC like this PR does.
Maybe we should look into using fcopyfile() only for clone operations and use the common implementation for everything else. It would essentially be a macOS specific version of this code block.
Makes sense. An open() is needed for the perm check anyways.
The chmod() stuff is a bit extra, but ehh... let it slide.
Meh, reasoning through the open fds and fallbacks with clonefile(2) is still too hairy for me.
- I need to
unlinkthe file forclonefileto work, or it would fail with EEXIST. Yeah it takes paths, not fds. - If
clonefiledoes fail for some other reason, I would need to do the very verbose open thing again. That's even uglier than two separate blocks.
@Artoria2e5 Is this ready for re-review or are you still working things out?
There isn't much I can change right now, so please just throw more comments at me.
Meh, reasoning through the open fds and fallbacks with clonefile(2) is still too hairy for me.
FWIW, I was suggesting to use fcopyfile() since it works with fds and doesn't require unlinking the file first.
That wouldn't be possible since fcopyfile() does not perform cloning. I am not sure if it's in the documentation, since copyfile.c is a bit easier to Google.
(If this was any other vendor, I would've figured that it's impossible since the lower level syscall doesn't work with fds. But everything's possible with Apple :)
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
File cloning is very important to package managers. This makes this PR very important for a lot of people, especially when a good chunk of the JS developer ecosystem uses OSX on their day-to-day work 😕
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
That wouldn't be possible since fcopyfile() does not perform cloning.
FWIW, I tested fcopyfile() with the COPYFILE_CLONE_FORCE, and it appeared to work (at least it didn't return an error):
diff --git a/src/unix/fs.c b/src/unix/fs.c
index 88ac0731..6186289a 100644
--- a/src/unix/fs.c
+++ b/src/unix/fs.c
@@ -62,6 +62,7 @@
#endif
#if defined(__APPLE__)
+# include <copyfile.h>
# include <sys/sysctl.h>
#elif defined(__linux__) && !defined(FICLONE)
# include <sys/ioctl.h>
@@ -1259,6 +1260,37 @@ static ssize_t uv__fs_copyfile(uv_fs_t* req) {
goto out;
}
}
+#elif defined(__APPLE__) && !TARGET_OS_IPHONE
+ if (req->flags & UV_FS_COPYFILE_FICLONE ||
+ req->flags & UV_FS_COPYFILE_FICLONE_FORCE) {
+ static int can_clone;
+ copyfile_flags_t flags;
+ char buf[64];
+ size_t len;
+ int major;
+
+ /* Check OS version. Cloning is only supported on macOS >= 10.12. */
+ if (req->flags & UV_FS_COPYFILE_FICLONE_FORCE) {
+ if (can_clone == 0) {
+ len = sizeof(buf);
+ if (sysctlbyname("kern.osrelease", buf, &len, NULL, 0))
+ return UV__ERR(errno);
+
+ if (1 != sscanf(buf, "%d", &major))
+ abort();
+
+ can_clone = -1 + 2 * (major >= 16); /* macOS >= 10.12 */
+ }
+
+ if (can_clone < 0)
+ return UV_ENOSYS;
+ }
+
+ flags = COPYFILE_ALL | (1 << 25 /* COPYFILE_CLONE_FORCE */);
+ err = fcopyfile(srcfd, dstfd, NULL, flags);
+ if (err == 0 || req->flags & UV_FS_COPYFILE_FICLONE_FORCE)
+ goto out;
+ }
#else
if (req->flags & UV_FS_COPYFILE_FICLONE_FORCE) {
err = UV_ENOSYS;
I may have spoken too soon:
COPYFILE_CLONE_FORCE Clone the file instead. This is a force flag i.e.
if cloning fails, an error is returned. This flag
is equivalent to (COPYFILE_EXCL | COPYFILE_ACL |
COPYFILE_STAT | COPYFILE_XATTR | COPYFILE_DATA |
COPYFILE_NOFOLLOW_SRC). Note that if cloning is
successful, progress callbacks will not be
invoked. Note also that there is no support for
cloning directories: if a directory is provided as
the source, an error will be returned. (This is
only applicable for the copyfile() function.)
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Do I summarize correctly this pull request is blocked indefinitely as long as libuv supports old macos versions?
Personally, I'm okay with raising the minimum version (10.7 is 10 years old) but, on the other hand, I know there are still a handful of users left.
Well it's not like they couldn't just... Use the old version. It's what macOS 10.7 users are very much into anyway.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
It's really quite frustrating indeed. @bnoordhuis @cjihrig what does the OP need to do to bring this PR to the finish line?
I think this is in dire need of a concrete action item.
Adding a + 1 too, just ran into this issue on Mac using pnpm + git worktrees.
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.
To give you a sense of the impact here — bun’s implementation of fs.copyFileSync which uses copyfile() on macOS performs 2x faster than node’s fs.copyFileSync when copying a 1 GB file.
I opened #3406 earlier this week to raise the baseline to macOS 10.15. When accepted and merged, I think that would unblock this PR.
To give you a sense of the impact here — bun’s implementation of fs.copyFileSync which uses copyfile() on macOS performs 2x faster than node’s fs.copyFileSync when copying a 1 GB file.
That's good, but I thought the main issue here was that APFS Copy on Write is not supported. When it is supported, the speedup should be closer to infinity than 2x.
This wasn’t with the COPYFILE_FICLONE flag, so whether or not it used copy on write is up to the implementation of copyfile()
On Fri, Jan 14, 2022 at 5:09 AM Petter Uvesten @.***> wrote:
To give you a sense of the impact here — bun’s implementation of fs.copyFileSync which uses copyfile() on macOS performs 2x faster than node’s fs.copyFileSync when copying a 1 GB file.
That's good, but I thought the main issue here was that APFS Copy on Write is not supported. When it is supported, the speedup should be closer to infinity than 2x.
— Reply to this email directly, view it on GitHub https://github.com/libuv/libuv/pull/2578#issuecomment-1013102928, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFNGS7IQTIBOXES67KGINLUWAN7NANCNFSM4J3JET2A . You are receiving this because you commented.Message ID: @.***>
The baseline is now macos 10.15. There's one remaining comment from @cjihrig so I defer to him on whether it's ready for merging. Let me know if I should also review.
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.
Adding my comment so my anti stale bot will take care of this pr from now on
There's some unaddressed feedback I left in February. Most is stylistic but the faccessat() question needs an answer before this can go in.
Also, a gentle reminder that bots and their owners are likely to meet with swift and permanent bans.
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.
@Artoria2e5 Do you plan on picking up this pull request again? If not, maybe someone else wants to step up and get this over the finish line?
If not, maybe someone else wants to step up and get this over the finish line?
I've tried to incorporate your feedback: https://github.com/libuv/libuv/pull/3654
In light of an updated PR being available I am closing this one. Just don't have time for code when I am all stuck at McD, I am afraid.