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

lib: unmask mode_t values with 0o777 #20975

Closed
wants to merge 1 commit into from
Closed

lib: unmask mode_t values with 0o777 #20975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions doc/api/fs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1089,6 +1089,12 @@ For example, the octal value `0o765` means:
* The group may read and write the file.
* Others may read and execute the file.

Note: When using raw numbers where file modes are expected,
any value larger than `0o777` may result in platform-specific
behaviors that are not supported to work consistently.
Therefore constants like `S_ISVTX`, `S_ISGID` or `S_ISUID` are
not exposed in `fs.constants`.

Caveats: on Windows only the write permission can be changed, and the
distinction among the permissions of group, owner or others is not
implemented.
Expand Down
18 changes: 9 additions & 9 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ const {
} = require('internal/constants');
const {
isUint32,
validateAndMaskMode,
validateMode,
validateInteger,
validateInt32,
validateUint32
Expand Down Expand Up @@ -416,7 +416,7 @@ function open(path, flags, mode, callback) {
callback = makeCallback(mode);
mode = 0o666;
} else {
mode = validateAndMaskMode(mode, 'mode', 0o666);
mode = validateMode(mode, 'mode', 0o666);
callback = makeCallback(callback);
}

Expand All @@ -434,7 +434,7 @@ function openSync(path, flags, mode) {
path = getPathFromURL(path);
validatePath(path);
const flagsNumber = stringToFlags(flags);
mode = validateAndMaskMode(mode, 'mode', 0o666);
mode = validateMode(mode, 'mode', 0o666);

const ctx = { path };
const result = binding.open(pathModule.toNamespacedPath(path),
Expand Down Expand Up @@ -721,7 +721,7 @@ function mkdir(path, mode, callback) {
mode = 0o777;
} else {
callback = makeCallback(callback);
mode = validateAndMaskMode(mode, 'mode', 0o777);
mode = validateMode(mode, 'mode', 0o777);
}

const req = new FSReqWrap();
Expand All @@ -732,7 +732,7 @@ function mkdir(path, mode, callback) {
function mkdirSync(path, mode) {
path = getPathFromURL(path);
validatePath(path);
mode = validateAndMaskMode(mode, 'mode', 0o777);
mode = validateMode(mode, 'mode', 0o777);
const ctx = { path };
binding.mkdir(pathModule.toNamespacedPath(path), mode, undefined, ctx);
handleErrorFromBinding(ctx);
Expand Down Expand Up @@ -915,7 +915,7 @@ function unlinkSync(path) {

function fchmod(fd, mode, callback) {
validateInt32(fd, 'fd', 0);
mode = validateAndMaskMode(mode, 'mode');
mode = validateMode(mode, 'mode');
callback = makeCallback(callback);

const req = new FSReqWrap();
Expand All @@ -925,7 +925,7 @@ function fchmod(fd, mode, callback) {

function fchmodSync(fd, mode) {
validateInt32(fd, 'fd', 0);
mode = validateAndMaskMode(mode, 'mode');
mode = validateMode(mode, 'mode');
const ctx = {};
binding.fchmod(fd, mode, undefined, ctx);
handleErrorFromBinding(ctx);
Expand Down Expand Up @@ -966,7 +966,7 @@ function lchmodSync(path, mode) {
function chmod(path, mode, callback) {
path = getPathFromURL(path);
validatePath(path);
mode = validateAndMaskMode(mode, 'mode');
mode = validateMode(mode, 'mode');
callback = makeCallback(callback);

const req = new FSReqWrap();
Expand All @@ -977,7 +977,7 @@ function chmod(path, mode, callback) {
function chmodSync(path, mode) {
path = getPathFromURL(path);
validatePath(path);
mode = validateAndMaskMode(mode, 'mode');
mode = validateMode(mode, 'mode');

const ctx = { path };
binding.chmod(pathModule.toNamespacedPath(path), mode, undefined, ctx);
Expand Down
10 changes: 5 additions & 5 deletions lib/internal/fs/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const {
} = require('internal/fs/utils');
const {
isUint32,
validateAndMaskMode,
validateMode,
validateInteger,
validateUint32
} = require('internal/validators');
Expand Down Expand Up @@ -192,7 +192,7 @@ async function copyFile(src, dest, flags) {
async function open(path, flags, mode) {
path = getPathFromURL(path);
validatePath(path);
mode = validateAndMaskMode(mode, 'mode', 0o666);
mode = validateMode(mode, 'mode', 0o666);
return new FileHandle(
await binding.openFileHandle(pathModule.toNamespacedPath(path),
stringToFlags(flags),
Expand Down Expand Up @@ -287,7 +287,7 @@ async function fsync(handle) {
async function mkdir(path, mode) {
path = getPathFromURL(path);
validatePath(path);
mode = validateAndMaskMode(mode, 'mode', 0o777);
mode = validateMode(mode, 'mode', 0o777);
return binding.mkdir(pathModule.toNamespacedPath(path), mode, kUsePromises);
}

Expand Down Expand Up @@ -359,14 +359,14 @@ async function unlink(path) {

async function fchmod(handle, mode) {
validateFileHandle(handle);
mode = validateAndMaskMode(mode, 'mode');
mode = validateMode(mode, 'mode');
return binding.fchmod(handle.fd, mode, kUsePromises);
}

async function chmod(path, mode) {
path = getPathFromURL(path);
validatePath(path);
mode = validateAndMaskMode(mode, 'mode');
mode = validateMode(mode, 'mode');
return binding.chmod(pathModule.toNamespacedPath(path), mode, kUsePromises);
}

Expand Down
4 changes: 2 additions & 2 deletions lib/internal/process/methods.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const {
ERR_UNKNOWN_CREDENTIAL
} = require('internal/errors').codes;
const {
validateAndMaskMode,
validateMode,
validateUint32
} = require('internal/validators');

Expand Down Expand Up @@ -35,7 +35,7 @@ function setupProcessMethods() {
// Get the mask
return _umask(mask);
}
mask = validateAndMaskMode(mask, 'mask');
mask = validateMode(mask, 'mask');
return _umask(mask);
}
}
Expand Down
23 changes: 17 additions & 6 deletions lib/internal/validators.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,22 @@ function isUint32(value) {

const octalReg = /^[0-7]+$/;
const modeDesc = 'must be a 32-bit unsigned integer or an octal string';
// Validator for mode_t (the S_* constants). Valid numbers or octal strings
// will be masked with 0o777 to be consistent with the behavior in POSIX APIs.
function validateAndMaskMode(value, name, def) {

/**
* Validate values that will be converted into mode_t (the S_* constants).
* Only valid numbers and octal strings are allowed. They could be converted
* to 32-bit unsigned integers or non-negative signed integers in the C++
* land, but any value higher than 0o777 will result in platform-specific
* behaviors.
*
* @param {*} value Values to be validated
* @param {string} name Name of the argument
* @param {number} def If specified, will be returned for invalid values
* @returns {number}
*/
function validateMode(value, name, def) {
if (isUint32(value)) {
return value & 0o777;
return value;
}

if (typeof value === 'number') {
Expand All @@ -37,7 +48,7 @@ function validateAndMaskMode(value, name, def) {
throw new ERR_INVALID_ARG_VALUE(name, value, modeDesc);
}
const parsed = parseInt(value, 8);
return parsed & 0o777;
return parsed;
}

// TODO(BridgeAR): Only return `def` in case `value == null`
Expand Down Expand Up @@ -106,7 +117,7 @@ function validateUint32(value, name, positive) {
module.exports = {
isInt32,
isUint32,
validateAndMaskMode,
validateMode,
validateInteger,
validateInt32,
validateUint32
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-fs-chmod-mask.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

// This tests that mode > 0o777 will be masked off with 0o777 in fs APIs.
// This tests that the lower bits of mode > 0o777 still works in fs APIs.

const common = require('../common');
const assert = require('assert');
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-fs-mkdir-mode-mask.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

// This tests that mode > 0o777 will be masked off with 0o777 in fs.mkdir().
// This tests that the lower bits of mode > 0o777 still works in fs.mkdir().

const common = require('../common');
const assert = require('assert');
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-fs-open-mode-mask.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

// This tests that mode > 0o777 will be masked off with 0o777 in fs.open().
// This tests that the lower bits of mode > 0o777 still works in fs.open().

const common = require('../common');
const assert = require('assert');
Expand Down
5 changes: 2 additions & 3 deletions test/parallel/test-fs-promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,8 @@ function verifyStatObject(stat) {
await chmod(dest, 0o666);
await handle.chmod(0o666);

// Mode larger than 0o777 should be masked off.
await chmod(dest, (0o777 + 1));
await handle.chmod(0o777 + 1);
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be nice to check 0o777 + 1 as well (I guess it throws?).

Copy link
Member Author

@joyeecheung joyeecheung May 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@BridgeAR It fails on FreeBSD, that turns the bits into 0o1000 which is S_ISVTX and throws an unknown error. I am not entirely sure why it fails since it's 0o666 before that but not supporting it is kind of the point of the PR

await chmod(dest, (0o10777));
await handle.chmod(0o10777);

await utimes(dest, new Date(), new Date());

Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-process-umask-mask.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

// This tests that mask > 0o777 will be masked off with 0o777 in
// This tests that the lower bits of mode > 0o777 still works in
// process.umask()

const common = require('../common');
Expand Down