fs.readSync & fs.read position argument does not support BigInt

[mikeal]

• Version: v14.10.1
• Platform: Linux edef72861714 4.14.152-127.182.amzn2.x86_64 deps: update openssl to 1.0.1j #1 SMP Thu Nov 14 17:32:43 UTC 2019 x86_64 GNU/Linux

What steps will reproduce the bug?

readSync(reader, buffer, 0, 12, BigInt(position))           

How often does it reproduce? Is there a required condition?

Everytime.

What is the expected behavior?

It should read from the position in the file, and it will if you use Number(position) on a BigInt position.

What do you see instead?

The call reads data into the buffer but it begins at position 0 in the file instead of at the position noted in the BigInt.

[mikeal]

Also happens w/ fs.read

[benjamingr]

This is the same behaviour fs.read and fs.readSync have with anything that is not a Number or fails Number.isSafeInteger.

I think this is a reasonable improvement. Though it might be misleading since people may assume fs.read will work with offsets larger than integers which it doesn't.

I think a fix would be to:

• go here: https://github.com/nodejs/node/blob/master/lib/fs.js#L546-L547 and https://github.com/nodejs/node/blob/master/lib/fs.js#L598-L599 for the sync version.
• Add a check if the typeof of the argument is a BigInt, if it is, coerce it to a Number and pass it to the same IsSafeInteger check.
• Add a test to test-fs-read in test/parallel.
• Update the docs in fs.md with a YAML comment indicating that fs.read and fs.readSync now potentially accept a BigInt (I think the actual docs don't need an update probably?)

A more elaborate fix would be to try and get reads to work with position > Int32, which would require:

• Going to node_file.cc static void Read(const FunctionCallbackInfo<Value>& args)
• Remove the IsSafeJsInt check in line 2039
• Instead of reading the value as Integer in line 2040, check IsBigInt() and if so read it as a BigInt (an int64 is probably still fine for most cases since a 64 bit offset into a file is 9,223,372,036,854,775,807 bytes big which is still quite theoretical I believe).
• uv_fs_read is already being passed a 64 bit integer so the actual offset itself should work.

[BridgeAR]

Instead of coercing the value to a number, I'd in fact throw an error. That way there's just a single way to do this and the immediate feedback should be fine to let the user handle the coercing on their own or to use a number instead.

[darvesh]

@BridgeAR Shall I work on this?
If I'm right I have to go here: https://github.com/nodejs/node/blob/master/lib/fs.js#L546-L547 and https://github.com/nodejs/node/blob/master/lib/fs.js#L598-L599 and throw error if typeof position === "bigint" then add a test to test-fs-read in test/parallel.

[mikeal]

Though it might be misleading since people may assume fs.read will work with offsets larger than integers which it doesn't.

Why you gotta go and make my max file size 9 petabytes! 😭 This is so unreasonable 🤬

j/k

[Trott]

(Removed good first issue because there are at least two open PRs that address this.)

[jasnell]

Given that it is perfectly fine to have bigints well within the acceptable range, I'd say that accepting a BigInt should be fine. We can throw a RangeError if too large of a bigint is provided.

[kaizhu256]

if bigint is allowed, would bigdecimal also be allowed if it hypothetically reaches stage 4?

[benjamingr]

if bigint is allowed, would bigdecimal also be allowed if it hypothetically reaches stage 4?

BigDecimal is currently stage 1, very early in the stage process. I say one bridge at a time.