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

test: test buffer behavior when zeroFill undefined #11706

Closed
wants to merge 1 commit into from
Closed

test: test buffer behavior when zeroFill undefined #11706

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 6, 2017 •
edited
Loading

When ArrayBufferAllocator has an undefined zeroFill property,
Buffer.allocUnsafe() should zero fill.

Refs: 27e84ddd4e1#commitcomment-19182129

The || [0] (discussed in the Refs: above) is the only piece of code in buffer.js not covered by tests. Ideally, this test would probably be a C++ addon test, but until someone writes that test, this will at least confirm that the intended behavior is correct.

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

test buffer

@Trott Trott added buffer Issues and PRs related to the buffer subsystem. test Issues and PRs related to the tests. labels Mar 6, 2017
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 6, 2017
@Trott
test: test buffer behavior when zeroFill undefined
531dba7 
When ArrayBufferAllocator has an undefined zeroFill property,
Buffer.allocUnsafe() should zero fill.

Refs: nodejs@27e84ddd4e1#commitcomment-19182129
@bnoordhuis
Copy link
Member

I don't know. This is brittle both in how it monkey-patches the bindings and how it expects buffers to contain random garbage. If in an individual run the buffers all come from freshly mmap-ed memory (not inconceivable with a loop like that) the test will fail.

You could ameliorate it somewhat by filling the buffer with non-zero data so that it's not all zeroes when its memory is reused for another buffer but that's still no guarantee.

@Trott
Copy link
Member Author

Trott commented Mar 6, 2017 •
edited
Loading

This is brittle

Yes, indeed. Best what can be said for it is that it's probably better than no test at all (our current situation).

Would happily close this in favor of a C++ addon that did the testing instead.

addaleax
addaleax approved these changes Mar 6, 2017
View reviewed changes
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.

Code LGTM, and I don’t think it hurts to have this

test/parallel/test-buffer-bindingobj-no-zerofill.js
// `process.binding('buffer').setupBufferJS` be monkey-patched before this runs.
const monkeyPatchedBuffer = require('../../lib/buffer');

// On unpatched buffer. allocUnsafe() should not zero fill memory. It's always
Copy link
Member

Choose a reason for hiding this comment

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

typo: , after buffer?

@fhinkel
Copy link
Member

fhinkel commented Mar 9, 2017

Thanks, landed with fixed typo in 9ee58c0.

@fhinkel fhinkel closed this Mar 9, 2017
fhinkel pushed a commit that referenced this pull request Mar 9, 2017
@Trott @fhinkel
test: test buffer behavior when zeroFill undefined
9ee58c0 
When ArrayBufferAllocator has an undefined zeroFill property,
Buffer.allocUnsafe() should zero fill.

Refs: 27e84ddd4e1#commitcomment-19182129

PR-URL: #11706
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@Trott
Copy link
Member Author

Trott commented Mar 9, 2017

(Looks like this may have landed without a CI run. :-O )

italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 13, 2017
@Trott @italoacasas
test: test buffer behavior when zeroFill undefined
1af0fa4 
When ArrayBufferAllocator has an undefined zeroFill property,
Buffer.allocUnsafe() should zero fill.

Refs: nodejs@27e84ddd4e1#commitcomment-19182129

PR-URL: nodejs#11706
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@italoacasas italoacasas mentioned this pull request Mar 13, 2017
Merged
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@Trott @jungx098
test: test buffer behavior when zeroFill undefined
1dfa279 
When ArrayBufferAllocator has an undefined zeroFill property,
Buffer.allocUnsafe() should zero fill.

Refs: nodejs@27e84ddd4e1#commitcomment-19182129

PR-URL: nodejs#11706
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Qantas94Heavy added a commit to Qantas94Heavy/node that referenced this pull request Apr 9, 2017
@Qantas94Heavy
test: fix allocUnsafe uninitialized buffer check
5700fc8 
Fixes parallel/test-buffer-bindingobj-no-zerofill to properly check
that buffers created with `Buffer.allocUnsafe()` are not zero-filled.

The test introduced in nodejs#11706 passes even if the buffer has been
zero-filled and fails if none of the buffer values are zero.

Signed-off-by: Karl Cheng <qantas94heavy@gmail.com>
Refs: nodejs#11706
@Qantas94Heavy Qantas94Heavy mentioned this pull request Apr 9, 2017
Closed
2 tasks
jasnell pushed a commit that referenced this pull request Apr 11, 2017
@Qantas94Heavy @jasnell
test: fix allocUnsafe uninitialized buffer check
eb49d59 
Fixes parallel/test-buffer-bindingobj-no-zerofill to properly check
that buffers created with `Buffer.allocUnsafe()` are not zero-filled.

The test introduced in #11706 passes even if the buffer has been
zero-filled and fails if none of the buffer values are zero.

Refs: #11706
PR-URL: #12290
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

looks like this has failures. Please manually backport (and include #12290 )

@Trott
Copy link
Member Author

Trott commented Apr 18, 2017

This depends on #7082 which is labeled dont-land-on-v6.x. Labeling this the same (and #12290 as well).

evanlucas pushed a commit that referenced this pull request Apr 25, 2017
@Qantas94Heavy @evanlucas
test: fix allocUnsafe uninitialized buffer check
8ee62cd 
Fixes parallel/test-buffer-bindingobj-no-zerofill to properly check
that buffers created with `Buffer.allocUnsafe()` are not zero-filled.

The test introduced in #11706 passes even if the buffer has been
zero-filled and fails if none of the buffer values are zero.

Refs: #11706
PR-URL: #12290
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 1, 2017
@Qantas94Heavy @evanlucas
test: fix allocUnsafe uninitialized buffer check
e63607b 
Fixes parallel/test-buffer-bindingobj-no-zerofill to properly check
that buffers created with `Buffer.allocUnsafe()` are not zero-filled.

The test introduced in #11706 passes even if the buffer has been
zero-filled and fails if none of the buffer values are zero.

Refs: #11706
PR-URL: #12290
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@Qantas94Heavy @evanlucas
test: fix allocUnsafe uninitialized buffer check
1ab998f 
Fixes parallel/test-buffer-bindingobj-no-zerofill to properly check
that buffers created with `Buffer.allocUnsafe()` are not zero-filled.

The test introduced in #11706 passes even if the buffer has been
zero-filled and fails if none of the buffer values are zero.

Refs: #11706
PR-URL: #12290
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott mentioned this pull request Aug 18, 2018
Closed
3 tasks
@starkwang starkwang mentioned this pull request Aug 27, 2018
Closed
@Trott Trott deleted the buffer-addon branch January 13, 2022 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fhinkel fhinkel fhinkel approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Trott @bnoordhuis @fhinkel @MylesBorins @addaleax @nodejs-github-bot