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.read into zero buffer should not throw exception #4518

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@feross
Contributor

feross commented Jan 3, 2016

Fixes #4517.

@feross

View changes

src/node_file.cc Outdated
@@ -1098,7 +1098,7 @@ static void Read(const FunctionCallbackInfo<Value>& args) {
size_t buffer_length = Buffer::Length(buffer_obj);
size_t off = args[2]->Int32Value();
if (off >= buffer_length) {
if (off > buffer_length) {

This comment has been minimized.

@feross

feross Jan 3, 2016

Contributor

This should be safe because the following if-statement will catch if offset + length would be out-of-bounds of the buffer.

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 3, 2016

Member

I don't think it is. If buffer_length == 0, then it's possible for buffer_data == nullptr. nullptr + 0 is technically defined behavior (in C++, not C) but passing that pointer onto the kernel can - but does not have to - fail.

If zero-sized reads are desirable, I would handle them in lib/fs.js.

This comment has been minimized.

@feross

feross Jan 3, 2016

Contributor

Done.

@Trott

This comment has been minimized.

Member

Trott commented Jan 3, 2016

@thefourtheye

View changes

test/parallel/test-fs-read-buffer-zero-length.js Outdated
bufferSync = new Buffer(0),
readCalled = 0;
fs.read(fd, bufferAsync, 0, 0, 0, function(err, bytesRead) {

This comment has been minimized.

@thefourtheye

thefourtheye Jan 3, 2016

Contributor

Use common.mustCall instead of using readCalled

This comment has been minimized.

@feross

feross Jan 3, 2016

Contributor

I just copied this from another test. Will update.

@thefourtheye

This comment has been minimized.

Contributor

thefourtheye commented Jan 3, 2016

What could be the possible use-case which would need this?

@Trott

This comment has been minimized.

Member

Trott commented Jan 3, 2016

@feross

This comment has been minimized.

Contributor

feross commented Jan 3, 2016

@thefourtheye Yep, what @Trott said.

Zero-length files are valid files. If the user passes one into our module, I think it's reasonable to expect fs.read to work. Instead, we currently have to resort to detecting this special case like this: random-access-storage/random-access-file@32f3d1b

@feross

This comment has been minimized.

Contributor

feross commented Jan 3, 2016

@thefourtheye Just addressed your comment by updating the PR.

@thefourtheye thefourtheye added the fs label Jan 3, 2016

@tejasmanohar

This comment has been minimized.

tejasmanohar commented Jan 3, 2016

👍

@Trott

This comment has been minimized.

Member

Trott commented Jan 3, 2016

Re-running CI since force push: https://ci.nodejs.org/job/node-test-commit/1612/

@feross

This comment has been minimized.

Contributor

feross commented Jan 3, 2016

I just force pushed again to address @bnoordhuis's comments. I moved the logic to lib/fs.js, as he suggested.

@Trott You might want to re-run the CI again ;)

@Trott

This comment has been minimized.

Member

Trott commented Jan 3, 2016

@evanlucas

This comment has been minimized.

Member

evanlucas commented Jan 4, 2016

CI looks good (minus the occasional flaky tests). LGTM

@@ -609,6 +609,12 @@ fs.read = function(fd, buffer, offset, length, position, callback) {
};
}
if (length === 0) {
return process.nextTick(function() {

This comment has been minimized.

@thefourtheye

thefourtheye Jan 4, 2016

Contributor

Hmmm, this is not right. There is actually a deprecated fs.read form which returns string.

So,

  1. This will actually return a buffer and the deprecated fs.read will collapse.
  2. The order of parameters to the callback, in string form, is actually string and the bytesRead.

This comment has been minimized.

@feross

feross Jan 4, 2016

Contributor

Actually, this calls the redefined callback function (that starts on line 600), so the right arguments should be passed to the callback function, namely: null, '', and 0.

Unless I'm missing something.

This comment has been minimized.

@feross

feross Jan 4, 2016

Contributor

You comment prompted me to check the fs.readSync case and the issue you describe exists there. I'll fix it and add a test.

@feross

This comment has been minimized.

Contributor

feross commented Jan 4, 2016

Caught one additional bug in the legacy usage of fs.readSync, so I fixed it and added a test.

Please take a look at the changes and re-run CI. 👍

@thefourtheye

View changes

test/parallel/test-fs-read-zero-length.js Outdated
fd = fs.openSync(filepath, 'r'),
expected = '';
fs.read(fd, 0, 0, 'utf-8', common.mustCall(function(err, str, bytesRead) {

This comment has been minimized.

@thefourtheye

thefourtheye Jan 4, 2016

Contributor

Make length consistent with this and the readSync case.

This comment has been minimized.

@feross

feross Jan 6, 2016

Contributor

Done!

@@ -648,6 +654,14 @@ fs.readSync = function(fd, buffer, offset, length, position) {
offset = 0;
}
if (length === 0) {

This comment has been minimized.

@thefourtheye

thefourtheye Jan 4, 2016

Contributor

The check here means that even if the file doesn't exist/inaccessible, if the length is zero, we will return valid values.

This comment has been minimized.

@feross

feross Jan 5, 2016

Contributor

How do you propose to solve this? @bnoordhuis asked me to move the checks to lib/fs.js to avoid some kernel edge case.

Thoughts?

This comment has been minimized.

@trevnorris

trevnorris Jan 6, 2016

Contributor

@thefourtheye Since the fd itself is passed best we can attempt to do is read directly from it. But as @feross mentioned, there are cross platform issues that prevents this being done correctly. I don't think that assuming the fd is alright is a problem in this case.

This comment has been minimized.

@thefourtheye

thefourtheye Jan 6, 2016

Contributor

@trevnorris Ah, you are right. I totally missed that. Then this will not be a problem here.

@jasnell jasnell added the semver-major label Jan 5, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Jan 5, 2016

Defensively marking as semver-major due to the error handling change. Minor might be more appropriate.

@feross

This comment has been minimized.

Contributor

feross commented Jan 6, 2016

Updated PR to make the test more consistent, per @thefourtheye's comment.

I think this is ready to be landed 👍

@thefourtheye

This comment has been minimized.

Contributor

thefourtheye commented Jan 6, 2016

@jasnell

View changes

test/parallel/test-fs-read-buffer-zero-length.js Outdated
'use strict';
var common = require('../common');
var assert = require('assert');
var path = require('path'),

This comment has been minimized.

@jasnell

jasnell Jan 6, 2016

Member

these should use const

This comment has been minimized.

@feross

feross Jan 6, 2016

Contributor

You guys should really add an eslint rule for this. So much overhead for contributors to deal with the back-and-forth.

This comment has been minimized.

@Fishrock123

Fishrock123 Jan 6, 2016

Member

As far as I know, the eslint rule only catches const vs let, not vs var..

cc @silverwind?

@jasnell

View changes

test/parallel/test-fs-read-buffer-zero-length.js Outdated
fd = fs.openSync(filepath, 'r'),
bufferAsync = new Buffer(0),
bufferSync = new Buffer(0);

This comment has been minimized.

@jasnell

jasnell Jan 6, 2016

Member

Nit: would prefer the decl's to be separated but not critical

This comment has been minimized.

@feross

feross Jan 6, 2016

Contributor

This was copied from another test file. Will change.

@jasnell

View changes

test/parallel/test-fs-read-zero-length.js Outdated
var common = require('../common');
var assert = require('assert');
var path = require('path'),
fs = require('fs'),

This comment has been minimized.

@jasnell

jasnell Jan 6, 2016

Member

const please :-)

This comment has been minimized.

@feross

feross Jan 6, 2016

Contributor

Yep.

@feross

This comment has been minimized.

Contributor

feross commented Jan 6, 2016

Force pushed again to address @jasnell's comments.

fingers crossed

@jasnell

This comment has been minimized.

Member

jasnell commented Jan 6, 2016

thank you @feross ... and thank you for bearing with us ;-) the contribution is very appreciated.
Doing another CI pass: https://ci.nodejs.org/job/node-test-pull-request/1151/
LGTM if CI is green! :-)

@feross

This comment has been minimized.

Contributor

feross commented Jan 6, 2016

Thanks @jasnell, I appreciate the attention to detail.

@jasnell

This comment has been minimized.

Member

jasnell commented Jan 6, 2016

CI failures look unrelated! Will get this landed

jasnell added a commit that referenced this pull request Jan 6, 2016

fs: fs.read into zero buffer should not throw exception
Fixes: #4517.
PR-URL: #4518
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Member

jasnell commented Jan 6, 2016

Landed in 2b15e68

@jasnell jasnell closed this Jan 6, 2016

@feross feross deleted the feross:fs-read-fix branch Jan 6, 2016

@feross

This comment has been minimized.

Contributor

feross commented Jan 6, 2016

Nice!

@jasnell jasnell referenced this pull request Mar 17, 2016

Closed

Planning for v6 #5766

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

fs: fs.read into zero buffer should not throw exception
Fixes: nodejs#4517.
PR-URL: nodejs#4518
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@jasnell jasnell referenced this pull request Apr 19, 2016

Closed

What is new in v6? #6264

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* OS X
  * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7
    [#6402](#6402).
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* OS X
  * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7
    [#6402](#6402).
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* OS X
  * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7
    [#6402](#6402).
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment