Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fs.lchown should be undeprecated #19868

Closed
simevo opened this issue Apr 7, 2018 · 12 comments
Closed

fs.lchown should be undeprecated #19868

simevo opened this issue Apr 7, 2018 · 12 comments
Labels
fs Issues and PRs related to the fs subsystem / file system.

Comments

@simevo
Copy link

simevo commented Apr 7, 2018

fs.lchown is currently deprecated

But fs.lchown can be helpful to address Time of check to time of use (TOCTOU) bugs, like this one,
as it does not dereference symbolic links and merely changes the owner of the link.

Please undeprecate fs.lchown and implement it on the linux platform.
@ryzokuken
Copy link
Contributor

@nodejs/fs thoughts on this?

@simevo you sure there's no alternative?

@addaleax addaleax added the fs Issues and PRs related to the fs subsystem / file system. label Apr 7, 2018
@addaleax
Copy link
Member

addaleax commented Apr 7, 2018

I’m surprised to hear it’s deprecated and only implemented on some platforms, tbh.

@ryzokuken
Copy link
Contributor

Strange. I'm equally surprised to hear that it's implemented on Windows and not on Linux. I mean, it's chown. How do you even not implement chown on linux? 😛

@addaleax that said, if we decide to go ahead and add this functionality, I'd love to take a shot at it.

@anliting
Copy link

anliting commented Apr 7, 2018

@ryzokuken

I don't think @simevo means it is implemented on Windows. As nodejs/node-v0.x-archive#7382, it is only implemented on macOS.

@ryzokuken
Copy link
Contributor

@anliting I just found that out myself, and needless to say, it makes a lot of sense.

Could we somehow implement this functionality on Windows too? I'd love if we could make this platform-independent somehow. Let me look into it.

@bnoordhuis
Copy link
Member

bnoordhuis commented Apr 7, 2018 •
edited
Loading

Related: #16695

I’m surprised to hear it’s deprecated and only implemented on some platforms, tbh.

There is no way to implement it with old Linux kernels: fchmodat() was added in 2.6.16 and it was until recently that we still still supported 2.6.9.

2.6.18 is the baseline now so that's no longer a blocker.

@ryzokuken
Copy link
Contributor

@bnoordhuis Sounds great!

Now that the baseline has moved, should someone start working on making it work on Linux? We'd probably have to un-deprecate the function before that though, wouldn't we?

Also, does that mean it'd be semver-major? Or would it be semver-minor because we're not really removing anything, just adding new functionality?

@bnoordhuis
Copy link
Member

Now that the baseline has moved, should someone start working on making it work on Linux?

Yes. It should be added to libuv first.

We'd probably have to un-deprecate the function before that though, wouldn't we?

No, the other way around. It was deprecated because it's platform-specific. It should be un-deprecated only when that's no longer true.

would it be semver-minor because we're not really removing anything, just adding new functionality?

Yep, I'd say so.

@ryzokuken
Copy link
Contributor

Great! I think @chris--young had been working on adding it to libuv? If there's no open PR regarding this in there, I could try looking into it, or submit an issue. That way, we could go with whichever way helps us ship this quicker.

@bnoordhuis
Copy link
Member

I don't remember any libuv PRs. A quick search doesn't turn up anything either.

@ryzokuken
Copy link
Contributor

@bnoordhuis in that case, I'd make an issue on libuv and start looking into it myself as well.

@ryzokuken ryzokuken mentioned this issue Apr 7, 2018
Closed
@bmacnaughton bmacnaughton mentioned this issue May 10, 2018
Merged
@simevo
Copy link
Author

simevo commented Jun 19, 2018

Hi please note that libuv now has lchown, see: libuv/libuv#1826 (comment)

@cjihrig cjihrig mentioned this issue Jun 24, 2018
Merged
4 tasks
cjihrig added a commit to cjihrig/node that referenced this issue Jun 25, 2018
@cjihrig
deps: upgrade to libuv 1.21.0
537a4ba 
Notable changes:

- Building via cmake is now supported.
  PR-URL: libuv/libuv#1850
- Stricter checks have been added to prevent watching the same
  file descriptor multiple times.
  PR-URL: libuv/libuv#1851
  Refs: nodejs#3604
- An IPC deadlock on Windows has been fixed.
  PR-URL: libuv/libuv#1843
  Fixes: nodejs#9706
  Fixes: nodejs#7657
- uv_fs_lchown() has been added.
  PR-URL: libuv/libuv#1826
  Refs: nodejs#19868
- uv_fs_copyfile() sets errno on error.
  PR-URL: libuv/libuv#1881
  Fixes: nodejs#21329
- uv_fs_fchmod() supports -A files on Windows.
  PR-URL: libuv/libuv#1819
  Refs: nodejs#12803

PR-URL: nodejs#21466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Jun 25, 2018
@cjihrig @targos
deps: upgrade to libuv 1.21.0
f162939 
Notable changes:

- Building via cmake is now supported.
  PR-URL: libuv/libuv#1850
- Stricter checks have been added to prevent watching the same
  file descriptor multiple times.
  PR-URL: libuv/libuv#1851
  Refs: #3604
- An IPC deadlock on Windows has been fixed.
  PR-URL: libuv/libuv#1843
  Fixes: #9706
  Fixes: #7657
- uv_fs_lchown() has been added.
  PR-URL: libuv/libuv#1826
  Refs: #19868
- uv_fs_copyfile() sets errno on error.
  PR-URL: libuv/libuv#1881
  Fixes: #21329
- uv_fs_fchmod() supports -A files on Windows.
  PR-URL: libuv/libuv#1819
  Refs: #12803

PR-URL: #21466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
cjihrig added a commit to cjihrig/node that referenced this issue Jun 27, 2018
@cjihrig
fs: undeprecate lchown()
7ff50f9 
uv_fs_lchown() exists, as of libuv 1.21.0. fs.lchown() can now
be undeprecated. This commit also adds tests, as there were
none.

PR-URL: nodejs#21498
Fixes: nodejs#19868
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
targos pushed a commit that referenced this issue Jun 28, 2018
@cjihrig @targos
fs: undeprecate lchown()
2e07d45 
uv_fs_lchown() exists, as of libuv 1.21.0. fs.lchown() can now
be undeprecated. This commit also adds tests, as there were
none.

PR-URL: #21498
Fixes: #19868
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this issue Nov 5, 2018
@cjihrig @MylesBorins
deps: upgrade to libuv 1.21.0
4de5458 
Notable changes:

- Building via cmake is now supported.
  PR-URL: libuv/libuv#1850
- Stricter checks have been added to prevent watching the same
  file descriptor multiple times.
  PR-URL: libuv/libuv#1851
  Refs: nodejs#3604
- An IPC deadlock on Windows has been fixed.
  PR-URL: libuv/libuv#1843
  Fixes: nodejs#9706
  Fixes: nodejs#7657
- uv_fs_lchown() has been added.
  PR-URL: libuv/libuv#1826
  Refs: nodejs#19868
- uv_fs_copyfile() sets errno on error.
  PR-URL: libuv/libuv#1881
  Fixes: nodejs#21329
- uv_fs_fchmod() supports -A files on Windows.
  PR-URL: libuv/libuv#1819
  Refs: nodejs#12803

PR-URL: nodejs#21466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Nov 11, 2018
@cjihrig @MylesBorins
deps: upgrade to libuv 1.21.0
11cb09b 
Notable changes:

- Building via cmake is now supported.
  PR-URL: libuv/libuv#1850
- Stricter checks have been added to prevent watching the same
  file descriptor multiple times.
  PR-URL: libuv/libuv#1851
  Refs: #3604
- An IPC deadlock on Windows has been fixed.
  PR-URL: libuv/libuv#1843
  Fixes: #9706
  Fixes: #7657
- uv_fs_lchown() has been added.
  PR-URL: libuv/libuv#1826
  Refs: #19868
- uv_fs_copyfile() sets errno on error.
  PR-URL: libuv/libuv#1881
  Fixes: #21329
- uv_fs_fchmod() supports -A files on Windows.
  PR-URL: libuv/libuv#1819
  Refs: #12803

Backport-PR-URL: #24103
PR-URL: #21466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@bnoordhuis @addaleax @anliting @simevo @ryzokuken