buffer: make byteLength throw on invalid input #8946

mscdex · 2016-10-06T05:37:52Z

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/

cjihrig

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

jasnell · 2016-10-06T18:30:29Z

test/parallel/test-buffer-bytelength.js

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


Perhaps add validation of the error that is thrown?

jasnell

LGTM with a nit.

jasnell · 2016-10-07T15:38:35Z

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

jasnell · 2016-10-07T16:19:48Z

Failure appears to be an unrelated flaky.

mscdex · 2016-10-07T20:31:59Z

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

mcollina · 2016-10-07T22:08:38Z

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

jasnell · 2016-10-07T23:20:36Z

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

jasnell · 2016-10-07T23:21:45Z

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

mscdex · 2016-10-07T23:39:44Z

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

LGTM

silverwind · 2016-10-08T06:56:14Z

doc/api/buffer.md

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


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

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

I don't mind one way or the other.

So, let's remove it?

jasnell · 2016-10-08T15:51:45Z

citgm came up completely green :-)

jasnell · 2016-10-10T17:51:05Z

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

mscdex · 2016-10-10T17:59:00Z

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

jasnell · 2016-10-10T18:00:46Z

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

LGTM

trevnorris · 2016-10-10T18:47:29Z

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 · 2016-10-10T19:08:13Z

I think it's overkill

+1

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 · 2016-10-10T20:03:30Z

Landed in 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>

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>

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

jasnell added the error-handling label Oct 6, 2016

jasnell reviewed Oct 6, 2016

View reviewed changes

jasnell approved these changes Oct 6, 2016

View reviewed changes

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

thefourtheye approved these changes Oct 7, 2016

View reviewed changes

not-an-aardvark approved these changes Oct 7, 2016

View reviewed changes

lpinca approved these changes Oct 8, 2016

View reviewed changes

silverwind reviewed Oct 8, 2016

View reviewed changes

buffer: make byteLength throw on invalid input

a89e03b

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

silverwind approved these changes Oct 8, 2016

View reviewed changes

addaleax approved these changes Oct 10, 2016

View reviewed changes

jasnell closed this Oct 10, 2016

mscdex deleted the buffer-bytelength-typeof branch October 10, 2016 20:47

jasnell mentioned this pull request Oct 14, 2016

v7.0.0 Proposal #9099

Closed

Shwetajain148 mentioned this pull request May 26, 2017

Fix undefined issue while logging from node-loggly-bulk library loggly/node-loggly-bulk#11

Merged

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

Ezwal mentioned this pull request Nov 10, 2017

Crash on empty file upload lennym/busboy-body-parser#21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: make byteLength throw on invalid input #8946

buffer: make byteLength throw on invalid input #8946

mscdex commented Oct 6, 2016 •

edited

Loading

cjihrig left a comment

jasnell Oct 6, 2016

mscdex Oct 6, 2016

jasnell left a comment

jasnell commented Oct 7, 2016

jasnell commented Oct 7, 2016

mscdex commented Oct 7, 2016

mcollina commented Oct 7, 2016

jasnell commented Oct 7, 2016

jasnell commented Oct 7, 2016

mscdex commented Oct 7, 2016 •

edited

Loading

lpinca left a comment

silverwind Oct 8, 2016

thefourtheye Oct 8, 2016

mscdex Oct 8, 2016

silverwind Oct 8, 2016

mscdex Oct 8, 2016

jasnell commented Oct 8, 2016

jasnell commented Oct 10, 2016

mscdex commented Oct 10, 2016

jasnell commented Oct 10, 2016

addaleax left a comment

trevnorris commented Oct 10, 2016

addaleax commented Oct 10, 2016

jasnell commented Oct 10, 2016

buffer: make byteLength throw on invalid input #8946

buffer: make byteLength throw on invalid input #8946

Conversation

mscdex commented Oct 6, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

cjihrig left a comment

Choose a reason for hiding this comment

jasnell Oct 6, 2016

Choose a reason for hiding this comment

mscdex Oct 6, 2016

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

jasnell commented Oct 7, 2016

jasnell commented Oct 7, 2016

mscdex commented Oct 7, 2016

mcollina commented Oct 7, 2016

jasnell commented Oct 7, 2016

jasnell commented Oct 7, 2016

mscdex commented Oct 7, 2016 • edited Loading

lpinca left a comment

Choose a reason for hiding this comment

silverwind Oct 8, 2016

Choose a reason for hiding this comment

thefourtheye Oct 8, 2016

Choose a reason for hiding this comment

mscdex Oct 8, 2016

Choose a reason for hiding this comment

silverwind Oct 8, 2016

Choose a reason for hiding this comment

mscdex Oct 8, 2016

Choose a reason for hiding this comment

jasnell commented Oct 8, 2016

jasnell commented Oct 10, 2016

mscdex commented Oct 10, 2016

jasnell commented Oct 10, 2016

addaleax left a comment

Choose a reason for hiding this comment

trevnorris commented Oct 10, 2016

addaleax commented Oct 10, 2016

jasnell commented Oct 10, 2016

mscdex commented Oct 6, 2016 •

edited

Loading

mscdex commented Oct 7, 2016 •

edited

Loading