From 0b29acb0cab7baeb6af77aa9ae259b4c6ebf000a Mon Sep 17 00:00:00 2001 From: Santiago Gimeno Date: Fri, 17 May 2019 14:25:43 +0200 Subject: [PATCH] fs: fix uv_fs_copyfile if same src and dst Specifically return success. It fixes a tight loop on unices as `sendfile()` was returning 0 and also makes the function behave the same both on `unix` and `windows`. It partially implements: https://github.com/libuv/libuv/issues/2237 PR-URL: https://github.com/libuv/libuv/pull/2298 Refs: https://github.com/nodejs/node/issues/27746 Reviewed-By: Jameson Nash Reviewed-By: Colin Ihrig --- src/unix/fs.c | 23 ++++++++++++---- src/win/fs.c | 60 +++++++++++++++++++++++++++++------------ test/test-fs-copyfile.c | 7 +++++ 3 files changed, 68 insertions(+), 22 deletions(-) diff --git a/src/unix/fs.c b/src/unix/fs.c index a532d810..5138c619 100644 --- a/src/unix/fs.c +++ b/src/unix/fs.c @@ -947,7 +947,8 @@ static ssize_t uv__fs_copyfile(uv_fs_t* req) { uv_fs_t fs_req; uv_file srcfd; uv_file dstfd; - struct stat statsbuf; + struct stat src_statsbuf; + struct stat dst_statsbuf; int dst_flags; int result; int err; @@ -965,7 +966,7 @@ static ssize_t uv__fs_copyfile(uv_fs_t* req) { return srcfd; /* Get the source file's mode. */ - if (fstat(srcfd, &statsbuf)) { + if (fstat(srcfd, &src_statsbuf)) { err = UV__ERR(errno); goto out; } @@ -980,7 +981,7 @@ static ssize_t uv__fs_copyfile(uv_fs_t* req) { &fs_req, req->new_path, dst_flags, - statsbuf.st_mode, + src_statsbuf.st_mode, NULL); uv_fs_req_cleanup(&fs_req); @@ -989,7 +990,19 @@ static ssize_t uv__fs_copyfile(uv_fs_t* req) { goto out; } - if (fchmod(dstfd, statsbuf.st_mode) == -1) { + /* Get the destination file's mode. */ + if (fstat(dstfd, &dst_statsbuf)) { + err = UV__ERR(errno); + goto out; + } + + /* Check if srcfd and dstfd refer to the same file */ + if (src_statsbuf.st_dev == dst_statsbuf.st_dev && + src_statsbuf.st_ino == dst_statsbuf.st_ino) { + goto out; + } + + if (fchmod(dstfd, src_statsbuf.st_mode) == -1) { err = UV__ERR(errno); goto out; } @@ -1019,7 +1032,7 @@ static ssize_t uv__fs_copyfile(uv_fs_t* req) { } #endif - bytes_to_send = statsbuf.st_size; + bytes_to_send = src_statsbuf.st_size; in_offset = 0; while (bytes_to_send != 0) { err = uv_fs_sendfile(NULL, diff --git a/src/win/fs.c b/src/win/fs.c index 9e2f084c..7d78d466 100644 --- a/src/win/fs.c +++ b/src/win/fs.c @@ -1409,47 +1409,57 @@ INLINE static void fs__stat_prepare_path(WCHAR* pathw) { } -INLINE static void fs__stat_impl(uv_fs_t* req, int do_lstat) { +INLINE static DWORD fs__stat_impl_from_path(WCHAR* path, + int do_lstat, + uv_stat_t* statbuf) { HANDLE handle; DWORD flags; + DWORD ret; flags = FILE_FLAG_BACKUP_SEMANTICS; - if (do_lstat) { + if (do_lstat) flags |= FILE_FLAG_OPEN_REPARSE_POINT; - } - handle = CreateFileW(req->file.pathw, + handle = CreateFileW(path, FILE_READ_ATTRIBUTES, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL, OPEN_EXISTING, flags, NULL); - if (handle == INVALID_HANDLE_VALUE) { - SET_REQ_WIN32_ERROR(req, GetLastError()); - return; - } - if (fs__stat_handle(handle, &req->statbuf, do_lstat) != 0) { - DWORD error = GetLastError(); + if (handle == INVALID_HANDLE_VALUE) + ret = GetLastError(); + else if (fs__stat_handle(handle, statbuf, do_lstat) != 0) + ret = GetLastError(); + else + ret = 0; + + CloseHandle(handle); + return ret; +} + + +INLINE static void fs__stat_impl(uv_fs_t* req, int do_lstat) { + DWORD error; + + error = fs__stat_impl_from_path(req->file.pathw, do_lstat, &req->statbuf); + if (error != 0) { if (do_lstat && (error == ERROR_SYMLINK_NOT_SUPPORTED || error == ERROR_NOT_A_REPARSE_POINT)) { /* We opened a reparse point but it was not a symlink. Try again. */ fs__stat_impl(req, 0); - } else { /* Stat failed. */ - SET_REQ_WIN32_ERROR(req, GetLastError()); + SET_REQ_WIN32_ERROR(req, error); } - CloseHandle(handle); return; } req->ptr = &req->statbuf; req->result = 0; - CloseHandle(handle); } @@ -1553,6 +1563,9 @@ static void fs__ftruncate(uv_fs_t* req) { static void fs__copyfile(uv_fs_t* req) { int flags; int overwrite; + DWORD error; + uv_stat_t statbuf; + uv_stat_t new_statbuf; flags = req->fs.info.file_flags; @@ -1563,12 +1576,25 @@ static void fs__copyfile(uv_fs_t* req) { overwrite = flags & UV_FS_COPYFILE_EXCL; - if (CopyFileW(req->file.pathw, req->fs.info.new_pathw, overwrite) == 0) { - SET_REQ_WIN32_ERROR(req, GetLastError()); + if (CopyFileW(req->file.pathw, req->fs.info.new_pathw, overwrite) != 0) { + SET_REQ_RESULT(req, 0); return; } - SET_REQ_RESULT(req, 0); + SET_REQ_WIN32_ERROR(req, GetLastError()); + if (req->result != UV_EBUSY) + return; + + /* if error UV_EBUSY check if src and dst file are the same */ + if (fs__stat_impl_from_path(req->file.pathw, 0, &statbuf) != 0 || + fs__stat_impl_from_path(req->fs.info.new_pathw, 0, &new_statbuf) != 0) { + return; + } + + if (statbuf.st_dev == new_statbuf.st_dev && + statbuf.st_ino == new_statbuf.st_ino) { + SET_REQ_RESULT(req, 0); + } } diff --git a/test/test-fs-copyfile.c b/test/test-fs-copyfile.c index 918c086d..def3d967 100644 --- a/test/test-fs-copyfile.c +++ b/test/test-fs-copyfile.c @@ -120,6 +120,13 @@ TEST_IMPL(fs_copyfile) { ASSERT(r != 0); uv_fs_req_cleanup(&req); + /* Succeeds if src and dst files are identical. */ + touch_file(src, 12); + r = uv_fs_copyfile(NULL, &req, src, src, 0, NULL); + ASSERT(r == 0); + uv_fs_req_cleanup(&req); + unlink(src); + /* Copies file synchronously. Creates new file. */ unlink(dst); r = uv_fs_copyfile(NULL, &req, fixture, dst, 0, NULL);