buffer: throw on negative .allocUnsafe() argument by addaleax · Pull Request #7079 · nodejs/node · GitHub

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/

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

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

Should this have 7.0 milestone?

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.

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.

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

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.

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

LGTM

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

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

LGTM

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.

Good catch. +1

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

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
.

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

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

Works for me also.

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.

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>

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

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

@LinusU ... doc changes are always helpful!

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

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

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>

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

addaleax mentioned this pull request May 31, 2016

buffer: ignore negative allocation lengths #7051

Merged

3 tasks

yorkie reviewed May 31, 2016
View reviewed changes

addaleax added the blocked PRs that are blocked by other issues or PRs. label May 31, 2016

addaleax mentioned this pull request May 31, 2016

buffer: improve creation performance #6893

Closed

2 tasks

addaleax added this to the 7.0.0 milestone Jun 1, 2016

RReverser reviewed Jun 6, 2016
View reviewed changes

addaleax removed the blocked PRs that are blocked by other issues or PRs. label Jun 8, 2016

addaleax force-pushed the buffer-negative-allocunsafe-throw branch from 5c9869e to fc43fe1 Compare June 8, 2016 10:22

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

addaleax force-pushed the buffer-negative-allocunsafe-throw branch from a98022a to be32633 Compare August 8, 2016 13:12

addaleax closed this Aug 8, 2016

addaleax deleted the buffer-negative-allocunsafe-throw branch August 8, 2016 13:14

jasnell mentioned this pull request Oct 14, 2016

v7.0.0 Proposal #9099

Closed

joyeecheung mentioned this pull request Dec 6, 2016

doc: buffer allocation methods throw for negative size #10151

Closed

2 tasks

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

buffer: throw on negative .allocUnsafe() argument by addaleax · Pull Request #7079 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

buffer: throw on negative .allocUnsafe() argument#7079addaleax wants to merge 1 commit intonodejs:masternodejs/node:masterfrom addaleax:buffer-negative-allocunsafe-throwaddaleax/node:buffer-negative-allocunsafe-throwCopy head branch name to clipboard

Conversation

addaleax commented May 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

yorkie May 31, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax May 31, 2016

Choose a reason for hiding this comment

Uh oh!

ChALkeR commented Jun 1, 2016

Uh oh!

trevnorris commented Jun 1, 2016

Uh oh!

jasnell commented Jun 6, 2016

Uh oh!

RReverser Jun 6, 2016

Choose a reason for hiding this comment

Uh oh!

trevnorris Jun 7, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jun 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChALkeR commented Jun 8, 2016

Uh oh!

cjihrig commented Jun 8, 2016

Uh oh!

addaleax commented Aug 4, 2016

Uh oh!

jasnell commented Aug 4, 2016

Uh oh!

ChALkeR commented Aug 5, 2016

Uh oh!

jasnell commented Aug 5, 2016

Uh oh!

ChALkeR commented Aug 5, 2016

Uh oh!

jasnell commented Aug 5, 2016

Uh oh!

mscdex commented Aug 5, 2016

Uh oh!

ChALkeR commented Aug 5, 2016

Uh oh!

jasnell commented Aug 5, 2016

Uh oh!

yorkie commented Aug 5, 2016

Uh oh!

addaleax commented Aug 8, 2016

Uh oh!

LinusU commented Aug 18, 2016

Uh oh!

jasnell commented Aug 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

buffer: throw on negative .allocUnsafe() argument#7079
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:buffer-negative-allocunsafe-throw

addaleax commented May 31, 2016 •
edited
Loading

addaleax commented Jun 8, 2016 •
edited
Loading