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: fix valid id range on chown, lchown, fchown #31694

Closed
wants to merge 1 commit into from
Closed

fs: fix valid id range on chown, lchown, fchown #31694

wants to merge 1 commit into from

Conversation

himself65
Copy link
Member

@himself65 himself65 commented Feb 8, 2020 •
edited
Loading

fix #31673

related:
http://man7.org/linux/man-pages/man2/chown.2.html

If the owner or group is specified as -1, then that ID is not changed.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Feb 8, 2020
@himself65 himself65 force-pushed the 31673 branch 3 times, most recently from 5deeef9 to a19201f Compare February 8, 2020 14:29
addaleax
addaleax approved these changes Feb 8, 2020
View reviewed changes
test/parallel/test-fs-fchown.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29025/

BridgeAR
BridgeAR approved these changes Feb 9, 2020
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality wise it's LGTM but the code could be simplified.

lib/internal/validators.js Outdated Show resolved Hide resolved
@himself65 himself65 force-pushed the 31673 branch 3 times, most recently from 5802f9b to 01df5b3 Compare February 9, 2020 15:14
jasnell
jasnell reviewed Feb 10, 2020
View reviewed changes
lib/fs.js Outdated Show resolved Hide resolved
addaleax
addaleax reviewed Feb 10, 2020
View reviewed changes
lib/fs.js Outdated Show resolved Hide resolved
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 13, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29114/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29614/

addaleax pushed a commit that referenced this pull request Mar 9, 2020
@himself65 @addaleax
fs: fix valid id range on chown, lchown, fchown
0a539dd 
PR-URL: #31694
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
@addaleax
Copy link
Member

addaleax commented Mar 9, 2020

Landed in 0a539dd

@addaleax addaleax closed this Mar 9, 2020
MylesBorins pushed a commit that referenced this pull request Mar 9, 2020
@himself65 @MylesBorins
fs: fix valid id range on chown, lchown, fchown
c106a85 
PR-URL: #31694
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 17, 2020
@himself65 @codebytere
fs: fix valid id range on chown, lchown, fchown
3ce4db2 
PR-URL: #31694
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 23, 2020
@himself65 @codebytere
fs: fix valid id range on chown, lchown, fchown
81f7a67 
PR-URL: #31694
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 30, 2020
@himself65 @codebytere
fs: fix valid id range on chown, lchown, fchown
de5d162 
PR-URL: #31694
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
@RaisinTen RaisinTen mentioned this pull request Mar 31, 2021
Closed
RaisinTen added a commit to RaisinTen/node that referenced this pull request Mar 31, 2021
@RaisinTen
fs: fix chown abort
5402b26 
This syncs the type assertion introduced in the referenced PR in the C++
side. Since chown, lchown, and fchown can accept -1 as a value for uid
and gid, we should also accept signed integers from the JS side.

Fixes: nodejs#37995
Refs: nodejs#31694
@RaisinTen RaisinTen mentioned this pull request Mar 31, 2021
Closed
Trott pushed a commit that referenced this pull request Apr 3, 2021
@RaisinTen @Trott
fs: fix chown abort
e9ed113 
This syncs the type assertion introduced in the referenced PR in the C++
side. Since chown, lchown, and fchown can accept -1 as a value for uid
and gid, we should also accept signed integers from the JS side.

Fixes: #37995
Refs: #31694

PR-URL: #38004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 4, 2021
@RaisinTen @MylesBorins
fs: fix chown abort
dc46073 
This syncs the type assertion introduced in the referenced PR in the C++
side. Since chown, lchown, and fchown can accept -1 as a value for uid
and gid, we should also accept signed integers from the JS side.

Fixes: #37995
Refs: #31694

PR-URL: #38004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 5, 2021
@RaisinTen @MylesBorins
fs: fix chown abort
ad7e344 
This syncs the type assertion introduced in the referenced PR in the C++
side. Since chown, lchown, and fchown can accept -1 as a value for uid
and gid, we should also accept signed integers from the JS side.

Fixes: #37995
Refs: #31694

PR-URL: #38004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request May 1, 2021
@RaisinTen @targos
fs: fix chown abort
8598745 
This syncs the type assertion introduced in the referenced PR in the C++
side. Since chown, lchown, and fchown can accept -1 as a value for uid
and gid, we should also accept signed integers from the JS side.

Fixes: #37995
Refs: #31694

PR-URL: #38004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lundibundi lundibundi lundibundi left review comments

@addaleax addaleax addaleax approved these changes

@ZYSzys ZYSzys ZYSzys approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Chown not conform to glibc's chown
7 participants
@himself65 @nodejs-github-bot @addaleax @jasnell @BridgeAR @lundibundi @ZYSzys