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

@@ -489,7 +489,7 @@ function readSync(fd, buffer, offset, length, position) {

  validateOffsetLengthRead(offset, length, buffer.length);

-  if (!isUint32(position))
+  if (!Number.isNumber(position))


isNumber? ;)

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

lib/fs.js

@@ -459,7 +459,7 @@ function read(fd, buffer, offset, length, position, callback) {

  validateOffsetLengthRead(offset, length, buffer.length);

-  if (!isUint32(position))
+  if (!Number.isInteger(position))


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

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

@@ -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


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

+
+{
+  // Reading beyond file length (3 in this case) should return no data.
+  // This is a test for a bug where reads > uin32 would return


s/uin32/uint32/

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

test/parallel/test-fs-read.js

+{
+  // 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.


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#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>

PR-URL: #21148 Fixes: #21121 Refs: #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

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 e18a19e Compare May 28, 2018 17:16

TimothyGu reviewed May 28, 2018

View reviewed changes

mafintosh force-pushed the fs-read-fix branch from e18a19e to bcc8f81 Compare 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 fbcbb4b Compare 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 e7c72dd Compare May 28, 2018 19:02

mscdex reviewed May 28, 2018

View reviewed changes

MylesBorins mentioned this pull request May 29, 2018

v10.3.0 proposal #21011

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: fix reads with pos > 4GB #21003

fs: fix reads with pos > 4GB #21003

mafintosh commented May 28, 2018 •

edited

Loading

addaleax commented May 28, 2018

mafintosh commented May 28, 2018

mafintosh commented May 28, 2018

TimothyGu left a comment

TimothyGu May 28, 2018

TimothyGu May 28, 2018

mafintosh May 28, 2018

mscdex commented May 28, 2018 •

edited

Loading

TimothyGu left a comment

mafintosh commented May 28, 2018

mafintosh commented May 28, 2018

mafintosh commented May 28, 2018

jasnell left a comment

addaleax commented May 28, 2018

mscdex May 28, 2018

mafintosh May 28, 2018

mcollina left a comment

mscdex May 28, 2018

mscdex May 28, 2018

mafintosh May 28, 2018

mafintosh May 28, 2018

mscdex commented May 28, 2018 •

edited

Loading

mafintosh commented May 28, 2018

mscdex commented May 28, 2018

fs: fix reads with pos > 4GB #21003

fs: fix reads with pos > 4GB #21003

Conversation

mafintosh commented May 28, 2018 • edited Loading

Checklist

addaleax commented May 28, 2018

mafintosh commented May 28, 2018

mafintosh commented May 28, 2018

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented May 28, 2018 • edited Loading

TimothyGu left a comment

Choose a reason for hiding this comment

mafintosh commented May 28, 2018

mafintosh commented May 28, 2018

mafintosh commented May 28, 2018

jasnell left a comment

Choose a reason for hiding this comment

addaleax commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented May 28, 2018 • edited Loading

mafintosh commented May 28, 2018

mscdex commented May 28, 2018

mafintosh commented May 28, 2018 •

edited

Loading

mscdex commented May 28, 2018 •

edited

Loading

mscdex commented May 28, 2018 •

edited

Loading