fix: ftruncate for negative and maximum values

[IgorHalfeld]

fix: fs.ftruncate and fd.ftruncateSync both silently ignore negative offsets

Fixes #35632

Checklist

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

[addaleax]

I would use ERR_INVALID_ARG_VALUE here – it’s not the file that has a negative length, it is an invalid argument to the function here.

Likewise, using kIoMaxLength/ERR_FS_FILE_TOO_LARGE seems a bit odd here. The 2 GB limit is because that is the maximum buffer size supported, but Node.js generally does support way larger files than that (just not reading or writing to them in multi-GB chunks). We should not fail just because len exceeds this smaller limit, and only fail if it exceeds the actual limit after which the operation would fail or do the wrong thing.

[IgorHalfeld]

Makes sense!

So, that should solve

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

I'll update the PR

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[aduh95]

@IgorHalfeld can you drop the merge commit and rebase please? Merge commits break our tooling.

[gireeshpunathil]

@IgorHalfeld - this needs a rebase

[IgorHalfeld]

Done 🎉

[aduh95]

@IgorHalfeld can you rebase rather than using merge commits please?

git fetch upstream master
git rebase upstream/master -i

Then you select the commits you want to keep (all of them except the merge commits), fix git conflicts, and force push to your fix-ftruncate branch.

The reason this is needed is because the CI downloads the patch from GitHub and tries to apply it on top of the master branch. This fails when trying to apply d9e745c.

[IgorHalfeld]

any updates?

[RaisinTen]

@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

My suggestion was very architecture specific.

[aduh95]

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);

[aduh95]

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);

[aduh95]

Suggested change

	code: 'ERR_INVALID_ARG_VALUE',
	code: 'ERR_OUT_OF_RANGE',

[RaisinTen]

Thank you very much for the contribution.
An alt solution for the related issue has already landed in #37483 so, I'm closing this PR.
Feel free to reopen if you still feel like continuing your work on this.