fs.ftruncate silently accepts negative offsets rather than failing with ERR_INVALID

[sbc100]

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

node/lib/fs.js

Line 840 in 2cfdf28

len = MathMax(0, len);

This didn't always do behave like this.. looks like it was introduced in 8974df1

What steps will reproduce the bug?

var fs = require("fs");
var fd = fs.openSync("f", 'w+');
console.log(fs.ftruncateSync(fd, -99));

What is the expected behavior?

The ftruncate POSIX function is described as returning EINVAL when given a negative offset:
https://linux.die.net/man/2/ftruncate

In emscripten we emulate a POSIX environment on top of the Web and on top of node and expect ftruncate to fail in the same way.

We can obviously add a check to our code as a workaround but this does seem like a bug in node.

What do you see instead?

Silently assumed 0 length is what the caller really wants.

[Narasimha1997]

Maybe removing len = MathMax(0, len); would fix the issue, or by explicitly handling the negative value case before calling the internal method. Had experienced this issue, a week back ! I solved this by explicitly checking the len before passing it down to the ftruncateSync()

[RaisinTen]

What about adding this sort of a change?

-  len = MathMax(0, len);
+  if (len < 0) {
+    throw new ERR_FS_FILE_TOO_SMALL(len);
+  } else if (len > kIoMaxLength) {
+    throw new ERR_FS_FILE_TOO_LARGE(len);
+  }

where, ERR_FS_FILE_TOO_LARGE already exists and we make a new error code ERR_FS_FILE_TOO_SMALL for negative sizes and this is all about kIoMaxLength:

node/lib/fs.js

Lines 27 to 29 in 999e7d7

	// Most platforms don't allow reads or writes >= 2 GB.
	// See https://github.com/libuv/libuv/pull/1501.
	const kIoMaxLength = 2 ** 31 - 1;

This would take care of both the upper and the lower limits for the allowed size in ftruncate.

[sbc100]

I'm not familiar with node code but isn't there already the simple equivalent of EINVAL? Is it a good idea to add more error codes?

[Narasimha1997]

Node 8.xx.xx throws EINVAL properly for negative offset. Maybe there is no need to handle explicitly. Even len = MathMax(0, len) is not required. The internal system call itself terminates with EINVAL when it sees negative offset. So no need to handle anything explicitly. If you check the source code of 8.xx.xx there is no explicit validation being done.

[jasnell]

PR: #37483