Skip to content

buffer: stricter argument checking in toString - #11120

Merged
seishun merged 4 commits into
nodejs:masterfrom
seishun:buffer-encoding-0
Feb 5, 2017
Merged

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

Conversation

@seishun

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

@seishun
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-botnodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Feb 2, 2017
@addaleax

addaleax commented Feb 2, 2017

Copy link
Copy Markdown
Member

Should we consider this semver-major?

joyeecheung
joyeecheung reviewed Feb 2, 2017
View reviewed changes

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

Mind adding a test for it?

@cjihrig

cjihrig commented Feb 2, 2017

Copy link
Copy Markdown
Contributor

Should we consider this semver-major?

To be safe, I think so.

@addaleaxaddaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 2, 2017
cjihrig
cjihrig approved these changes Feb 2, 2017
View reviewed changes

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

LGTM with a test.

@seishun

seishun commented Feb 2, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

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

seishun commented Feb 2, 2017

Copy link
Copy Markdown
ContributorAuthor

@joyeecheung added a test, PTAL

@thefourtheye

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
cjihrig reviewed Feb 2, 2017
View reviewed changes
Comment threadtest/parallel/test-buffer-tostring-range.js Outdated
}}), 'abc');

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

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

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

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

Can you add a similar test for null, minimally.

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

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

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

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

@seishun

seishun commented Feb 2, 2017

Copy link
Copy Markdown
ContributorAuthor

@thefourtheye what public API would it affect?

@joyeecheung

joyeecheung commented Feb 2, 2017

Copy link
Copy Markdown
Member

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

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

seishun commented Feb 2, 2017

Copy link
Copy Markdown
ContributorAuthor

@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
seishun merged commit 9a0829d into nodejs:masterFeb 5, 2017
@seishun
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
@seishun@krydos
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>
@jasnelljasnell mentioned this pull request Apr 4, 2017
Closed
@vsemozhetbytvsemozhetbyt mentioned this pull request Jun 26, 2017
Closed
@abdulmoizengabdulmoizeng mentioned this pull request Mar 8, 2018
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrigcjihrigcjihrig approved these changes
@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@lpincalpincalpinca approved these changes
@joyeecheungjoyeecheungjoyeecheung approved these changes
+1 more reviewer
@thefourtheyethefourtheyethefourtheye approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bufferIssues and PRs related to the buffer subsystem.semver-majorPRs 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

@seishun@addaleax@cjihrig@joyeecheung@thefourtheye@jasnell@lpinca@nodejs-github-bot