src: fix check for accepting Buffers into Node’s allocator #27174
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This condition was incorrect. We currently take the fallback path in default Node builds, which always works, but may come with some overhead, whereas the intention was that we use the fast path in this condition. This is causing issues for embedders, because we would erroneously try to take the fast path when they don’t provide a Node.js-style `ArrayBufferAlloactor`, and crash as a consequence of that. This also requires us to relax the check in the debugging ArrayBuffer allocator a bit, because since d117e41, 0-sized ArrayBuffers may actually point to allocations of size 1. Previously, that wasn’t caught because the fallback path circumvented our ArrayBufferAllocator. Refs: nodejs@84e02b1#r33116006
nodejs-github-bot
added
buffer
addaleax
referenced
this pull request
Apr 10, 2019
Always use the right allocator for memory that is turned into an `ArrayBuffer` at a later point. This enables embedders to use their own `ArrayBuffer::Allocator`s, and is inspired by Electron’s electron/node@f61bae3440e. It should render their downstream patch unnecessary. Refs: electron/node@f61bae3 PR-URL: #26207 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
joyeecheung
approved these changes
Apr 10, 2019
|
BTW, is it possible to cctest this now? |
|
It would be, but our cctest helpers aren’t really written to accommodate this… |
richardlau
approved these changes
Apr 10, 2019
cjihrig
approved these changes
Apr 10, 2019
| @@ -138,7 +138,11 @@ void DebuggingArrayBufferAllocator::UnregisterPointerInternal(void* data, | |||
| if (data == nullptr) return; | |||
| auto it = allocations_.find(data); | |||
| CHECK_NE(it, allocations_.end()); | |||
| CHECK_EQ(it->second, size); | |||
| if (size > 0) { | |||
There was a problem hiding this comment.
Would this be a good place to use CHECK_IMPLIES()?
There was a problem hiding this comment.
@cjihrig Does it make a difference? If I want to say “this check is only valid under these conditions”, then I am okay with using an if…
There was a problem hiding this comment.
It shouldn't make a difference. I just thought it might be a bit more concise and possibly (I'm not sure) simplify code coverage.
There was a problem hiding this comment.
Yeah, I’d prefer to keep this as it is here. I’ve noticed this because of failing tests, so I think code coverage isn’t an issue either.
jasnell
approved these changes
Apr 10, 2019
addaleax
added
the
author ready
devsnek
approved these changes
Apr 14, 2019
|
Landed in 427fce7 |
addaleax
added a commit
that referenced
this pull request
Apr 14, 2019
This condition was incorrect. We currently take the fallback path in default Node builds, which always works, but may come with some overhead, whereas the intention was that we use the fast path in this condition. This is causing issues for embedders, because we would erroneously try to take the fast path when they don’t provide a Node.js-style `ArrayBufferAlloactor`, and crash as a consequence of that. This also requires us to relax the check in the debugging ArrayBuffer allocator a bit, because since d117e41, 0-sized ArrayBuffers may actually point to allocations of size 1. Previously, that wasn’t caught because the fallback path circumvented our ArrayBufferAllocator. Refs: 84e02b1#r33116006 PR-URL: #27174 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
This was referenced Apr 23, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This condition was incorrect. We currently take the fallback
path in default Node builds, which always works, but may come with
some overhead, whereas the intention was that we use the fast path
in this condition.
This is causing issues for embedders, because we would erroneously
try to take the fast path when they don’t provide a Node.js-style
ArrayBufferAlloactor, and crash as a consequence of that.This also requires us to relax the check in the debugging ArrayBuffer
allocator a bit, because since d117e41, 0-sized ArrayBuffers
may actually point to allocations of size 1. Previously, that wasn’t
caught because the fallback path circumvented our ArrayBufferAllocator.
Refs: 84e02b1#r33116006
/cc @zcbenz
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes