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
bnoordhuis 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 1 commit
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
Next Next commit
lib,src: reset zero fill flag on exception 
Exceptions thrown from the Uint8Array constructor would leave it
disabled.

Regression introduced in commit 27e84dd ("lib,src: clean up
ArrayBufferAllocator") from two days ago.  A follow-up commit
will add a regression test.

PR-URL: #7093
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
  • Loading branch information
@bnoordhuis
bnoordhuis committed Jun 2, 2016
commit 3a3996315c39b377d19db03434acf27d1d83cb98
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
Copy Markdown
Member

@ChALkeR ChALkeR Jun 1, 2016

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
Copy Markdown
Member Author

@bnoordhuis bnoordhuis Jun 1, 2016

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
Copy Markdown
Member

@ChALkeR ChALkeR Jun 1, 2016

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
Copy Markdown
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
Copy Markdown
Contributor

@trevnorris trevnorris Jun 2, 2016

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
Copy Markdown
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
Copy Markdown
Contributor

@trevnorris trevnorris Jun 2, 2016

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
Copy Markdown
Member

@ChALkeR ChALkeR Jun 2, 2016

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