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

buffer: stricter argument checking in toString by seishun · Pull Request #11120 · nodejs/node · GitHub

/ node Public

buffer: stricter argument checking in toString - #11120

Merged
seishun merged 4 commits into
nodejs:masterfrom
seishun:buffer-encoding-0
Feb 5, 2017
ConversationCommits (4)ChecksFiles changed
Merged

buffer: stricter argument checking in toString#11120
seishun merged 4 commits into
nodejs:masterfrom
seishun:buffer-encoding-0

Conversation

seishun commented Feb 2, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Currently, Buffer.from('hello').toString(0, 1) returns ello, which is confusing. On the other hand, Buffer.from('hello').toString(1, 2) throws an expected error. This PR disallows passing anything other than undefined or a valid encoding as encoding.

I would love to remove the check altogether thereby also disallowing undefined, but the documentation mentions this feature and there is a test for it, so I guess it's not happening, although I can hardly imagine anyone doing this.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer

buffer: stricter argument checking in toString
4c453db 
This prevents the confusing behavior of `buf.toString(0, 5)` by
disallowing passing `0` as the encoding.
nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Feb 2, 2017

addaleax commented Feb 2, 2017

Copy link
Copy Markdown
Member

Should we consider this semver-major?

joyeecheung reviewed Feb 2, 2017
View reviewed changes

joyeecheung 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

Mind adding a test for it?

cjihrig commented Feb 2, 2017

Copy link
Copy Markdown
Contributor

Should we consider this semver-major?

To be safe, I think so.

addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 2, 2017
cjihrig approved these changes Feb 2, 2017
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

LGTM with a test.

seishun commented Feb 2, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Should we consider this semver-major?

Considering the docs only mention undefined, this boils down to whether anyone in the ecosystem passes any other falsy values to toString.

Mind adding a test for it?

Do you think there's an existing test file where I could add it, or does it belong in a new file?

joyeecheung commented Feb 2, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

@seishun Probably test-buffer-tostring-range.js?

EDIT: that's a existing one.

seishun commented Feb 2, 2017

Copy link
Copy Markdown
Contributor Author

@joyeecheung added a test, PTAL

thefourtheye commented Feb 2, 2017

Copy link
Copy Markdown
Contributor

I would love to see the same change in lib/internal/util.js's normalizeEncoding as well.

cjihrig reviewed Feb 2, 2017
View reviewed changes
Comment thread test/parallel/test-buffer-tostring-range.js Outdated
}}), 'abc');

// try toString() with 0 as the encoding
assert.throws(() => rangeBuffer.toString(0, 1, 2), /Unknown encoding/);

cjihrig Feb 2, 2017

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

Can you improve the regular expression to something like /^TypeError: Unknown encoding: 0$/.

cjihrig Feb 2, 2017

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

Can you add a similar test for null, minimally.

joyeecheung reviewed Feb 2, 2017
View reviewed changes
Comment thread test/parallel/test-buffer-tostring-range.js Outdated
}}), 'abc');

// try toString() with 0 as the encoding
assert.throws(() => rangeBuffer.toString(0, 1, 2), /Unknown encoding/);

joyeecheung Feb 2, 2017

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

Better include the complete message, like ^TypeError: Unknown encoding: undefined$.

seishun commented Feb 2, 2017

Copy link
Copy Markdown
Contributor Author

@thefourtheye what public API would it affect?

joyeecheung commented Feb 2, 2017

Copy link
Copy Markdown
Member

@thefourtheye normalizeEncoding does coerce false and '' to utf8, not sure would anybody in the userland really pass those kinds of falsy values into the buffer methods that calls it though. Nonetheless we could at least add a test in test-internal-util-normalizeencoding.js

thefourtheye commented Feb 2, 2017

Copy link
Copy Markdown
Contributor

@seishun I could find only the following in a quick scan.

  • Buffer.isEncoding
  • Buffer.fill
  • Buffer.transcode
  • StringDecoder

seishun commented Feb 2, 2017

Copy link
Copy Markdown
Contributor Author

@thefourtheye only the last two are affected, the others check the type. I don't think this is a problem, there is no confusion it can cause like in this case. In any case, it belongs in a separate PR.

seishun merged commit 9a0829d into nodejs:master Feb 5, 2017
seishun deleted the buffer-encoding-0 branch February 10, 2017 14:30
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
buffer: stricter argument checking in toString
513ffb3 
This prevents the confusing behavior of `buf.toString(0, 5)` by
disallowing passing `0` as the encoding.

PR-URL: nodejs#11120
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
jasnell mentioned this pull request Apr 4, 2017
Closed
vsemozhetbyt mentioned this pull request Jun 26, 2017
Closed
abdulmoizeng mentioned this pull request Mar 8, 2018
Merged
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

cjihrig cjihrig approved these changes
jasnell jasnell approved these changes
addaleax addaleax approved these changes
lpinca lpinca approved these changes
joyeecheung joyeecheung approved these changes
+1 more reviewer
thefourtheye thefourtheye 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.

8 participants

Back | FazBrowse Home | New Git URL