Skip to content

lib,src: reset zero fill flag on exception #7093

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

Merged
merged 2 commits into from
Jun 2, 2016
Merged

lib,src: reset zero fill flag on exception #7093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a39963
lib,src: reset zero fill flag on exception
bnoordhuis Jun 1, 2016
fea3070
test: add buffer testcase for resetting kZeroFill
ChALkeR Jun 1, 2016
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
11 changes: 9 additions & 2 deletions lib/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,15 @@ const zeroFill = bindingObj.zeroFill || [0];

function createBuffer(size, noZeroFill) {
if (noZeroFill)
zeroFill[0] = 0; // Reset by the runtime.
const ui8 = new Uint8Array(size);
zeroFill[0] = 0;

try {
var ui8 = new Uint8Array(size);
} finally {
if (noZeroFill)
Copy link
Member

Choose a reason for hiding this comment

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

Is an extra if needed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

'Needed', no, but I added it for symmetry with the check above and because it saves a bounds check.

Copy link
Member

Choose a reason for hiding this comment

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

@bnoordhuis Ah, that makes sense. Thanks!

zeroFill[0] = 1;
}

Object.setPrototypeOf(ui8, Buffer.prototype);
return ui8;
}
Expand Down
4 changes: 2 additions & 2 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -973,8 +973,8 @@ Local<Value> WinapiErrnoException(Isolate* isolate,
void* ArrayBufferAllocator::Allocate(size_t size) {
if (zero_fill_field_ || zero_fill_all_buffers)
return calloc(size, 1);
zero_fill_field_ = 1;
return malloc(size);
else
return malloc(size);
Copy link
Member

@RReverser RReverser Jun 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Can't we just reset zero-flag here instead of delegating to JS side, right before the malloc so that allocation exception couldn't happen yet? This would lead to less changes + would avoid try-catch deopt in createBuffer.

Copy link
Contributor

Choose a reason for hiding this comment

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

That was my initial implementation. Problem is if new Uint8Array() for some reason throws it'll stay flipped.

Copy link
Member

@ChALkeR ChALkeR Jun 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure of that. try-finally also checks that no non-failing shortcuts (that return an empty array) result in the flag not being reset.

Do you have an example that passes the tests here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ChALkeR Are you addressing my comment? I'm saying I reset the bit in C++ and I believe you were the one that realized the bit can be flipped and remain flipped if the allocation fails.

Copy link
Member

Choose a reason for hiding this comment

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

@trevnorris No, somewhy I didn't see your comment and was adressing @RReverser comment.

}

static bool DomainHasErrorHandler(const Environment* env,
Expand Down
23 changes: 23 additions & 0 deletions test/parallel/test-buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1476,3 +1476,26 @@ assert.equal(SlowBuffer.prototype.offset, undefined);
// Check pool offset after that by trying to write string into the pool.
assert.doesNotThrow(() => Buffer.from('abc'));
}


// Test failed or zero-sized Buffer allocations not affecting typed arrays
{
const zeroArray = new Uint32Array(10).fill(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

is the .fill(0) just precautionary in case it hasn't been properly zero filled?

Copy link
Member

Choose a reason for hiding this comment

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

This testcase ensures that typed arrays are zero filled, by comparing them to a typed array that is surely zero-filled.

If something breaks and typed arrays become non zero filled, then this testcase should fail. Without .fill(0) a change where this and following typed arrays become filled with equivalent garbage (e.g. with a constant number due to some of the previous tests) will slip through.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. But

a change where this and following typed arrays become filled with equivalent garbage

the two allocations would need to be filled with exactly the same garbage, for several allocations. Though I see what you're getting at.

const sizes = [1e10, 0, 0.1, -1, 'a', undefined, null, NaN];
const allocators = [
Buffer,
SlowBuffer,
Buffer.alloc,
Buffer.allocUnsafe,
Buffer.allocUnsafeSlow
];
for (const allocator of allocators) {
for (const size of sizes) {
try {
allocator(size);
} catch (e) {
assert.deepStrictEqual(new Uint32Array(10), zeroArray);
}
}
}
}