-
Notifications
You must be signed in to change notification settings - Fork 524
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
websocket: don't use pooled buffer in mask pool #3357
Conversation
@@ -27,7 +27,7 @@ try { | |||
function generateMask () { | |||
if (bufIdx === BUFFER_SIZE) { | |||
bufIdx = 0 | |||
crypto.randomFillSync((buffer ??= Buffer.allocUnsafe(BUFFER_SIZE)), 0, BUFFER_SIZE) | |||
crypto.randomFillSync((buffer ??= Buffer.allocUnsafeSlow(BUFFER_SIZE)), 0, BUFFER_SIZE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, but with the current code, I have some concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see them, unless we acess buffer.buffer anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they access it, they can rewrite the mask, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
example:
import { WebSocket } from './index.js'
Buffer.poolSize = 16386 * 4
// any echo server url
const u = new WebSocket('ws://localhost:3000')
u.onmessage = (data) => {
// The mask can be rewritten here.
new Uint8Array(Buffer.allocUnsafe(1).buffer).fill(0)
u.send('Hi')
}
u.onopen = () => {
u.send('Hi')
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add a test?
Unfortunately, I can write a test, but this can't catch using pooled buffer completely. |
better something than nothing |
test was placed incorrectly. this can catch it until now! |
* upstream/main: fix: use explicit flag for when use has interacted with stream (nodejs#3361) refactor: simplify signal handling (nodejs#3362) fix: consider bytes read when dumping (nodejs#3360) websocket: don't use pooled buffer in mask pool (nodejs#3357) Revert "fix: post request signal (nodejs#3354)" (nodejs#3359) fix: post request signal (nodejs#3354) build(deps): bump node from `075a5cc` to `9af472b` in /build (nodejs#3355)
@mcollina