buffer: auto random fill Buffer(num) and new Buffer(num) #11806

jasnell · 2017-03-11T23:36:53Z

@addaleax @nodejs/ctc ...

This PR does a couple things as a proposal to bring some closure to #9531

As suggested by @addaleax, a pseudo-random byte value is selected at startup and used to auto-fill new Buffer(num) and Buffer(num).
A new --pending-deprecation command-line flag and NODE_PENDING_DEPRECATION=1 environment variable is added that allows conditional off-by-default pending deprecation warnings.
A pending deprecation warning is added for Buffer() and new Buffer(). This is off by default and will only be emitted when the new command line flag is set.

These are separated out into multiple commits.

There is a definite significant performance hit to new Buffer(num) and Buffer(num) using this.

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)

buffer

addaleax · 2017-03-11T23:53:40Z

src/node.cc

-    return node::UncheckedMalloc(size);
+  } else if (zero_fill_field_ == 3) {
+    void* mem = node::UncheckedMalloc(size);
+    memset(mem, random_fill_value_, size);


You’ll need a mem != nullptr check around this ;)

doh... I always forget about that.

addaleax · 2017-03-11T23:57:26Z

Hm, my first intuition is that a flag specifically for the buffer constructor itself, or specific to security-related API usage, would be better than one for general “pending” deprecations?

addaleax · 2017-03-11T23:58:25Z

lib/buffer.js

  if (typeof arg === 'number') {
    if (typeof encodingOrOffset === 'string') {
      throw new Error(
        'If encoding is specified then the first argument must be a string'
      );
    }
-    return Buffer.allocUnsafe(arg);
+    assertSize(arg);
+    return new FastBuffer(createRandomFillBuffer(arg));


Part of the performance hit might be that this stops using pooled allocation. Is that intentional?

Yes, but it needn't I suppose. This was largely just to keep it simple and consistent. If we happen to pull off the pooled allocation, do you imagine we would just zero fill or use the randomly selected value to fill?

@jasnell The best way that comes to mind for me right now is to have separate pools for uninitialized & for random-filled Buffers … alternatively, we could just keep using Buffer.allocUnsafe() here and fill manually.

Yeah. I'm leaning in that direction to be honest (specifically, using Buffer.alloc(size, fill)). It would be much less disruptive and wouldn't require the underlying c/c++ change. I did it this way first to see what the perf would be like and it is way too much of a hit

I'm going to open a second PR that uses this other method. I'd like us to get some comparison benchmark runs on various platforms. Definitely think going with the alloc then fill in js is going to be the better option tho.

jasnell · 2017-03-12T00:06:48Z

Regarding the flag, I'd really rather not have yet another special-case buffer-specific flag but certainly not completely opposed to it.

Command line flag that can be used to indicate that pending deprecations should be emitted.

The pending deprecation warning is off by default. Launch the node process with --pending-deprecation or NODE_PENDING_DEPRECATION=1 env var set.

ChALkeR · 2017-03-12T08:07:29Z

@jasnell, note that #7152 had some prototype changes to avoid breaking valid usecases. Is that concern still applicable? /cc @seishun

seishun · 2017-03-12T09:03:14Z

There is a definite significant performance hit to new Buffer(num) and Buffer(num) using this.

It should be noted that this makes it a breaking change for performance-critical applications, and by introducing it in 8.0.0 we won't be giving people time to migrate as described in our deprecation policy.

This isn't to say that this is necessarily a bad solution, but I think it's an important fact that shouldn't be overlooked.

ChALkeR

This currently prints a warning on Buffer.alloc(10).map(x => x + 1).

It shouldn't do that. (Perhaps include that as a testcase?)

@seishun had some work-around in #7152, but I didn't check that one.

jasnell · 2017-03-12T14:03:13Z

I'm going to close this one in favor of the alternate which is much less disruptive overall and has a better perf profile.

jasnell added wip Issues and PRs that are still a work in progress. semver-major PRs that contain breaking changes and should be released in the next major version. labels Mar 11, 2017

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 Mar 11, 2017

addaleax reviewed Mar 11, 2017

View reviewed changes

jasnell added 3 commits March 11, 2017 16:40

buffer: random fill when using Buffer(num)

12cf538

src: add --pending-deprecation and NODE_PENDING_DEPRECATION

53b1253

Command line flag that can be used to indicate that pending deprecations should be emitted.

buffer: add pending deprecation warning

e89e556

The pending deprecation warning is off by default. Launch the node process with --pending-deprecation or NODE_PENDING_DEPRECATION=1 env var set.

jasnell force-pushed the fill-buffers branch from a7158c9 to e89e556 Compare March 12, 2017 00:41

jasnell mentioned this pull request Mar 12, 2017

buffer: auto random fill Buffer(num) and new Buffer(num) #11808

Closed

3 tasks

ChALkeR mentioned this pull request Mar 12, 2017

buffer: runtime-deprecate Buffer constructor #7152

Closed

2 tasks

ChALkeR suggested changes Mar 12, 2017

View reviewed changes

jasnell closed this Mar 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: auto random fill Buffer(num) and new Buffer(num) #11806

buffer: auto random fill Buffer(num) and new Buffer(num) #11806

jasnell commented Mar 11, 2017 •

edited

Loading

addaleax Mar 11, 2017

jasnell Mar 12, 2017

addaleax commented Mar 11, 2017

addaleax Mar 11, 2017

jasnell Mar 12, 2017

addaleax Mar 12, 2017

jasnell Mar 12, 2017

jasnell Mar 12, 2017

jasnell commented Mar 12, 2017

ChALkeR commented Mar 12, 2017

seishun commented Mar 12, 2017

ChALkeR left a comment •

edited

Loading

jasnell commented Mar 12, 2017

buffer: auto random fill Buffer(num) and new Buffer(num) #11806

buffer: auto random fill Buffer(num) and new Buffer(num) #11806

Conversation

jasnell commented Mar 11, 2017 • edited Loading

Checklist

Affected core subsystem(s)

addaleax Mar 11, 2017

Choose a reason for hiding this comment

jasnell Mar 12, 2017

Choose a reason for hiding this comment

addaleax commented Mar 11, 2017

addaleax Mar 11, 2017

Choose a reason for hiding this comment

jasnell Mar 12, 2017

Choose a reason for hiding this comment

addaleax Mar 12, 2017

Choose a reason for hiding this comment

jasnell Mar 12, 2017

Choose a reason for hiding this comment

jasnell Mar 12, 2017

Choose a reason for hiding this comment

jasnell commented Mar 12, 2017

ChALkeR commented Mar 12, 2017

seishun commented Mar 12, 2017

ChALkeR left a comment • edited Loading

Choose a reason for hiding this comment

jasnell commented Mar 12, 2017

jasnell commented Mar 11, 2017 •

edited

Loading

ChALkeR left a comment •

edited

Loading