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: empty string is not a valid encoding #9661

Closed
wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Nov 17, 2016

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

buffer

Description of change

Buffer.isEncoding() relies on the internal utility function normalizeEncoding(), which converts falsey values to 'utf8'. This commit adds a check to Buffer.isEncoding() to catch the empty
string, and return false.

Fixes: #9654

Buffer.isEncoding() relies on the internal utility function
normalizeEncoding(), which converts falsey values to 'utf8'. This
commit adds a check to Buffer.isEncoding() to catch the empty
string, and return false.
@nodejs-github-bot nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Nov 17, 2016
@Fishrock123
Copy link
Contributor

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

semver-major?

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 17, 2016

semver-major?

It technically changes behavior, but I'd say it's purely a bug fix. The current behavior is just wrong.

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 and +1 to considering this a bugfix

@bnoordhuis
Copy link
Member

The current behavior is just wrong.

Is it? I thought it kind of makes sense that Buffer.from('x', '') behaves the same as Buffer('x').

Copy link
Contributor

@evanlucas evanlucas left a comment

Choose a reason for hiding this comment

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

Code LGTM, but can we make sure no one is relying on this behavior? cc @ChALkeR

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 18, 2016

I thought it kind of makes sense that Buffer.from('x', '') behaves the same as Buffer('x').

IMO that doesn't make sense either, but it is what it is :-)

@bnoordhuis do you have a preference on the semver-ness of this change, or even think this is a bug that should be fixed?

@jasnell
Copy link
Member

jasnell commented Nov 18, 2016

I'm kind of with @bnoordhuis that treating Buffer.from('abc', '') as equivalent to Buffer.from('abc') makes sense. I'm ok with this change but I'd prefer semver-major

@targos
Copy link
Member

targos commented Nov 18, 2016

This change affects Buffer.prototype.fill(val, start, end, encoding): it will throw if encoding is the empty string. Maybe it should be modified to work like Buffer.from('abc', '') ?

@addaleax addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 18, 2016
@cjihrig cjihrig closed this Nov 23, 2016
@cjihrig cjihrig deleted the buf branch November 23, 2016 20:20
@tniessen
Copy link
Member

tniessen commented Jun 2, 2017

Can we get to a consensus about this issue? I think Buffer.isEncoding('') == true does not make sense at all. IMO we can keep the behavior of Buffer.from('abc', '') if we decide to fallback to utf8 for all falsy values, but we should still change Buffer.isEncoding().

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.

9 participants