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

buffer: throw on negative .allocUnsafe() argument #7079

Closed

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented May 31, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

buffer

Description of change

Add a check for size < 0 to assertSize(), as passing a negative value almost certainly indicates a programming error.

This also lines up the behaviour of .allocUnsafe() with the ones of .alloc() and .allocUnsafeSlow() (which previously threw errors from the Uint8Array constructor).

CI: https://ci.nodejs.org/job/node-test-commit/3599/

@addaleax addaleax added buffer Issues and PRs related to the buffer subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. error-handling labels May 31, 2016
const err = new RangeError('"size" argument must not be negative');
Error.captureStackTrace(err, assertSize);
throw err;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How about this abstraction:

function assertSize(size) {
  let err = null;
  if (typeof size !== 'number') err = new TypeError('"size" argument must be a number');
  if (size < 0) err = new RangeError('"size" argument must not be negative');
  if (err) {
    // The following hides the 'assertSize' method from the
    // callstack. This is done simply to hide the internal
    // details of the implementation from bleeding out to users.
    Error.captureStackTrace(err, assertSize);
    throw err;
  }
}

This could improve the comments for Error.captureStackTrace(err, assertSize);. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated with that, although I’m not it makes a huge difference ;)

@addaleax addaleax added the blocked PRs that are blocked by other issues or PRs. label May 31, 2016
@ChALkeR
Copy link
Member

ChALkeR commented Jun 1, 2016

Should this have 7.0 milestone?

@addaleax addaleax added this to the 7.0.0 milestone Jun 1, 2016
@trevnorris
Copy link
Contributor

Only suggestion I may have is that Buffer.alloc*() calls were being placed in test/parallel/test-buffer-alloc.js after the new API was introduced. Other than that, LGTM.

@jasnell
Copy link
Member

jasnell commented Jun 6, 2016

Aside: we really ought to refactor the test-buffer.js and test-buffer-alloc.js to make those more sane... likely should split them into multiple individual tests.


if (typeof size !== 'number')
err = new TypeError('"size" argument must be a number');
if (size < 0)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps else if here so that it wouldn't attempt to call .valueOf etc. on objects?

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case might as well add the test

assert.throws(() => Buffer.alloc({ valueOf: () => -1 }), /"size" argument must be a number/);

or swapping them would achieve the same logical effect.

@addaleax addaleax removed the blocked PRs that are blocked by other issues or PRs. label Jun 8, 2016
@addaleax addaleax force-pushed the buffer-negative-allocunsafe-throw branch from 5c9869e to fc43fe1 Compare June 8, 2016 10:22
@addaleax
Copy link
Member Author

addaleax commented Jun 8, 2016

Updated with everyone’s comments, new CI: https://ci.nodejs.org/job/node-test-commit/3693/

@ChALkeR
Copy link
Member

ChALkeR commented Jun 8, 2016

LGTM

1 similar comment
@cjihrig
Copy link
Contributor

cjihrig commented Jun 8, 2016

LGTM

@addaleax addaleax force-pushed the buffer-negative-allocunsafe-throw branch from fc43fe1 to a98022a Compare August 4, 2016 23:04
@addaleax
Copy link
Member Author

addaleax commented Aug 4, 2016

Rebased, new CI: https://ci.nodejs.org/job/node-test-commit/4408/

I’ll land this some time in the next couple of days.

@jasnell
Copy link
Member

jasnell commented Aug 4, 2016

LGTM

@ChALkeR
Copy link
Member

ChALkeR commented Aug 5, 2016

Just to clarify: this now also throws on negative Buffer(number), because that does Buffer.allocUnsafe(number). That should be also noted in the changelog, I suppose.

@jasnell
Copy link
Member

jasnell commented Aug 5, 2016

Good catch. +1

@ChALkeR
Copy link
Member

ChALkeR commented Aug 5, 2016

Hm. @jasnell, per #7964 do we want to hard-deprecate this first or not?

@jasnell
Copy link
Member

jasnell commented Aug 5, 2016

Excellent question! To be honest I don't know. What do you think?
@nodejs/ctc...
Thoughts?

On Thursday, August 4, 2016, Сковорода Никита Андреевич <
notifications@github.com> wrote:

Hm. @jasnell https://github.com/jasnell, per #7964
#7964 do we want to hard-deprecate
this first or not?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7079 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2ecrWOnS-dbSA8zliVwTg7G-z3e8zks5qcstzgaJpZM4Iqyya
.

@mscdex
Copy link
Contributor

mscdex commented Aug 5, 2016

I think I'd be more in favor of just landing in v7.

@ChALkeR
Copy link
Member

ChALkeR commented Aug 5, 2016

I'm ok with both and this still LGTM =).

@jasnell
Copy link
Member

jasnell commented Aug 5, 2016

Works for me also.

@yorkie
Copy link
Contributor

yorkie commented Aug 5, 2016

LGTM

Add a check for `size < 0` to `assertSize()`, as passing a negative
value almost certainly indicates a programming error.

This also lines up the behaviour of `.allocUnsafe()` with the ones
of `.alloc()` and `.allocUnsafeSlow()` (which previously threw errors
from the Uint8Array constructor).

Notably, this also affects `Buffer()` calls with negative arguments.
@addaleax addaleax force-pushed the buffer-negative-allocunsafe-throw branch from a98022a to be32633 Compare August 8, 2016 13:12
addaleax added a commit that referenced this pull request Aug 8, 2016
Add a check for `size < 0` to `assertSize()`, as passing a negative
value almost certainly indicates a programming error.

This also lines up the behaviour of `.allocUnsafe()` with the ones
of `.alloc()` and `.allocUnsafeSlow()` (which previously threw errors
from the Uint8Array constructor).

Notably, this also affects `Buffer()` calls with negative arguments.

PR-URL: #7079
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
@addaleax
Copy link
Member Author

addaleax commented Aug 8, 2016

I’ve gone ahead and landed this in 8f90dcc since there seems to be agreement that this can safely go into v7.

@addaleax addaleax closed this Aug 8, 2016
@addaleax addaleax deleted the buffer-negative-allocunsafe-throw branch August 8, 2016 13:14
@LinusU
Copy link
Contributor

LinusU commented Aug 18, 2016

The docs was not updated with this PR, was that intentional or would a PR for docs be welcome?

@jasnell
Copy link
Member

jasnell commented Aug 18, 2016

@LinusU ... doc changes are always helpful!

@jasnell jasnell mentioned this pull request Oct 14, 2016
jasnell added a commit to jasnell/node that referenced this pull request Oct 24, 2016
Notable Changes:

* Buffer
  * Passing invalid input to Buffer.byteLength will now throw an error [nodejs#8946](nodejs#8946).
  * Calling Buffer without new is now deprecated and will emit a process warning [nodejs#8169](nodejs#8169).
  * Passing a negative number to allocUnsafe will now throw an error [nodejs#7079](nodejs#7079).
* Child Process
  * The fork and execFile methods now have stronger argument validation [nodejs#7399](nodejs#7399).
* Cluster
  * The worker.suicide method is deprecated and will emit a process warning [nodejs#3747](nodejs#3747).
* Deps
  * V8 has been updated to 5.4.500.36 [nodejs#8317](nodejs#8317), [nodejs#8852](nodejs#8852), [nodejs#9253](nodejs#9253).
  * NODE_MODULE_VERSION has been updated to 51 [nodejs#8808](nodejs#8808).
* File System
  * A process warning is emitted if a callback is not passed to async file system methods [nodejs#7897](nodejs#7897).
* Intl
  * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [nodejs#8908](nodejs#8908).
* Promises
  * Unhandled Promise rejections have been deprecated and will emit a process warning [nodejs#8217](nodejs#8217).
* Punycode
  * The `punycode` module has been deprecated [nodejs#7941](nodejs#7941).
* URL
  * An Experimental WHATWG URL Parser has been introduced [nodejs#7448](nodejs#7448).
jasnell added a commit that referenced this pull request Oct 25, 2016
Notable Changes:

* Buffer
  * Passing invalid input to Buffer.byteLength will now throw an error [#8946](#8946).
  * Calling Buffer without new is now deprecated and will emit a process warning [#8169](#8169).
  * Passing a negative number to allocUnsafe will now throw an error [#7079](#7079).
* Child Process
  * The fork and execFile methods now have stronger argument validation [#7399](#7399).
* Cluster
  * The worker.suicide method is deprecated and will emit a process warning [#3747](#3747).
* Deps
  * V8 has been updated to 5.4.500.36 [#8317](#8317), [#8852](#8852), [#9253](#9253).
  * NODE_MODULE_VERSION has been updated to 51 [#8808](#8808).
* File System
  * A process warning is emitted if a callback is not passed to async file system methods [#7897](#7897).
* Intl
  * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [#8908](#8908).
* Promises
  * Unhandled Promise rejections have been deprecated and will emit a process warning [#8217](#8217).
* Punycode
  * The `punycode` module has been deprecated [#7941](#7941).
* URL
  * An Experimental WHATWG URL Parser has been introduced [#7448](#7448).

PR-URL: #9099
jasnell added a commit that referenced this pull request Oct 25, 2016
Notable Changes:

* Buffer
  * Passing invalid input to Buffer.byteLength will now throw an error [#8946](#8946).
  * Calling Buffer without new is now deprecated and will emit a process warning [#8169](#8169).
  * Passing a negative number to allocUnsafe will now throw an error [#7079](#7079).
* Child Process
  * The fork and execFile methods now have stronger argument validation [#7399](#7399).
* Cluster
  * The worker.suicide method is deprecated and will emit a process warning [#3747](#3747).
* Deps
  * V8 has been updated to 5.4.500.36 [#8317](#8317), [#8852](#8852), [#9253](#9253).
  * NODE_MODULE_VERSION has been updated to 51 [#8808](#8808).
* File System
  * A process warning is emitted if a callback is not passed to async file system methods [#7897](#7897).
* Intl
  * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [#8908](#8908).
* Promises
  * Unhandled Promise rejections have been deprecated and will emit a process warning [#8217](#8217).
* Punycode
  * The `punycode` module has been deprecated [#7941](#7941).
* URL
  * An Experimental WHATWG URL Parser has been introduced [#7448](#7448).

PR-URL: #9099
imyller added a commit to imyller/meta-nodejs that referenced this pull request Oct 25, 2016
    Notable Changes:

    * Buffer
      * Passing invalid input to Buffer.byteLength will now throw an error [#8946](nodejs/node#8946).
      * Calling Buffer without new is now deprecated and will emit a process warning [#8169](nodejs/node#8169).
      * Passing a negative number to allocUnsafe will now throw an error [#7079](nodejs/node#7079).
    * Child Process
      * The fork and execFile methods now have stronger argument validation [#7399](nodejs/node#7399).
    * Cluster
      * The worker.suicide method is deprecated and will emit a process warning [#3747](nodejs/node#3747).
    * Deps
      * V8 has been updated to 5.4.500.36 [#8317](nodejs/node#8317), [#8852](nodejs/node#8852), [#9253](nodejs/node#9253).
      * NODE_MODULE_VERSION has been updated to 51 [#8808](nodejs/node#8808).
    * File System
      * A process warning is emitted if a callback is not passed to async file system methods [#7897](nodejs/node#7897).
    * Intl
      * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [#8908](nodejs/node#8908).
    * Promises
      * Unhandled Promise rejections have been deprecated and will emit a process warning [#8217](nodejs/node#8217).
    * Punycode
      * The `punycode` module has been deprecated [#7941](nodejs/node#7941).
    * URL
      * An Experimental WHATWG URL Parser has been introduced [#7448](nodejs/node#7448).

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants