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

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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion lib/buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,13 @@
'use strict';

const binding = process.binding('buffer');
const config = process.binding('config');
const { compare: compare_, compareOffset } = binding;
const { isArrayBuffer, isSharedArrayBuffer, isUint8Array } =
process.binding('util');
const bindingObj = {};
const internalUtil = require('internal/util');
const pendingDeprecation = !!config.pendingDeprecation;

class FastBuffer extends Uint8Array {
constructor(arg1, arg2, arg3) {
Expand Down Expand Up @@ -69,6 +71,15 @@ function createUnsafeArrayBuffer(size) {
}
}

function createRandomFillBuffer(size) {
zeroFill[0] = 3;
try {
return new ArrayBuffer(size);
} finally {
zeroFill[0] = 1;
}
}

function createPool() {
poolSize = Buffer.poolSize;
allocPool = createUnsafeArrayBuffer(poolSize);
Expand All @@ -85,6 +96,12 @@ function alignPool() {
}
}

var bufferWarn = true;
const bufferWarning = 'The Buffer() and new Buffer() constructors are not ' +
'recommended for use due to security and usability ' +
'concerns. Please use the new Buffer.alloc(), ' +
'Buffer.allocUnsafe(), or Buffer.from() construction ' +
'methods instead.';
/**
* The Buffer() construtor is deprecated in documentation and should not be
* used moving forward. Rather, developers should use one of the three new
Expand All @@ -97,13 +114,22 @@ function alignPool() {
**/
function Buffer(arg, encodingOrOffset, length) {
// Common case.
if (pendingDeprecation && bufferWarn) {
// This is a *pending* deprecation warning. It is not emitted by
// default unless the --pending-deprecation command-line flag is
// used or the NODE_PENDING_DEPRECATION=1 envvar is set.
process.emitWarning(bufferWarning, 'DeprecationWarning', 'DEP0005');
bufferWarn = false;
}

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));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}
return Buffer.from(arg, encodingOrOffset, length);
}
Expand Down
23 changes: 20 additions & 3 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,10 @@ bool trace_warnings = false;
// that is used by lib/module.js
bool config_preserve_symlinks = false;

// Set by ParseArgs when --pending-deprecation or NODE_PENDING_DEPRECATION
// is used.
bool config_pending_deprecation = false;

// Set in node.cc by ParseArgs when --redirect-warnings= is used.
std::string config_warning_file; // NOLINT(runtime/string)

Expand Down Expand Up @@ -1036,10 +1040,15 @@ Local<Value> WinapiErrnoException(Isolate* isolate,


void* ArrayBufferAllocator::Allocate(size_t size) {
if (zero_fill_field_ || zero_fill_all_buffers)
if (zero_fill_all_buffers || zero_fill_field_ == 1) {
return node::UncheckedCalloc(size);
else
return node::UncheckedMalloc(size);
} else if (zero_fill_field_ == 3) {
void* mem = node::UncheckedMalloc(size);
if (mem != nullptr)
memset(mem, random_fill_value_, size);
return mem;
}
return node::UncheckedMalloc(size);
}

static bool DomainHasErrorHandler(const Environment* env,
Expand Down Expand Up @@ -3739,6 +3748,8 @@ static void ParseArgs(int* argc,
short_circuit = true;
} else if (strcmp(arg, "--zero-fill-buffers") == 0) {
zero_fill_all_buffers = true;
} else if (strcmp(arg, "--pending-deprecation") == 0) {
config_pending_deprecation = true;
} else if (strcmp(arg, "--v8-options") == 0) {
new_v8_argv[new_v8_argc] = "--help";
new_v8_argc += 1;
Expand Down Expand Up @@ -4248,6 +4259,12 @@ void Init(int* argc,
V8::SetFlagsFromString(NODE_V8_OPTIONS, sizeof(NODE_V8_OPTIONS) - 1);
#endif

{
std::string text;
config_pending_deprecation =
SafeGetenv("NODE_PENDING_DEPRECATION", &text) && text[0] == '1';
}

// Allow for environment set preserving symlinks.
{
std::string text;
Expand Down
3 changes: 3 additions & 0 deletions src/node_config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ void InitConfig(Local<Object> target,
if (config_preserve_symlinks)
READONLY_BOOLEAN_PROPERTY("preserveSymlinks");

if (config_pending_deprecation)
READONLY_BOOLEAN_PROPERTY("pendingDeprecation");

if (!config_warning_file.empty()) {
Local<String> name = OneByteString(env->isolate(), "warningFile");
Local<String> value = String::NewFromUtf8(env->isolate(),
Expand Down
10 changes: 10 additions & 0 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ extern bool config_preserve_symlinks;
// it to stderr.
extern std::string config_warning_file; // NOLINT(runtime/string)

// Set in node.cc by ParseArgs when --pending-deprecation or
// NODE_PENDING_DEPRECATION is used
extern bool config_pending_deprecation;

// Tells whether it is safe to call v8::Isolate::GetCurrent().
extern bool v8_initialized;

Expand Down Expand Up @@ -196,6 +200,11 @@ inline bool IsBigEndian() {

class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
public:
ArrayBufferAllocator() {
unsigned int seed = time(NULL);
random_fill_value_ = rand_r(&seed) % 256;
}

inline uint32_t* zero_fill_field() { return &zero_fill_field_; }

virtual void* Allocate(size_t size); // Defined in src/node.cc
Expand All @@ -205,6 +214,7 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {

private:
uint32_t zero_fill_field_ = 1; // Boolean but exposed as uint32 to JS land.
uint8_t random_fill_value_;
};

// Clear any domain and/or uncaughtException handlers to force the error's
Expand Down
15 changes: 15 additions & 0 deletions test/parallel/test-buffer-pending-deprecation.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Flags: --pending-deprecation --no-warnings
'use strict';

const common = require('../common');
const Buffer = require('buffer').Buffer;

const bufferWarning = 'The Buffer() and new Buffer() constructors are not ' +
'recommended for use due to security and usability ' +
'concerns. Please use the new Buffer.alloc(), ' +
'Buffer.allocUnsafe(), or Buffer.from() construction ' +
'methods instead.';

common.expectWarning('DeprecationWarning', bufferWarning);

new Buffer(10);