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: make byteLength throw on invalid input #8946

Closed
wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Oct 6, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
  • buffer
Description of change

This makes buffer.byteLength() throw if the first argument passed to it is not a Buffer/TypedArray/etc. or string, instead of converting the value to string and calculating the length of that string.

If other values are being passed, IMHO that could signal a programming error. For example, if an undefined value is passed in, I would not expect to get 'undefined'.length returned. If undefined is passed in, it probably means I have a bug in my code somewhere where a variable isn't being set (properly).

CI: https://ci.nodejs.org/job/node-test-pull-request/4404/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/403/

@mscdex mscdex 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. labels Oct 6, 2016
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

I like this change, although I'm not sure what the ecosystem impact is.

assert.strictEqual(Buffer.byteLength(NaN, 'utf8'), 3);
assert.strictEqual(Buffer.byteLength({}, 'latin1'), 15);
assert.strictEqual(Buffer.byteLength(), 9);
assert.throws(() => { Buffer.byteLength(32, 'latin1'); });
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add validation of the error that is thrown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

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.

LGTM with a nit.

@jasnell
Copy link
Member

jasnell commented Oct 7, 2016

@jasnell
Copy link
Member

jasnell commented Oct 7, 2016

Failure appears to be an unrelated flaky.

@mscdex
Copy link
Contributor Author

mscdex commented Oct 7, 2016

I'd like to get more input from @nodejs/collaborators if possible.

@mcollina
Copy link
Member

mcollina commented Oct 7, 2016

I think the ecosystem impact might be massive.
This change goes together with the deprecation of new Buffer(), so LGTM.

@jasnell
Copy link
Member

jasnell commented Oct 7, 2016

Let's get a citgm run on it then /cc @nodejs/citgm

@jasnell
Copy link
Member

jasnell commented Oct 7, 2016

@mscdex
Copy link
Contributor Author

mscdex commented Oct 7, 2016

I already ran citgm and it looked fine to me (link in OP). The only issue was that there were some unrelated failures which are already known about over on the citgm repo issue tracker.

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -631,7 +631,7 @@ console.log(`${str}: ${str.length} characters, ` +
When `string` is a `Buffer`/[`DataView`]/[`TypedArray`]/[`ArrayBuffer`], the
actual byte length is returned.

Otherwise, converts to `String` and returns the byte length of string.
Otherwise if `typeof string !== 'string'`, an exception is thrown.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we even need this sentence? It already clearly states above which types are accepted.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that we can improve this, without quoting actual code. Either that, or we can simply remove this as @silverwind suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind one way or the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, let's remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jasnell
Copy link
Member

jasnell commented Oct 8, 2016

citgm came up completely green :-)

@jasnell
Copy link
Member

jasnell commented Oct 10, 2016

@mscdex ... if you don't mind I'll go ahead and get this landed.
@nodejs/ctc ... any objections to this landing in v7?

@mscdex
Copy link
Contributor Author

mscdex commented Oct 10, 2016

@jasnell That's fine, I just wanted to give any others the opportunity to look at it over the weekend before landing.

@jasnell
Copy link
Member

jasnell commented Oct 10, 2016

I'll land it a bit later today. I'd like to get it into the new v7 beta build so I can do some additional testing on it before the RC.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

@trevnorris
Copy link
Contributor

Personally I don't care about this, but someone is bound to come along and ask. Should SharedArrayBuffer be added to the TypeError description? I think it's overkill, but wanted to clear that up. Also, LGTM.

@addaleax
Copy link
Member

I think it's overkill

+1

jasnell pushed a commit that referenced this pull request Oct 10, 2016
PR-URL: #8946
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 10, 2016

Landed in c9dade4

@jasnell jasnell closed this Oct 10, 2016
@mscdex mscdex deleted the buffer-bytelength-typeof branch October 10, 2016 20:47
jasnell pushed a commit that referenced this pull request Oct 10, 2016
PR-URL: #8946
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@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.

10 participants