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

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

Closed
mbloch opened this issue Apr 1, 2016 · 11 comments
Closed

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

mbloch opened this issue Apr 1, 2016 · 11 comments
Labels
security Issues and PRs related to security. v8 engine Issues and PRs related to the V8 dependency.

Comments

@mbloch
Copy link

mbloch commented Apr 1, 2016

Version: v5.10.0
Platform: Darwin [...] 14.5.0 Darwin Kernel Version 14.5.0: Mon Jan 11 18:48:35 PST 2016; root:xnu-2782.50.2~1/RELEASE_X86_64 x86_64

The first typed array created during script execution may contain non-zero values. Subsequent typed arrays are zero-filled, as expected.

I began to see this behavior after installing v5.10.0 from the following source: https://nodejs.org/dist/v5.10.0/node-v5.10.0-darwin-x64.tar.gz

The previous version, 5.9.1, is not affected.

I'm pasting sample output from the attached script zerofill_bug.txt below.

zerofill_bug.txt

File contents

#!/usr/bin/env node
console.log(new Float64Array(10));
console.log(new Float64Array(10));

Sample output

Float64Array {
  '0': 0,
  '1': 0,
  '2': 2.146862934e-314,
  '3': 2.1468647947e-314,
  '4': 2.146864866e-314,
  '5': 2.1468649963e-314,
  '6': 2.146865024e-314,
  '7': 5.56270611491583e-309,
  '8': 2.3194251029888626e+242,
  '9': 9.765157996892863e+199 }
Float64Array {
  '0': 0,
  '1': 0,
  '2': 0,
  '3': 0,
  '4': 0,
  '5': 0,
  '6': 0,
  '7': 0,
  '8': 0,
  '9': 0 }
@MylesBorins MylesBorins added buffer Issues and PRs related to the buffer subsystem. security Issues and PRs related to security. v8 engine Issues and PRs related to the V8 dependency. and removed buffer Issues and PRs related to the buffer subsystem. labels Apr 1, 2016
@MylesBorins
Copy link
Member

I am able to reproduce. Not sure if the labels I have applied are correct or not. I'm doing a bisect right now to see what's up

/cc @nodejs/security

@evanlucas
Copy link
Contributor

Hm, I cannot reproduce it from the repl though...weird

@vkurchatkin
Copy link
Contributor

@evanlucas probably because some allocations are already made

@MylesBorins
Copy link
Member

Looks like the regression comes in from 3c02727

edit: wrong sha at first

@evanlucas
Copy link
Contributor

master is affected also

@MylesBorins
Copy link
Member

/cc @jasnell

@MylesBorins
Copy link
Member

I'm not 100% but I'm going to guess we are doing something wrong here 3c02727#diff-cd53544f44aab2c697bcd7b6a57f23ccR949

@vkurchatkin
Copy link
Contributor

Calling Buffer.allocUnsafe reproduces this state

@evanlucas
Copy link
Contributor

Passing the --zero-fill-buffers flag definitely makes it work

@jasnell
Copy link
Member

jasnell commented Apr 1, 2016

@thealphanerd ... quick test on v5.x with that additional check removed still shows the problem. Master includes that exact same check also and I'm unable to reproduce there.

@MylesBorins
Copy link
Member

I just ran a test with the following on master

console.log(new Float64Array(10));
console.log(new Float64Array(10));
var a = new Buffer.allocUnsafe(200);
console.log(new Float64Array(10));
console.log(new Float64Array(10));

output is

Float64Array [
  0,
  0,
  2.5196862903032468e+180,
  2.1390944465e-314,
  0,
  1.1125369292536007e-308,
  0,
  3.105036371167128e+231,
  1.7272338147994846e-77,
  6.953355807835004e-309 ]
Float64Array [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 ]
Float64Array [
  0,
  0,
  2.1345912614e-314,
  2.1346182947e-314,
  0,
  0,
  2.1345912604e-314,
  2.1346095003e-314,
  2.134609532e-314,
  2.1346095636e-314 ]
Float64Array [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 ]

vkurchatkin added a commit to vkurchatkin/node that referenced this issue Apr 2, 2016
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 jasnell closed this as completed in 0dcb026 Apr 2, 2016
jasnell pushed a commit that referenced this issue Apr 2, 2016
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 pushed a commit that referenced this issue Apr 26, 2016
This makes sure that `kNoZeroFill` flag is not accidentally set by
moving the all the flag operations directly inside `createBuffer()`.
It safeguards against logical errors like
#6006.

This also ensures that `kNoZeroFill` flag is always restored to 0 using
a try-finally block, as it could be not restored to 0 in cases of failed
or zero-size `Uint8Array` allocation.
It safeguards against errors like
#2930.
It also makes the `size > 0` check not needed there.

PR-URL: nodejs-private/node-private#30
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
jasnell pushed a commit that referenced this issue Apr 26, 2016
This makes sure that `kNoZeroFill` flag is not accidentally set by
moving the all the flag operations directly inside `createBuffer()`.
It safeguards against logical errors like
#6006.

This also ensures that `kNoZeroFill` flag is always restored to 0 using
a try-finally block, as it could be not restored to 0 in cases of failed
or zero-size `Uint8Array` allocation.
It safeguards against errors like
#2930.
It also makes the `size > 0` check not needed there.

PR-URL: nodejs-private/node-private#30
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Issues and PRs related to security. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants