Skip to content

fs: fix chown abort#38004

Closed
RaisinTen wants to merge 5 commits intonodejs:masterfrom
RaisinTen:fs/fix-chown-abort
Closed

fs: fix chown abort#38004
RaisinTen wants to merge 5 commits intonodejs:masterfrom
RaisinTen:fs/fix-chown-abort

Conversation

@RaisinTen
Copy link
Member

@RaisinTen RaisinTen commented Mar 31, 2021

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

@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
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Mar 31, 2021
@RaisinTen RaisinTen mentioned this pull request Mar 31, 2021
Closed
aduh95
aduh95 reviewed Mar 31, 2021
View reviewed changes
lib/internal/fs/promises.js
const kWriteFileMaxChunkSize = 512 * 1024;

// 2 ** 32 - 1
const kMaxUserId = 4294967295;
Copy link
Contributor

@aduh95 aduh95 Mar 31, 2021

Choose a reason for hiding this comment

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

Why this value? Is this documented somewhere? Could you add a comment explaining where does it come from please?

Copy link
Member Author

@RaisinTen RaisinTen Mar 31, 2021

Choose a reason for hiding this comment

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

I actually pasted it from:

node/lib/fs.js

Lines 139 to 140 in 82bc5c3

// 2 ** 32 - 1
const kMaxUserId = 4294967295;

since I could not require it because of a circular dependency.
Should we move this (and possibly other k* constants) into lib/internal/fs/utils.js and require them from there?

@benjamingr
Copy link
Member

benjamingr commented Mar 31, 2021

Nice, though looks like #37997 was opened already?

@ycjcl868
Copy link
Contributor

ycjcl868 commented Apr 1, 2021

CI broken

@ycjcl868 ycjcl868 mentioned this pull request Apr 1, 2021
Closed
@RaisinTen
Copy link
Member Author

RaisinTen commented Apr 1, 2021

Nice, though looks like #37997 was opened already?

@benjamingr I opened this as it has a different approach of fixing the issue. Also, #37997 (comment).

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 1, 2021

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

@RaisinTen
Copy link
Member Author

RaisinTen commented Apr 1, 2021

CI broken

@ycjcl868 The Github Actions CI seems to be fixed now. Started the Jenkins CI run.

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 2, 2021
@Trott
Copy link
Member

Trott commented Apr 3, 2021

Landed in e9ed113

@Trott Trott closed this Apr 3, 2021
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>
@RaisinTen RaisinTen deleted the fs/fix-chown-abort branch April 3, 2021 14:35
RaisinTen added a commit to RaisinTen/node that referenced this pull request Apr 3, 2021
@RaisinTen
fs: move constants to internal/fs/utils.js
fcdbf00 
Refs: nodejs#38004 (comment)
@RaisinTen RaisinTen mentioned this pull request Apr 3, 2021
Closed
@RA80533 RA80533 mentioned this pull request Apr 4, 2021
Closed
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 MylesBorins mentioned this pull request Apr 4, 2021
Merged
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>
jasnell pushed a commit that referenced this pull request Apr 6, 2021
@RaisinTen @jasnell
fs: move constants to internal/fs/utils.js
24fd791 
Refs: #38004 (comment)

PR-URL: #38061
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: James M Snell <jasnell@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>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
MoritzLoewenstein pushed a commit to MoritzLoewenstein/node that referenced this pull request Aug 21, 2021
@RaisinTen @MoritzLoewenstein
fs: move constants to internal/fs/utils.js
f90fd11 
Refs: nodejs#38004 (comment)

PR-URL: nodejs#38061
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MoritzLoewenstein pushed a commit to MoritzLoewenstein/node that referenced this pull request Aug 21, 2021
@RaisinTen @MoritzLoewenstein
fs: move constants to internal/fs/utils.js
0ecc292 
Refs: nodejs#38004 (comment)

PR-URL: nodejs#38061
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MoritzLoewenstein pushed a commit to MoritzLoewenstein/node that referenced this pull request Aug 25, 2021
@RaisinTen @MoritzLoewenstein
fs: move constants to internal/fs/utils.js
8dc76e0 
Refs: nodejs#38004 (comment)

PR-URL: nodejs#38061
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MoritzLoewenstein pushed a commit to MoritzLoewenstein/node that referenced this pull request Sep 1, 2021
@RaisinTen @MoritzLoewenstein
fs: move constants to internal/fs/utils.js
07d5dd2 
Refs: nodejs#38004 (comment)

PR-URL: nodejs#38061
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 4, 2021
@RaisinTen @targos
fs: move constants to internal/fs/utils.js
a4d6f78 
Refs: #38004 (comment)

PR-URL: #38061
Backport-PR-URL: #39838
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@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

Reviewers

@aduh95 aduh95 aduh95 left review comments

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax 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. c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

"fs.chownSync" results in an abort

7 participants

@RaisinTen @benjamingr @ycjcl868 @nodejs-github-bot @Trott @addaleax @aduh95