fs: fix reads with pos > 4GB#21003

mafintosh · 2018-05-28T17:06:27Z

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

This 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.

addaleax · 2018-05-28T17:10:38Z

This applies to readSync as well, I think :/

mafintosh · 2018-05-28T17:14:56Z

@addaleax let me update it

mafintosh · 2018-05-28T17:17:36Z

added fix for readSync also

TimothyGu

Is it possible to write a test for this?

TimothyGu · 2018-05-28T17:19:54Z

lib/fs.js

isNumber? ;)

TimothyGu · 2018-05-28T17:23:01Z

lib/fs.js

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.

Sure, just putting what was in there before

mscdex · 2018-05-28T17:24:31Z

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';constfs=require('fs');constassert=require('assert');constfd=fs.openSync(__filename,'r');constnRead=fs.readSync(fd,Buffer.alloc(1),0,1,900719925474099);assert.strictEqual(nRead,0);

TimothyGu

Pending a test. Otherwise LGTM.

mafintosh · 2018-05-28T17:38:57Z

@mscdex that's a nice compromise 👍

mafintosh · 2018-05-28T17:41:25Z

Added a test for both read and readSync

mafintosh · 2018-05-28T17:43:05Z

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

jasnell

+1 for fast track

addaleax · 2018-05-28T18:04:21Z

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.

mscdex · 2018-05-28T18:06:58Z

test/parallel/test-fs-read.js

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.

tweaked the comment

mcollina

Lgtm. This should get info a release asap

mscdex · 2018-05-28T19:45:12Z

test/parallel/test-fs-read.js

s/uin32/uint32/

mscdex · 2018-05-28T19:45:44Z

test/parallel/test-fs-read.js

s/the start of the file/data from the current position in the file/

it seems to be the start of the file

nope you're right.

mscdex · 2018-05-28T19:47:57Z

I think the test will need to be isolated (a separate openSync()) from the other read tests in that file because the preceding tests will have read the entire file, meaning the read()/readSync() would always return 0 because the current file pointer is at the end of the file.

mafintosh · 2018-05-28T19:49:33Z

@mscdex you sure? the test fails for me if running it on node 10

mscdex · 2018-05-28T19:53:07Z

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

PR-URL: nodejs#21148Fixes: nodejs#21121 Refs: nodejs#21003 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>

PR-URL: #21148Fixes: #21121 Refs: #21003 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>

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

PR-URL: nodejs/node#21148Fixes: nodejs/node#21121 Refs: nodejs/node#21003 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label May 28, 2018

mafintosh force-pushed the fs-read-fix branch from d3c8285 to e18a19eCompare May 28, 2018 17:16

TimothyGu reviewed May 28, 2018
View reviewed changes

mafintosh force-pushed the fs-read-fix branch from e18a19e to bcc8f81Compare May 28, 2018 17:29

TimothyGu approved these changes May 28, 2018
View reviewed changes

addaleax approved these changes May 28, 2018
View reviewed changes

mafintosh force-pushed the fs-read-fix branch 2 times, most recently from 9c35f69 to fbcbb4bCompare May 28, 2018 17:41

cjihrig approved these changes May 28, 2018
View reviewed changes

jasnell approved these changes May 28, 2018
View reviewed changes

addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label May 28, 2018

mscdex reviewed May 28, 2018
View reviewed changes

mcollina approved these changes May 28, 2018
View reviewed changes

benjamingr approved these changes May 28, 2018
View reviewed changes

trivikr approved these changes May 28, 2018
View reviewed changes

lpinca approved these changes May 28, 2018
View reviewed changes

mafintosh force-pushed the fs-read-fix branch from fbcbb4b to e7c72ddCompare May 28, 2018 19:02

mscdex reviewed May 28, 2018
View reviewed changes

test/parallel/test-fs-read.js Outdated
Copy link
Contributor
mscdexMay 28, 2018
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/uin32/uint32/

mscdex reviewed May 28, 2018
View reviewed changes

mmarchini mentioned this pull request May 30, 2018
Flaky parallel/test-trace-events-fs-sync #21038
Closed

freiit mentioned this pull request Jun 4, 2018
fsPromise - read files from positions >4GB doesn't work #21121
Closed

cjihrig mentioned this pull request Jun 5, 2018
fs: fix promises reads with pos > 4GB #21148
Merged
3 tasks

codebytere mentioned this pull request Aug 15, 2018
Node api fs.readSync with a big position works not right electron/electron#13979
Closed

codebytere added a commit to electron/node that referenced this pull request Aug 15, 2018
fix: fs.readSync with large position value
c86e3d3
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

codebytere added a commit to electron/node that referenced this pull request Aug 15, 2018
fix: fs.readSync with large position value
c3c62e0
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

Uh oh!

fs: fix reads with pos > 4GB#21003

fs: fix reads with pos > 4GB #21003

Uh oh!

Conversation

mafintosh commented May 28, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

addaleax commented May 28, 2018

Uh oh!

mafintosh commented May 28, 2018

Uh oh!

mafintosh commented May 28, 2018

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented May 28, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

mafintosh commented May 28, 2018

Uh oh!

mafintosh commented May 28, 2018

Uh oh!

mafintosh commented May 28, 2018

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented May 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented May 28, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mafintosh commented May 28, 2018

Uh oh!

mscdex commented May 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

mafintosh commented May 28, 2018•
edited
Loading

mscdex commented May 28, 2018•
edited
Loading

mscdex commented May 28, 2018•
edited
Loading