Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: fix reads with pos > 4GB #21003

Closed
wants to merge 1 commit into from
Closed

Conversation

mafintosh
Copy link
Member

@mafintosh mafintosh commented May 28, 2018

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.

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

This applies to readSync as well, I think :/

@mafintosh
Copy link
Member Author

@addaleax let me update it

@mafintosh
Copy link
Member Author

added fix for readSync also

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to write a test for this?

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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isNumber? ;)

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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, just putting what was in there before

@mscdex
Copy link
Contributor

mscdex commented May 28, 2018

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

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending a test. Otherwise LGTM.

@mafintosh
Copy link
Member Author

@mscdex that's a nice compromise 👍

@mafintosh mafintosh force-pushed the fs-read-fix branch 2 times, most recently from 9c35f69 to fbcbb4b Compare May 28, 2018 17:41
@mafintosh
Copy link
Member Author

Added a test for both read and readSync

@mafintosh
Copy link
Member Author

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for fast track

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

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.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tweaked the comment

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm. This should get info a release asap


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/uin32/uint32/

{
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems to be the start of the file

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope you're right.

@mscdex
Copy link
Contributor

mscdex commented May 28, 2018

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
Copy link
Member Author

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

@mscdex
Copy link
Contributor

mscdex commented May 28, 2018

I forgot that specifying absolute file positions does not change the current file pointer, so that probably sounds right.

@MylesBorins MylesBorins mentioned this pull request May 29, 2018
MylesBorins added a commit that referenced this pull request May 29, 2018
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
MylesBorins added a commit that referenced this pull request May 29, 2018
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
MylesBorins added a commit that referenced this pull request May 29, 2018
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
cjihrig added a commit to cjihrig/node that referenced this pull request Jun 7, 2018
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>
targos pushed a commit that referenced this pull request Jun 7, 2018
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>
codebytere added a commit to electron/node that referenced this pull request Aug 15, 2018
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.