Repository navigation
net: reject immutable onread buffers - #67394
fatihguzeldev wants to merge 1 commit into
Conversation
reject immutable buffers supplied directly or returned by an onread buffer factory before using them for socket reads. emit ERR_INVALID_ARG_VALUE without modifying the backing store. Refs: nodejs#66346 Signed-off-by: Fatih Güzel <fatihguzeldev@gmail.com>
|
Review requested:
|
nodejs-github-bot
added
c++
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #67394 +/- ##
==========================================
- Coverage 90.74% 90.74% -0.01%
==========================================
Files 798 798
Lines 280083 280119 +36
Branches 53900 53896 -4
==========================================
+ Hits 254166 254195 +29
+ Misses 16217 16211 -6
- Partials 9700 9713 +13
🚀 New features to boost your workflow:
|
| return req; | ||
| } | ||
|
|
||
| function onUserBufferError(stream, buffer) { |
There was a problem hiding this comment.
The name should be a bit specific....
| function onUserBufferError(stream, buffer) { | |
| function destroyWithImmutableArrayBufferError(stream, buffer) { |
| self[kBuffer] = userBuf; | ||
| } | ||
| self._handle.useUserBuffer(userBuf); | ||
| if (setUserBuffer(self, userBuf, true)) |
There was a problem hiding this comment.
Really not a fan of bool arguments, but if we're going to have them, please label call sites to make them clear (we're not consistent with that, at all... would love us to start)
| if (setUserBuffer(self, userBuf, true)) | |
| if (setUserBuffer(self, userBuf, true /*deferError*/)) |
There was a problem hiding this comment.
out of curiosity, is it mainly about readability at the call site, or are there other reasons you tend to avoid boolean parameters?
| streamBaseState, | ||
| } = internalBinding('stream_wrap'); | ||
| const { UV_EOF } = internalBinding('uv'); | ||
| const { isArrayBufferViewImmutable } = internalBinding('util'); |
There was a problem hiding this comment.
It's not available via primordials yet because it is flagged, but there is already an ArrayBuffer.prototype.immutable property that really should be used instead.
There was a problem hiding this comment.
thanks. i was able to deserialize an immutable arraybuffer with the flag off, but the getter isn't available there. would you recommend keeping the native check for that case?
socket reads can currently overwrite immutable storage passed
through onread.buffer, either directly or from a buffer factory.
reject these buffers before they are used for reads and report
ERR_INVALID_ARG_VALUE through the socket's error event.
refs: #66346