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: don't set zero fill for zero-length buffer #2931

Closed
wants to merge 1 commit into from
Closed

buffer: don't set zero fill for zero-length buffer #2931

wants to merge 1 commit into from

Conversation

trevnorris
Copy link
Contributor

The Uint8Array constructor doesn't hit the ArrayBuffer::Allocator() when
length == 0. So in these cases don't set the kNoZeroFill flag, since it
won't be reset.

Add test to ensure Uint8Array's are zero-filled after creating a Buffer
of length zero. This test may falsely succeed, but will not falsely fail.

Fix: #2930

R=@indutny
R=@Fishrock123

@trevnorris
Copy link
Contributor Author

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

@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Sep 17, 2015
@trevnorris
Copy link
Contributor Author

CI is green.

@trevnorris
buffer: don't set zero fill for zero-length buffer
088585a 
The Uint8Array constructor doesn't hit the ArrayBuffer::Allocator() when
length == 0. So in these cases don't set the kNoZeroFill flag, since it
won't be reset.

Add test to ensure Uint8Array's are zero-filled after creating a Buffer
of length zero. This test may falsely succeed, but will not falsely fail.

Fix: nodejs#2930
indutny
indutny reviewed Sep 17, 2015
View reviewed changes
lib/buffer.js
// sanity check to prevent the arraybuffer from being allocated with
// garbage.
if (size > 0)
flags[kNoZeroFill] = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should just reset the flag ourselves?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already considered that. If for some reason buffer instantiation throws, and is caught, it won't have a chance to reset the value. The likelihood of that happening is slim to none. Just being super paranoid. I can swap the impl if you think it's unnecessary.

@rvagg
Copy link
Member

rvagg commented Sep 17, 2015

For anyone landing here as confused as I was about zero-filling zero-length buffers I had @trevnorris explain it to me in n00b terms. 74178a5 switched to using the V8 allocate and to make that performant enough to warrant removing Buffer::Create() a flag was conditionally set that tells ArrayBuffer::Allocator() to not zero-fill the next allocation—because we zero-fill initial allocations of Buffer because it slows it down (you can .fill(0) if you need). This is done by setting a flag that applies to the next allocation. Unfortunately for zero-length buffers there is no allocation required .. because .. zero .. so the flags don't get used and therefore don't get reset. So, when the next allocation comes around, it won't zero-fill it regardless of whether it's for a Node Buffer or not. So, the reported issue @ #2930 demonstrates that asking for a new Uint8Array from V8 will result in a non-zero-filled array, which is not what it should be doing and not what users expect to happen with those types.

Did I sum that up well enough @trevnorris?

LGTM

trevnorris added a commit that referenced this pull request Sep 18, 2015
@trevnorris
buffer: don't set zero fill for zero-length buffer
0a329d2 
Instantiating a Buffer of length zero would set the kNoZeroFill flag to
true but never actually call ArrayBuffer::Allocator(). Which means the
flag was never set back to false. The result was that the next
allocation would unconditionally not be zero filled.

Add test to ensure Uint8Array's are zero-filled after creating a Buffer
of length zero. This test may falsely succeed, but will not falsely fail.

Fix: #2930
PR-URL: #2931
Reviewed-By: Rod Vagg <rod@vagg.org>
@trevnorris
Copy link
Contributor Author

@rvagg That does well explain it. I've updated the commit message to be more clear on that point.

Landed in 0a329d2.

@trevnorris trevnorris closed this Sep 18, 2015
@trevnorris trevnorris mentioned this pull request Sep 18, 2015
Closed
trevnorris added a commit that referenced this pull request Sep 20, 2015
@trevnorris @Fishrock123
buffer: don't set zero fill for zero-length buffer
d63e02e 
Instantiating a Buffer of length zero would set the kNoZeroFill flag to
true but never actually call ArrayBuffer::Allocator(). Which means the
flag was never set back to false. The result was that the next
allocation would unconditionally not be zero filled.

Add test to ensure Uint8Array's are zero-filled after creating a Buffer
of length zero. This test may falsely succeed, but will not falsely fail.

Fix: #2930
PR-URL: #2931
Reviewed-By: Rod Vagg <rod@vagg.org>
mgol added a commit to mgol/check-dependencies that referenced this pull request Sep 20, 2015
@mgol
Bump advised Node 4 version to >=4.1.1
dc4d865 
See nodejs/node#2943 for the discussion about the issue

Refs nodejs/node#2931
mgol added a commit to mgol/grunt-check-dependencies that referenced this pull request Sep 20, 2015
@mgol
Bump advised Node 4 version to >=4.1.1
11a6b4d 
See nodejs/node#2943 for the discussion about the issue

Refs nodejs/node#2931
mgol added a commit to EE/grunt-inline-images that referenced this pull request Sep 20, 2015
@mgol
Bump advised Node 4 version to >=4.1.1
4a45d7f 
See nodejs/node#2943 for the discussion about the issue

Refs nodejs/node#2931
mgol added a commit to EE/jscs-trailing-comma that referenced this pull request Sep 20, 2015
@mgol
Bump advised Node 4 version to >=4.1.1
ca9ce65 
See nodejs/node#2943 for the discussion about the issue

Refs nodejs/node#2931
mgol added a commit to mgol/npm-bump that referenced this pull request Sep 20, 2015
@mgol
Bump advised Node 4 version to >=4.1.1
614d28d 
See nodejs/node#2943 for the discussion about the issue

Refs nodejs/node#2931
@vielmetti vielmetti mentioned this pull request Sep 22, 2015
Closed
@rvagg rvagg mentioned this pull request Sep 22, 2015
Merged
rvagg added a commit that referenced this pull request Sep 22, 2015
@rvagg
2015-09-22, Version 4.1.1 (Stable) Release
ab55b45 
Notable changes

* buffer: Fixed a bug introduced in v4.1.0 where allocating a new
  zero-length buffer can result in the next allocation of a TypedArray
  in JavaScript not being zero-filled. In certain circumstances this
  could result in data leakage via reuse of memory space in
  TypedArrays, breaking the normally safe assumption that TypedArrays
  should be always zero-filled. (Trevor Norris) #2931.
* http: Guard against response-splitting of HTTP trailing headers
  added via response.addTrailers() by removing new-line ([\r\n])
  characters from values. Note that standard header values are already
  stripped of new-line characters. The expected security impact is low
  because trailing headers are rarely used. (Ben Noordhuis) #2945.
* npm: Upgrade to npm 2.14.4 from 2.14.3, see release notes for full
  details (Kat Marchán) #2958
  - Upgrades graceful-fs on multiple dependencies to no longer rely on
    monkey-patching fs
  - Fix npm link for pre-release / RC builds of Node
* v8: Update post-mortem metadata to allow post-mortem debugging tools
  to find and inspect:
  - JavaScript objects that use dictionary properties
    (Julien Gilli) #2959
  - ScopeInfo and thus closures (Julien Gilli) #2974
rvagg added a commit that referenced this pull request Sep 23, 2015
@rvagg
2015-09-22, Version 4.1.1 (Stable) Release
4a6f1fe 
Notable changes

* buffer: Fixed a bug introduced in v4.1.0 where allocating a new
  zero-length buffer can result in the next allocation of a TypedArray
  in JavaScript not being zero-filled. In certain circumstances this
  could result in data leakage via reuse of memory space in
  TypedArrays, breaking the normally safe assumption that TypedArrays
  should be always zero-filled. (Trevor Norris) #2931.
* http: Guard against response-splitting of HTTP trailing headers
  added via response.addTrailers() by removing new-line ([\r\n])
  characters from values. Note that standard header values are already
  stripped of new-line characters. The expected security impact is low
  because trailing headers are rarely used. (Ben Noordhuis) #2945.
* npm: Upgrade to npm 2.14.4 from 2.14.3, see release notes for full
  details (Kat Marchán) #2958
  - Upgrades graceful-fs on multiple dependencies to no longer rely on
    monkey-patching fs
  - Fix npm link for pre-release / RC builds of Node
* v8: Update post-mortem metadata to allow post-mortem debugging tools
  to find and inspect:
  - JavaScript objects that use dictionary properties
    (Julien Gilli) #2959
  - ScopeInfo and thus closures (Julien Gilli) #2974

PR-URL: #2995
@foray1010 foray1010 mentioned this pull request Sep 23, 2015
Closed
@MylesBorins
Copy link
Contributor

landed in lts-v4.x-staging as d63e02e

@trevnorris trevnorris deleted the fix-ui8-zero-fill branch March 28, 2016 22:22
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.

None yet
6 participants
@trevnorris @rvagg @MylesBorins @indutny @mscdex @jasnell