fs: remove macOS-specific copyfile(3) #2233

Trott · 2019-03-27T06:15:32Z

Using copyfile(3) on macOS apparently results in situations where file
permissions are ignored. Using the same code for other UNIX-like
operating systems seems to fix the issue.

Refs: nodejs/node#26936 (comment)

@cjihrig

Using copyfile(3) on macOS apparently results in situations where file permissions are ignored. Using the same code for other UNIX-like operating systems seems to fix the issue. Refs: nodejs/node#26936 (comment)

On macOS, fs.copyFile() may not respect file permissions. There is a PR for libuv that should fix this, but until it lands and we can either float a patch or upgrade libuv, have a known_issues test. Ref: nodejs#26936 Ref: libuv/libuv#2233

Trott · 2019-03-27T06:39:14Z

(Seems like this should get a test, but I don't know enough about libuv's tests to write one quickly so I'm going to sleep now. If someone else writes the test or leaves me hints or links, awesome. I do have an open PR to add a test to Node.js for this. If that's sufficient, great.) EDIT: Actually, the test setup looks straightforward. 👍

cjihrig · 2019-03-27T12:13:45Z

Please add a test case in test/test-fs-copyfile.c with the reproduction from the Node issue. It should fail using the macOS code and pass with your changes.

thefourtheye · 2019-03-27T13:02:32Z

Do we generally report these sort of bugs to the respective projects?

Add test to test-fs-copyfile.c to check that uv_fs_copyfile() respects destination file permissions. Previously, in macOS, it did not.

Trott · 2019-03-27T15:37:50Z

Test added. PTAL.

Trott · 2019-03-27T15:38:28Z

(The test fails for me on Mojave with current libuv src but passes with the changes in this PR.)

Trott · 2019-03-27T17:05:21Z

Not sure if this is the right way to run CI on libuv, but it seems like it probably is?: https://ci.nodejs.org/view/libuv/job/libuv-test-pipeline/49/

src/unix/fs.c

cjihrig · 2019-03-27T17:46:20Z

Not sure if this is the right way to run CI on libuv

I generally use https://ci.nodejs.org/view/libuv/job/libuv-test-commit/. And, if you want to test in the context of Node's test suite: https://ci.nodejs.org/view/libuv/job/libuv-in-node/

Trott · 2019-03-27T18:10:39Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/1329/
Other CI: https://ci.nodejs.org/view/libuv/job/libuv-in-node/86/

Looks like my test doesn't compile on Windows?

cjihrig · 2019-03-27T18:15:54Z

Looks like my test doesn't compile on Windows?

Hi! Welcome to libuv 👋

On a more serious note, I think you can #ifdef the test out on Windows (this is done in other tests like test/test-pipe-set-fchmod.c) because the permissions don't really transfer over there.

Trott · 2019-03-27T18:20:22Z

OK, used #ifndef to skip the test on Windows. CI runs again:

https://ci.nodejs.org/view/libuv/job/libuv-test-commit/1330/
https://ci.nodejs.org/view/libuv/job/libuv-in-node/87/

cjihrig · 2019-03-27T19:13:35Z

The libuv CI is OK. The failures are #2234. The failures in the Node CI seem unrelated, although it would be preferable if macOS passed there.

Trott · 2019-03-27T19:17:58Z

The failures in the Node CI seem unrelated, although it would be preferable if macOS passed there.

Couldn't figure out how to quickly/easily rebuild just macOS, so here's a full re-run to see what happens:

https://ci.nodejs.org/view/libuv/job/libuv-in-node/89/

Trott · 2019-03-28T00:18:45Z

(CI re-run is yellow, which I imagine is close-enough to green.)

src/unix/fs.c

On macOS, fs.copyFile() may not respect file permissions. There is a PR for libuv that should fix this, but until it lands and we can either float a patch or upgrade libuv, have a known_issues test. Ref: nodejs#26936 Ref: libuv/libuv#2233 PR-URL: nodejs#26939 Refs: nodejs#26936 Refs: libuv/libuv#2233 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

cjihrig · 2019-03-31T05:13:18Z

Now that nodejs/node#26939 landed in Node, I ran the libuv+Node integration CI again. I would expect the new test in that PR, test/known_issues/test-fs-copyfile-respect-permissions.js, to fail on macOS when built with this PR. That's not what I'm seeing though.

Here is the new CI run https://ci.nodejs.org/view/libuv/job/libuv-in-node/90/ where macOS comes back green.

cjihrig · 2019-03-31T05:50:58Z

I think nodejs/node#27024 addresses the issue, meaning this is still 👍

On macOS, fs.copyFile() may not respect file permissions. There is a PR for libuv that should fix this, but until it lands and we can either float a patch or upgrade libuv, have a known_issues test. Ref: #26936 Ref: libuv/libuv#2233 PR-URL: #26939 Refs: #26936 Refs: libuv/libuv#2233 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Trott · 2019-04-05T05:17:10Z

Anything more for me to do before this is merge-able?

On macOS, fs.copyFile() may not respect file permissions. There is a PR for libuv that should fix this, but until it lands and we can either float a patch or upgrade libuv, have a known_issues test. Ref: #26936 Ref: libuv/libuv#2233 PR-URL: #26939 Refs: #26936 Refs: libuv/libuv#2233 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>

cjihrig · 2019-04-09T14:05:01Z

I don't think there is anything left to do, but I'm going to run the Node integration CI once more: https://ci.nodejs.org/view/libuv/job/libuv-in-node/91/

cjihrig · 2019-04-09T19:09:50Z

The integration CI looks good. The only failure was test/known_issues/test-fs-copyfile-respect-permissions.js, which is expected because it's a known issue that gets fixed by this PR. I'll move that test once I update Node with the new libuv release.

Using copyfile(3) on macOS apparently results in situations where file permissions are ignored. Using the same code for other UNIX-like operating systems seems to fix the issue. Refs: nodejs/node#26936 (comment) PR-URL: #2233 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Add test to test-fs-copyfile.c to check that uv_fs_copyfile() respects destination file permissions. Previously, in macOS, it did not. PR-URL: #2233 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

cjihrig · 2019-04-11T01:50:14Z

Landed in d0fb86c...c7b87b0. Thanks, and congrats on your first commit(s).

devongovett · 2019-04-29T02:49:28Z

This seems to remove support for COPYFILE_FICLONE and COPYFILE_FICLONE_FORCE on macOS. I get an ENOSYS error when using the later in node 12. Would be nice to be able to optionally use copy on write for fast copying without shelling out to cp -c.

On macOS, fs.copyFile() may not respect file permissions. There is a PR for libuv that should fix this, but until it lands and we can either float a patch or upgrade libuv, have a known_issues test. Ref: #26936 Ref: libuv/libuv#2233 PR-URL: #26939 Refs: #26936 Refs: libuv/libuv#2233 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Artoria2e5 · 2019-12-16T12:02:28Z

@devongovett I agree that this change may be regressive. Since we are doing the chmod stuff anyways on the normal path, maybe I can try restoring it. As long as the test passes we are in the clear.

fs: remove macOS-specific copyfile(3)

d46e754

Using copyfile(3) on macOS apparently results in situations where file permissions are ignored. Using the same code for other UNIX-like operating systems seems to fix the issue. Refs: nodejs/node#26936 (comment)

Trott marked this pull request as ready for review March 27, 2019 06:19

Trott mentioned this pull request Mar 27, 2019

fs.copyFile on Mac OS does not respect permissions nodejs/node#26936

Closed

Trott mentioned this pull request Mar 27, 2019

test: add known_issues test for fs.copyFile() nodejs/node#26939

Closed

3 tasks

fs: add test for copyfile() respecting permissions

94043e2

Add test to test-fs-copyfile.c to check that uv_fs_copyfile() respects destination file permissions. Previously, in macOS, it did not.

thefourtheye approved these changes Mar 27, 2019

View reviewed changes

cjihrig reviewed Mar 27, 2019

View reviewed changes

src/unix/fs.c Show resolved Hide resolved

fixup! fs: remove macOS-specific copyfile(3)

ed7483c

fixup! fs: add test for copyfile() respecting permissions

076a928

Trott commented Mar 28, 2019

View reviewed changes

src/unix/fs.c Show resolved Hide resolved

bnoordhuis approved these changes Mar 28, 2019

View reviewed changes

cjihrig approved these changes Mar 28, 2019

View reviewed changes

santigimeno approved these changes Mar 29, 2019

View reviewed changes

cjihrig closed this Apr 11, 2019

richardlau mentioned this pull request Apr 17, 2019

fs.copyFile with COPYFILE_FICLONE flag does not overwrite destination if it exists nodejs/node#27273

Closed

Artoria2e5 mentioned this pull request Dec 16, 2019

fs: bring back macos-specific copyfile(3) #2578

Closed

cristianl mentioned this pull request Jan 11, 2020

Use fs.copyFile with UV_FS_COPYFILE_FICLONE flag when ready pnpm/pnpm#1066

Closed

2 tasks

richardlau mentioned this pull request Aug 4, 2020

fs.copyFile creates empty file if source and destination are the same nodejs/node#34624

Closed

bnoordhuis mentioned this pull request Oct 11, 2020

uv__fs_copyfile is wrong for macOS #3016

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: remove macOS-specific copyfile(3) #2233

fs: remove macOS-specific copyfile(3) #2233

Trott commented Mar 27, 2019

Trott commented Mar 27, 2019 •

edited

Loading

cjihrig commented Mar 27, 2019

thefourtheye commented Mar 27, 2019

Trott commented Mar 27, 2019

Trott commented Mar 27, 2019

Trott commented Mar 27, 2019

cjihrig commented Mar 27, 2019

Trott commented Mar 27, 2019

cjihrig commented Mar 27, 2019

Trott commented Mar 27, 2019

cjihrig commented Mar 27, 2019

Trott commented Mar 27, 2019

Trott commented Mar 28, 2019

cjihrig commented Mar 31, 2019

cjihrig commented Mar 31, 2019

Trott commented Apr 5, 2019

cjihrig commented Apr 9, 2019

cjihrig commented Apr 9, 2019

cjihrig commented Apr 11, 2019

devongovett commented Apr 29, 2019

Artoria2e5 commented Dec 16, 2019

fs: remove macOS-specific copyfile(3) #2233

fs: remove macOS-specific copyfile(3) #2233

Conversation

Trott commented Mar 27, 2019

Trott commented Mar 27, 2019 • edited Loading

cjihrig commented Mar 27, 2019

thefourtheye commented Mar 27, 2019

Trott commented Mar 27, 2019

Trott commented Mar 27, 2019

Trott commented Mar 27, 2019

cjihrig commented Mar 27, 2019

Trott commented Mar 27, 2019

cjihrig commented Mar 27, 2019

Trott commented Mar 27, 2019

cjihrig commented Mar 27, 2019

Trott commented Mar 27, 2019

Trott commented Mar 28, 2019

cjihrig commented Mar 31, 2019

cjihrig commented Mar 31, 2019

Trott commented Apr 5, 2019

cjihrig commented Apr 9, 2019

cjihrig commented Apr 9, 2019

cjihrig commented Apr 11, 2019

devongovett commented Apr 29, 2019

Artoria2e5 commented Dec 16, 2019

Trott commented Mar 27, 2019 •

edited

Loading