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

buffer: move setupBufferJS to internal #16391

Closed
wants to merge 1 commit into from
Closed

buffer: move setupBufferJS to internal #16391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion lib/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ const {
readDoubleLE: _readDoubleLE,
readFloatBE: _readFloatBE,
readFloatLE: _readFloatLE,
setupBufferJS,
swap16: _swap16,
swap32: _swap32,
swap64: _swap64,
Expand Down Expand Up @@ -63,6 +62,8 @@ const errors = require('internal/errors');

const internalBuffer = require('internal/buffer');

const { setupBufferJS } = internalBuffer;

const bindingObj = {};

class FastBuffer extends Uint8Array {
Expand Down
4 changes: 4 additions & 0 deletions lib/internal/bootstrap_node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,10 @@
}
});

// This, as side effect, removes `setupBufferJS` from the buffer binding,
// and exposes it on `internal/buffer`.
NativeModule.require('internal/buffer');

global.Buffer = NativeModule.require('buffer').Buffer;
process.domain = null;
process._exiting = false;
Expand Down
13 changes: 11 additions & 2 deletions lib/internal/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,13 @@
'use strict';

// This is needed still for FastBuffer
module.exports = {};
const binding = process.binding('buffer');
const { setupBufferJS } = binding;

// Remove from the binding so that function is only available as exported here.
// (That is, for internal use only.)
delete binding.setupBufferJS;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking, and up for debate:
Since this changes global state, maybe move this to node_bootstap, or explicitly require with a comment. Otherwise AFAICT it will run as a side effect of

node/lib/internal/bootstrap_node.js

Line 295 in 02a5267

global.Buffer = NativeModule.require('buffer').Buffer;
which is not obvius

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I'd like to keep the caching and removing of setupBufferJS in the same place. Would a comment in bootstrap_node.js clarifying that requiring buffer also removes the setupBufferJS from the binding be sufficient to clarify? Something like:

// Note that this requires `lib/internal/buffer.js`, which removes `setupBufferJS`
// from the buffer binding.
global.Buffer = NativeModule.require('buffer').Buffer;

Copy link
Contributor

Choose a reason for hiding this comment

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

Another viable option is to add this to bootstrap_node.js

// This require as side effect removes `setupBufferJS` from the buffer binding.
NativeModule.require('internal/buffer.js');

Copy link
Member Author

@bengl bengl Oct 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Cool, I'll throw a comment and explicit require in. Stay tuned.


// FastBuffer wil be inserted here by lib/buffer.js
module.exports = {
setupBufferJS
};
22 changes: 9 additions & 13 deletions test/parallel/test-buffer-bindingobj-no-zerofill.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,33 +11,26 @@ const assert = require('assert');
const buffer = require('buffer');

// Monkey-patch setupBufferJS() to have an undefined zeroFill.
const process = require('process');
const originalBinding = process.binding;
const internalBuffer = require('internal/buffer');

const binding = originalBinding('buffer');
const originalSetup = binding.setupBufferJS;
const originalSetup = internalBuffer.setupBufferJS;

binding.setupBufferJS = (proto, obj) => {
internalBuffer.setupBufferJS = (proto, obj) => {
originalSetup(proto, obj);
assert.strictEqual(obj.zeroFill[0], 1);
delete obj.zeroFill;
};

const bindingObj = {};

binding.setupBufferJS(Buffer.prototype, bindingObj);
internalBuffer.setupBufferJS(Buffer.prototype, bindingObj);
assert.strictEqual(bindingObj.zeroFill, undefined);

process.binding = (bindee) => {
if (bindee === 'buffer')
return binding;
return originalBinding(bindee);
};

// Load from file system because internal buffer is already loaded and we're
// testing code that runs on first load only.
// Do not move this require() to top of file. It is important that
// `process.binding('buffer').setupBufferJS` be monkey-patched before this runs.
// `require('internal/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
Expand All @@ -51,3 +44,6 @@ while (uninitialized.every((val) => val === 0))
// zero-fill in that case.
const zeroFilled = monkeyPatchedBuffer.Buffer.allocUnsafe(1024);
assert(zeroFilled.every((val) => val === 0));

// setupBufferJS shouldn't still be exposed on the binding
assert(!('setupBufferJs' in process.binding('buffer')));