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.

@addaleax
Copy link
Member

@addaleax addaleax commented May 28, 2018

This applies to readSync as well, I think :/

@mafintosh
Copy link
Member Author

@mafintosh mafintosh commented May 28, 2018

@addaleax let me update it

@mafintosh
Copy link
Member Author

@mafintosh mafintosh commented May 28, 2018

added fix for readSync also

Copy link
Member

@TimothyGu TimothyGu left a comment

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

@TimothyGu TimothyGu May 28, 2018

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

@TimothyGu TimothyGu May 28, 2018

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

@mafintosh mafintosh May 28, 2018

Sure, just putting what was in there before

@mscdex
Copy link
Contributor

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

Pending a test. Otherwise LGTM.

@mafintosh
Copy link
Member Author

@mafintosh mafintosh commented May 28, 2018

@mscdex that's a nice compromise 👍

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

@mafintosh mafintosh commented May 28, 2018

Added a test for both read and readSync

@mafintosh
Copy link
Member Author

@mafintosh mafintosh commented May 28, 2018

Copy link
Member

@jasnell jasnell left a comment

+1 for fast track

@addaleax
Copy link
Member

@addaleax addaleax commented May 28, 2018

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

@mscdex mscdex May 28, 2018

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

@mafintosh mafintosh May 28, 2018

tweaked the comment

Copy link
Member

@mcollina mcollina left a comment

Lgtm. This should get info a release asap

lpinca
lpinca approved these changes May 28, 2018

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

@mscdex mscdex May 28, 2018

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

@mscdex mscdex May 28, 2018

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

Copy link
Member Author

@mafintosh mafintosh May 28, 2018

it seems to be the start of the file

Copy link
Member Author

@mafintosh mafintosh May 28, 2018

nope you're right.

@mscdex
Copy link
Contributor

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

@mafintosh mafintosh commented May 28, 2018

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

@mscdex
Copy link
Contributor

@mscdex 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 issue 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 issue 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 issue 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 issue 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 added a commit that referenced this issue 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 issue 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 issue 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet