Uh oh!
There was an error while loading. Please reload this page.
buffer: don't set kNoZeroFill flag in allocUnsafe - #6007
Conversation
evanlucas
commented
Apr 1, 2016
Maybe add a regression test? |
MylesBorins
commented
Apr 1, 2016
Tested locally and can confirm this fixes master + v5 |
jasnell
commented
Apr 1, 2016
Regression test for sure. Otherwise LGTM. |
cjihrig
commented
Apr 1, 2016
LGTM pending a test. |
82396ea to
6a3236aComparevkurchatkin
commented
Apr 1, 2016
Added some tests |
evanlucas
commented
Apr 1, 2016
LGTM if CI is happy |
jasnell
commented
Apr 1, 2016
MylesBorins
commented
Apr 1, 2016
LGTM |
There was a problem hiding this comment.
ha! sigh... I had uint8 on the brain I guess. good catch.
There was a problem hiding this comment.
Also -0 could be a factor, but I'm not sure. Any way, tests are a bit non-deterministic, since even if there is no actual zero filling, there is still a chance that some allocations would be zeros
There was a problem hiding this comment.
Yeah, that came up in the original review. It's not a great test. The plan was to revisit to see if the test can be made more robust.
There was a problem hiding this comment.
Also for some reason this bug is hard to reproduce with small Uint8Array.
There was a problem hiding this comment.
Looking at it now this one should have been obvious :-/. The bug only shows up after doing a pooled unsafeAlloc allocation because the zero fill flag was never being reset (because a real allocation wasn't being done).
(update: ha! I see you noted that in your commit log... it's definitely a friday)
If `kNoZeroFill` is set here, it won't be reset in case of pooled allocation. In case of "slow" allocation it will be set later anyway. Fixes: nodejs#6006
6a3236a to
abb9c4dComparejasnell
commented
Apr 2, 2016
CI looks good. One unrelated failure. |
If `kNoZeroFill` is set here, it won't be reset in case of pooled allocation. In case of "slow" allocation it will be set later anyway. Fixes: #6006 PR-URL: #6007 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
jasnell
commented
Apr 2, 2016
Landed in 0dcb026 |
jasnell
commented
Apr 2, 2016
Will cherry-pick this into v5.x as well. |
If `kNoZeroFill` is set here, it won't be reset in case of pooled allocation. In case of "slow" allocation it will be set later anyway. Fixes: #6006 PR-URL: #6007 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
indutny
commented
Apr 2, 2016
CVE? |
Pull Request check-list
make -j8 test(UNIX) orvcbuild test nosign(Windows) pass withthis change (including linting)?
test (or a benchmark) included?
existing APIs, or introduces new ones)?
Affected core subsystem(s)
buffer
Description of change
If
kNoZeroFillis set here, it won't be reset in case ofpooled allocation. In case of "slow" allocation it will be
set later anyway.
Fixes: #6006