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

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

Merged
merged 2 commits into from
Jun 2, 2016

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jun 1, 2016

R=@ChALkeR. I had to apply a small style fix-up to stop the linter from complaining.

I'm tagging this dont-land-on-anything but the test probably should be backported.

CI: https://ci.nodejs.org/job/node-test-pull-request/2891/

@bnoordhuis bnoordhuis added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. dont-land-on-v5.x labels Jun 1, 2016
@jasnell
Copy link
Member

jasnell commented Jun 1, 2016

LGTM.
@ChALkeR ... my apologies for missing the removal of the try/catch in the original PR. I saw the green CI and had incorrectly assumed that a test covering this was already being run.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 1, 2016

@bnoordhuis

I had to apply a small style fix-up to stop the linter from complaining.

If it's about missing ;, I believe I fixed that already in the original PR. If not — what was it?

@bnoordhuis
Copy link
Member Author

Yes, the missing semicolon.

@ChALkeR ChALkeR mentioned this pull request Jun 1, 2016
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!

@ChALkeR
Copy link
Member

ChALkeR commented Jun 1, 2016

Originally, that code looked like

flags[kNoZeroFill] = noZeroFill ? 1 : 0;
try {
  const ui8 = new Uint8Array(size);
  Object.setPrototypeOf(ui8, Buffer.prototype);
  return ui8;
} finally {
  flags[kNoZeroFill] = 0;
}

That way, we can keep it const. It's not important, though.

LGTM, thanks!

@ChALkeR
Copy link
Member

ChALkeR commented Jun 1, 2016

@jasnell It was my fault that the testcase was missing. I opened a separate issue to share my considerations about publishing testcases for security issues and to discuss how that should be done.

@ChALkeR ChALkeR mentioned this pull request Jun 1, 2016
2 tasks
@ChALkeR
Copy link
Member

ChALkeR commented Jun 1, 2016

Ah. @bnoordhuis, the first commit here misses a description.

dd67608 has an explanation why that try-finally was introduced.


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

@trevnorris
Copy link
Contributor

LGTM.

zero_fill_field_ = 1;
return malloc(size);
else
return malloc(size);
Copy link
Member

@RReverser RReverser Jun 1, 2016

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

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.

bnoordhuis and others added 2 commits June 2, 2016 09:21
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: nodejs#7093
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Test failed or zero-sized Buffer allocations not affecting subsequent
creations of typed arrays.

PR-URL: nodejs#7093
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@bnoordhuis bnoordhuis closed this Jun 2, 2016
@bnoordhuis bnoordhuis deleted the reset-zero-fill branch June 2, 2016 07:23
@bnoordhuis bnoordhuis merged commit fea3070 into nodejs:master Jun 2, 2016
@bnoordhuis
Copy link
Member Author

Landed with expanded commit log in 3549a5e...fea3070.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants