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

buffer: don't set `kNoZeroFill` flag in allocUnsafe by vkurchatkin · Pull Request #6007 · nodejs/node · GitHub

/ node Public

buffer: don't set kNoZeroFill flag in allocUnsafe - #6007

Closed
vkurchatkin wants to merge 1 commit into
nodejs:masterfrom
vkurchatkin:fix-buffer-zerof
ConversationCommits (1)ChecksFiles changed
Closed

buffer: don't set kNoZeroFill flag in allocUnsafe#6007
vkurchatkin wants to merge 1 commit into
nodejs:masterfrom
vkurchatkin:fix-buffer-zerof

Conversation

vkurchatkin commented Apr 1, 2016

Copy link
Copy Markdown
Contributor

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

buffer

Description of change

If kNoZeroFill is set here, it won't be reset in case of
pooled allocation. In case of "slow" allocation it will be
set later anyway.

Fixes: #6006

evanlucas commented Apr 1, 2016

Copy link
Copy Markdown
Contributor

Maybe add a regression test?

MylesBorins commented Apr 1, 2016

Copy link
Copy Markdown
Contributor

Tested locally and can confirm this fixes master + v5

jasnell commented Apr 1, 2016

Copy link
Copy Markdown
Member

Regression test for sure. Otherwise LGTM.

jasnell added the buffer Issues and PRs related to the buffer subsystem. label Apr 1, 2016

cjihrig commented Apr 1, 2016

Copy link
Copy Markdown
Contributor

LGTM pending a test.

vkurchatkin commented Apr 1, 2016

Copy link
Copy Markdown
Contributor Author

Added some tests

evanlucas reviewed Apr 1, 2016
View reviewed changes
Comment thread test/parallel/test-buffer-safe-unsafe.js Outdated

evanlucas Apr 1, 2016

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

style: missing newline

vkurchatkin Apr 2, 2016

Copy link
Copy Markdown
Contributor Author

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

fixed!

evanlucas commented Apr 1, 2016

Copy link
Copy Markdown
Contributor

LGTM if CI is happy

jasnell commented Apr 1, 2016

Copy link
Copy Markdown
Member

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

MylesBorins commented Apr 1, 2016

Copy link
Copy Markdown
Contributor

LGTM

jasnell reviewed Apr 1, 2016
View reviewed changes
Comment thread test/parallel/test-buffer-safe-unsafe.js Outdated

jasnell Apr 1, 2016

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

ha! sigh... I had uint8 on the brain I guess. good catch.

vkurchatkin Apr 2, 2016

Copy link
Copy Markdown
Contributor Author

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

Also -0 could be a factor, but I'm not sure. Any way, tests are a bit non-deterministic, since even if there is no actual zero filling, there is still a chance that some allocations would be zeros

jasnell Apr 2, 2016

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

Yeah, that came up in the original review. It's not a great test. The plan was to revisit to see if the test can be made more robust.

vkurchatkin Apr 2, 2016

Copy link
Copy Markdown
Contributor Author

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

Also for some reason this bug is hard to reproduce with small Uint8Array.

jasnell Apr 2, 2016

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

Looking at it now this one should have been obvious :-/. The bug only shows up after doing a pooled unsafeAlloc allocation because the zero fill flag was never being reset (because a real allocation wasn't being done).

(update: ha! I see you noted that in your commit log... it's definitely a friday)

buffer: don't set kNoZeroFill flag in allocUnsafe
abb9c4d 
If `kNoZeroFill` is set here, it won't be reset in case of
pooled allocation. In case of "slow" allocation it will be
set later anyway.

Fixes: nodejs#6006

jasnell commented Apr 2, 2016

Copy link
Copy Markdown
Member

CI looks good. One unrelated failure.

jasnell pushed a commit that referenced this pull request Apr 2, 2016
buffer: don't set kNoZeroFill flag in allocUnsafe
0dcb026 
If `kNoZeroFill` is set here, it won't be reset in case of
pooled allocation. In case of "slow" allocation it will be
set later anyway.

Fixes: #6006
PR-URL: #6007
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

jasnell commented Apr 2, 2016

Copy link
Copy Markdown
Member

Landed in 0dcb026

jasnell closed this Apr 2, 2016

jasnell commented Apr 2, 2016

Copy link
Copy Markdown
Member

Will cherry-pick this into v5.x as well.

jasnell pushed a commit that referenced this pull request Apr 2, 2016
buffer: don't set kNoZeroFill flag in allocUnsafe
e966d1f 
If `kNoZeroFill` is set here, it won't be reset in case of
pooled allocation. In case of "slow" allocation it will be
set later anyway.

Fixes: #6006
PR-URL: #6007
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

indutny commented Apr 2, 2016

Copy link
Copy Markdown
Member

CVE?

This was referenced Apr 5, 2016
Closed
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

No reviews

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

First typed array is not zero-filled (v5.10.0)

6 participants

Back | FazBrowse Home | New Git URL