fs: fix reads with pos > 4GB #21003
fs: fix reads with pos > 4GB #21003
Conversation
nodejs-github-bot
added
the
fs
|
This applies to |
|
@addaleax let me update it |
|
added fix for readSync also |
lib/fs.js
Outdated
| @@ -489,7 +489,7 @@ function readSync(fd, buffer, offset, length, position) { | |||
|
|
|||
| validateOffsetLengthRead(offset, length, buffer.length); | |||
|
|
|||
| if (!isUint32(position)) | |||
| if (!Number.isNumber(position)) | |||
lib/fs.js
Outdated
| @@ -459,7 +459,7 @@ function read(fd, buffer, offset, length, position, callback) { | |||
|
|
|||
| validateOffsetLengthRead(offset, length, buffer.length); | |||
|
|
|||
| if (!isUint32(position)) | |||
| if (!Number.isInteger(position)) | |||
There was a problem hiding this comment.
You probably want isSafeInteger, since non-safe integers are not guaranteed to represent what the user wants (and are thus invalid). We also use isSafeInteger in the validateInteger validator.
There was a problem hiding this comment.
Sure, just putting what was in there before
|
Shouldn't it be possible to test this without large/sparse files by testing whether the large position argument reads from the current position or doesn't read anything? For example: 'use strict';
const fs = require('fs');
const assert = require('assert');
const fd = fs.openSync(__filename, 'r');
const nRead = fs.readSync(fd, Buffer.alloc(1), 0, 1, 900719925474099);
assert.strictEqual(nRead, 0); |
|
@mscdex that's a nice compromise 👍 |
mafintosh
force-pushed
the
fs-read-fix
branch
2 times, most recently
from
9c35f69
to
fbcbb4b
Compare
|
Added a test for both read and readSync |
|
I’m okay with fast-tracking as well, but in that case it’s a good idea to open an issue immediately afterwards for the fact that we want a (proper) regression test. |
test/parallel/test-fs-read.js
Outdated
| @@ -53,3 +53,15 @@ test(Buffer.allocUnsafe(expected.length), | |||
| test(new Uint8Array(expected.length), | |||
| new Uint8Array(expected.length), | |||
| Uint8Array.from(expected)); | |||
|
|
|||
| { | |||
| // Reading > 4gb should return no data | |||
There was a problem hiding this comment.
This code comment is a little confusing because it's merely reading past the end of file that results in no data. That can happen no matter the file size.
Perhaps it should instead mention that it's testing whether >32-bit position arguments read from the absolute (good) or current (bad) position and that reading beyond the file is what should return no data.
test/parallel/test-fs-read.js
Outdated
|
|
||
| { | ||
| // Reading beyond file length (3 in this case) should return no data. | ||
| // This is a test for a bug where reads > uin32 would return |
test/parallel/test-fs-read.js
Outdated
| { | ||
| // Reading beyond file length (3 in this case) should return no data. | ||
| // This is a test for a bug where reads > uin32 would return | ||
| // the start of the file. |
There was a problem hiding this comment.
s/the start of the file/data from the current position in the file/
There was a problem hiding this comment.
it seems to be the start of the file
|
I think the test will need to be isolated (a separate |
|
@mscdex you sure? the test fails for me if running it on node 10 |
|
I forgot that specifying absolute file positions does not change the current file pointer, so that probably sounds right. |
Notable Changes:
* **deps**:
- upgrade npm to 6.1.0 (Rebecca Turner)
#20190
* **fs**:
- fix reads with pos \> 4GB (Mathias Buus)
#21003
* **net**:
- new option to allow IPC servers to be readable and writable
by all users (Bartosz Sosnowski)
#19472
* **stream**:
- fix removeAllListeners() for Stream.Readable to work as expected
when no arguments are passed (Kael Zhang)
#20924
* **Added new collaborators**
- John-David Dalton (https://github.com/jdalton)
PR-URL: #21011
Notable Changes:
* **deps**:
- upgrade npm to 6.1.0 (Rebecca Turner)
#20190
* **fs**:
- fix reads with pos \> 4GB (Mathias Buus)
#21003
* **net**:
- new option to allow IPC servers to be readable and writable
by all users (Bartosz Sosnowski)
#19472
* **stream**:
- fix removeAllListeners() for Stream.Readable to work as expected
when no arguments are passed (Kael Zhang)
#20924
* **Added new collaborators**
- John-David Dalton (https://github.com/jdalton)
PR-URL: #21011
Notable Changes:
* **deps**:
- upgrade npm to 6.1.0 (Rebecca Turner)
#20190
* **fs**:
- fix reads with pos \> 4GB (Mathias Buus)
#21003
* **net**:
- new option to allow IPC servers to be readable and writable
by all users (Bartosz Sosnowski)
#19472
* **stream**:
- fix removeAllListeners() for Stream.Readable to work as expected
when no arguments are passed (Kael Zhang)
#20924
* **Added new collaborators**
- John-David Dalton (https://github.com/jdalton)
PR-URL: #21011
PR-URL: nodejs#21148 Fixes: nodejs#21121 Refs: nodejs#21003 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Backport of nodejs/node#21003; fixed after 10.3.0 so this does not also need to be applied to the 10.6.0 upgrade
Backport to nodejs/node#21003; fixed after 10.3.0 so this does not also need to be applied to the 10.6.0 upgrade
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesThis fixes an issue in node 10 where all file reads with position > 4GB returns the data stored at position 0 in the file.
I would appreciate a quick review/release on this as this can have pretty serious consequences obvs (I just corrupted some local databases because of it).
A test case is available here: https://gist.github.com/mafintosh/1f9adf37bbc7ea05f1d2da6ab2d0f7a1
I'm happy to add it but not sure if we expect the tests to run on file systems that all support sparse files. Otherwise it would be a very slow test case.