lib,src: reset zero fill flag on exception #7093
Conversation
bnoordhuis
added
buffer
|
LGTM. |
If it's about missing |
|
Yes, the missing semicolon. |
| try { | ||
| var ui8 = new Uint8Array(size); | ||
| } finally { | ||
| if (noZeroFill) |
There was a problem hiding this comment.
'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.
@bnoordhuis Ah, that makes sense. Thanks!
|
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 LGTM, thanks! |
|
@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. |
|
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); |
There was a problem hiding this comment.
is the .fill(0) just precautionary in case it hasn't been properly zero filled?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
LGTM. |
| zero_fill_field_ = 1; | ||
| return malloc(size); | ||
| else | ||
| return malloc(size); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That was my initial implementation. Problem is if new Uint8Array() for some reason throws it'll stay flipped.
There was a problem hiding this comment.
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.
@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.
@trevnorris No, somewhy I didn't see your comment and was adressing @RReverser comment.
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: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Test failed or zero-sized Buffer allocations not affecting subsequent creations of typed arrays. PR-URL: nodejs#7093 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
bnoordhuis
force-pushed
the
reset-zero-fill
branch
from
1590da7
to
fea3070
Compare
|
Landed with expanded commit log in 3549a5e...fea3070. |
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/