Skip to content

fix: ftruncate for negative and maximum values #35645

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

Closed
IgorHalfeld wants to merge 6 commits into from
Closed

fix: ftruncate for negative and maximum values #35645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
41ca10a
fix ftruncate for negative and maximum values
IgorHalfeld Oct 14, 2020
9694783
put same validation for async ftruncate
IgorHalfeld Oct 14, 2020
e861776
remove unused var MathMax from lib/fs.js
IgorHalfeld Oct 14, 2020
7688f6c
change error type when parameter is not valid for ftruncate
IgorHalfeld Oct 14, 2020
ffb1861
put required arg on ERR_INVALID_ARG_VALUE
IgorHalfeld Oct 15, 2020
e04fd9b
fix tests for ftruncate
IgorHalfeld Oct 15, 2020
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
13 changes: 10 additions & 3 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ const kIoMaxLength = 2 ** 31 - 1;
const {
ArrayPrototypePush,
BigIntPrototypeToString,
MathMax,
Number,
NumberIsSafeInteger,
ObjectCreate,
Expand Down Expand Up @@ -845,7 +844,11 @@ function ftruncate(fd, len = 0, callback) {
}
validateInt32(fd, 'fd', 0);
validateInteger(len, 'len');
len = MathMax(0, len);

if (len < 0) {
throw new ERR_INVALID_ARG_VALUE('len', len);
}

Comment on lines 846 to +851
Copy link
Contributor

@aduh95 aduh95 Feb 19, 2021

Choose a reason for hiding this comment

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

Shouldn't we throw a ERR_OUT_OF_RANGE instead?

Suggested change
validateInteger(len, 'len');
len = MathMax(0, len);
if (len < 0) {
throw new ERR_INVALID_ARG_VALUE('len', len);
}
validateInteger(len, 'len', 0);

callback = makeCallback(callback);

const req = new FSReqCallback();
Expand All @@ -856,7 +859,11 @@ function ftruncate(fd, len = 0, callback) {
function ftruncateSync(fd, len = 0) {
validateInt32(fd, 'fd', 0);
validateInteger(len, 'len');
len = MathMax(0, len);

if (len < 0) {
throw new ERR_INVALID_ARG_VALUE('len', len);
}

Comment on lines 861 to +866
Copy link
Contributor

@aduh95 aduh95 Feb 19, 2021

Choose a reason for hiding this comment

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

Shouldn't we throw a ERR_OUT_OF_RANGE instead?

Suggested change
validateInteger(len, 'len');
len = MathMax(0, len);
if (len < 0) {
throw new ERR_INVALID_ARG_VALUE('len', len);
}
validateInteger(len, 'len', 0);

const ctx = {};
binding.ftruncate(fd, len, undefined, ctx);
handleErrorFromBinding(ctx);
Expand Down
15 changes: 8 additions & 7 deletions test/parallel/test-fs-truncate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,13 +224,14 @@ function testFtruncate(cb) {
}

{
const file6 = path.resolve(tmp, 'truncate-file-6.txt');
fs.writeFileSync(file6, 'Hi');
const fd = fs.openSync(file6, 'r+');
process.on('beforeExit', () => fs.closeSync(fd));
fs.ftruncate(fd, -1, common.mustSucceed(() => {
assert(fs.readFileSync(file6).equals(Buffer.from('')));
}));
assert.throws(
() => fs.ftruncate(fd, -1),
{
code: 'ERR_INVALID_ARG_VALUE',
Copy link
Contributor

@aduh95 aduh95 Feb 19, 2021

Choose a reason for hiding this comment

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

Suggested change
code: 'ERR_INVALID_ARG_VALUE',
code: 'ERR_OUT_OF_RANGE',

name: /(TypeError|RangeError)/,
message: 'The argument \'len\' is invalid. Received -1'
}
);
Comment on lines +227 to +234
Copy link
Member

@RaisinTen RaisinTen Jan 17, 2021

Choose a reason for hiding this comment

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

@IgorHalfeld The fd you have used here is already closed by the time this is run but the test passes because ERR_INVALID_ARG_VALUE is thrown earlier:

node/test/parallel/test-fs-truncate.js

Line 71 in 7efada6

fs.closeSync(fd);

Instead of replacing the previous test completely, consider replacing only this part with your test (you may take a look at the other tests for help):

node/test/parallel/test-fs-truncate.js

Lines 231 to 233 in 7efada6

fs.ftruncate(fd, -1, common.mustSucceed(() => {
assert(fs.readFileSync(file6).equals(Buffer.from('')));
}));

Also, fs.ftruncate needs to be called with a callback: https://nodejs.org/api/fs.html#fs_fs_ftruncate_fd_len_callback

Similarly, consider adding a corresponding test for fs.ftruncateSync too: https://nodejs.org/api/fs.html#fs_fs_ftruncatesync_fd_len

}

{
Expand Down