FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

buffer: make byteLength throw on invalid input by mscdex · Pull Request #8946 · nodejs/node · GitHub

/ node Public

buffer: make byteLength throw on invalid input - #8946

Closed
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:buffer-bytelength-typeof
ConversationCommits (1)ChecksFiles changed
Closed

buffer: make byteLength throw on invalid input#8946
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:buffer-bytelength-typeof

Conversation

mscdex commented Oct 6, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor
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 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
cjihrig approved these changes Oct 6, 2016
View reviewed changes

cjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jasnell reviewed Oct 6, 2016
View reviewed changes
Comment thread test/parallel/test-buffer-bytelength.js Outdated

jasnell Oct 6, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Perhaps add validation of the error that is thrown?

mscdex Oct 6, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Added.

jasnell approved these changes Oct 6, 2016
View reviewed changes

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with a nit.

mscdex force-pushed the buffer-bytelength-typeof branch from e265f1c to 591026d Compare October 6, 2016 20:40

jasnell commented Oct 7, 2016

Copy link
Copy Markdown
Member

New CI: https://ci.nodejs.org/job/node-test-pull-request/4419/

jasnell commented Oct 7, 2016

Copy link
Copy Markdown
Member

Failure appears to be an unrelated flaky.

mscdex commented Oct 7, 2016

Copy link
Copy Markdown
Contributor Author

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

mcollina commented Oct 7, 2016

Copy link
Copy Markdown
Member

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

jasnell commented Oct 7, 2016

Copy link
Copy Markdown
Member

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

jasnell commented Oct 7, 2016

Copy link
Copy Markdown
Member

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/406/

mscdex commented Oct 7, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

lpinca approved these changes Oct 8, 2016
View reviewed changes

lpinca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

silverwind reviewed Oct 8, 2016
View reviewed changes
Comment thread doc/api/buffer.md Outdated

silverwind Oct 8, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

thefourtheye Oct 8, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mscdex Oct 8, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't mind one way or the other.

silverwind Oct 8, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

So, let's remove it?

mscdex Oct 8, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done.

jasnell commented Oct 8, 2016

Copy link
Copy Markdown
Member

citgm came up completely green :-)

mscdex force-pushed the buffer-bytelength-typeof branch from 591026d to a89e03b Compare October 8, 2016 17:50

jasnell commented Oct 10, 2016

Copy link
Copy Markdown
Member

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

mscdex commented Oct 10, 2016

Copy link
Copy Markdown
Contributor Author

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

jasnell commented Oct 10, 2016

Copy link
Copy Markdown
Member

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.

addaleax approved these changes Oct 10, 2016
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

trevnorris commented Oct 10, 2016

Copy link
Copy Markdown
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 commented Oct 10, 2016

Copy link
Copy Markdown
Member

I think it's overkill

+1

jasnell pushed a commit that referenced this pull request Oct 10, 2016
buffer: make byteLength throw on invalid input
c9dade4 
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 commented Oct 10, 2016

Copy link
Copy Markdown
Member

Landed in c9dade4

jasnell closed this Oct 10, 2016
mscdex deleted the buffer-bytelength-typeof branch October 10, 2016 20:47
jasnell pushed a commit that referenced this pull request Oct 10, 2016
buffer: make byteLength throw on invalid input
96b501d 
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 mentioned this pull request Oct 14, 2016
Closed
jasnell added a commit to jasnell/node that referenced this pull request Oct 24, 2016
2016-10-25, Version 7.0.0 (Current)
c1e20df 
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
2016-10-25, Version 7.0.0 (Current)
362fe01 
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
2016-10-25, Version 7.0.0 (Current)
1e4fafc 
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
2016-10-25, Version 7.0.0 (Current)
7aae498 
    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>
Shwetajain148 mentioned this pull request May 26, 2017
Merged
gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
Ezwal mentioned this pull request Nov 10, 2017
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
+3 more reviewers
thefourtheye thefourtheye approved these changes
silverwind silverwind approved these changes
not-an-aardvark not-an-aardvark approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL