-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
lib,src: reset zero fill flag on exception #7093
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was my initial implementation. Problem is if There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. But
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); | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!