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.

Sign up for GitHub

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

Jump to bottom

buffer: throw when filling with empty buffers #18129

Merged
merged 2 commits into from
Jan 17, 2018
Merged

buffer: throw when filling with empty buffers #18129

merged 2 commits into from
Jan 17, 2018

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 13, 2018

Prior to this commit, Node would enter an infinite loop when attempting to fill a non-zero length buffer with a zero length buffer. This commit introduces a thrown exception in this scenario.

Fixes: #18128

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

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Jan 13, 2018
TimothyGu
TimothyGu approved these changes Jan 13, 2018
View reviewed changes
doc/api/buffer.md Outdated
@@ -519,6 +519,10 @@ changes:
pr-url: https://github.com/nodejs/node/pull/17427
description: Specifying an invalid string for `fill` triggers a thrown
exception.
- version: REPLACEME
Copy link
Member

Choose a reason for hiding this comment

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

Bad indentation (ditto below). This should also come first in changes.

Trott
Trott reviewed Jan 14, 2018
View reviewed changes
test/parallel/test-buffer-alloc.js
Buffer.alloc(1, Buffer.alloc(0));
}, {
code: 'ERR_INVALID_ARG_VALUE',
type: TypeError
Copy link
Member

Choose a reason for hiding this comment

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

This issue seems to be everywhere ERR_INVALID_ARG_VALUE is used, so consider this non-blocking, but I'd think TypeError is the wrong error to use here. The type of the argument (Buffer) is fine. It's the value that's the problem. I think it should be vanilla Error or maybe RangeError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To your point, this PR uses the same error handling logic from #17427. In that PR, I was against using a TypeError for the same reasons you list here, but other collaborators felt differently.

Copy link
Member

Choose a reason for hiding this comment

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

I think TypeError is fine here.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 15, 2018 •
edited
Loading

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

UPDATE: CI: https://ci.nodejs.org/job/node-test-pull-request/12573/

@cjihrig
buffer: throw when filling with empty buffers
1e80253 
Prior to this commit, Node would enter an infinite loop when
attempting to fill a non-zero length buffer with a zero length
buffer. This commit introduces a thrown exception in this scenario.

PR-URL: nodejs#18129
Fixes: nodejs#18128
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@cjihrig
doc: correct buffer changelog ordering
dbdcf12 
This commit reorders the function level changelogs of
Buffer.prototype.fill() and Buffer.alloc() to reflect the order
in which the entries were added.

PR-URL: nodejs#18129
Fixes: nodejs#18128
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 17, 2018

Landed in 1e80253 and dbdcf12.

@cjihrig cjihrig merged commit dbdcf12 into nodejs:master Jan 17, 2018
@cjihrig cjihrig deleted the 18128 branch January 17, 2018 14:08
@MylesBorins
Copy link
Member

This does not land cleanly on v9.x cleanly. Can you please submit a PR against v9.x-staging

@addaleax addaleax added semver-major PRs that contain breaking changes and should be released in the next major version. and removed backport-requested-v9.x labels Feb 20, 2018
@addaleax
Copy link
Member

This shouldn’t be backported, it’s a breaking change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott left review comments

@fhinkel fhinkel fhinkel approved these changes

@lpinca lpinca lpinca approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@richardlau richardlau richardlau approved these changes

Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. 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.

None yet
10 participants
@cjihrig @MylesBorins @addaleax @fhinkel @jasnell @Trott @lpinca @TimothyGu @richardlau @nodejs-github-bot