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

net: make holding the buffer in memory more robust #8252

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@addaleax
Member

addaleax commented Aug 24, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

net/src?

Description of change

Set the req.buffer property, which serves as a way of keeping a Buffer alive that is being written to a stream, on the C++ side instead of the JS side.

This closes a hole where buffers that were temporarily created in order to write strings with uncommon encodings (e.g. hex) were passed to the native side without being set as req.buffer.

Fixes: #8251
CI: https://ci.nodejs.org/job/node-test-commit/4749/

net: make holding the buffer in memory more robust
Set the `req.buffer` property, which serves as a way of keeping
a `Buffer` alive that is being written to a stream, on the C++
side instead of the JS side.

This closes a hole where buffers that were temporarily created
in order to write strings with uncommon encodings (e.g. `hex`)
were passed to the native side without being set as `req.buffer`.

Fixes: #8251
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 24, 2016

Member

CI is green. LGTM

Member

jasnell commented Aug 24, 2016

CI is green. LGTM

@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Aug 24, 2016

@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Aug 24, 2016

Contributor

LGTM. Though also want to point out this'll add some nanoseconds overhead. :P

Contributor

trevnorris commented Aug 24, 2016

LGTM. Though also want to point out this'll add some nanoseconds overhead. :P

Show outdated Hide outdated test/parallel/test-net-write-fully-async-buffer.js
const conn = net.createConnection(this.address().port, common.mustCall(() => {
let count = 0;
function write_loop() {

This comment has been minimized.

@bnoordhuis

bnoordhuis Aug 24, 2016

Member

s/write_loop/writeLoop/ here and in the other file?

@bnoordhuis

bnoordhuis Aug 24, 2016

Member

s/write_loop/writeLoop/ here and in the other file?

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Aug 24, 2016

Member

LGTM but I wonder, doesn't valgrind squeal about uninitialized reads?

Member

bnoordhuis commented Aug 24, 2016

LGTM but I wonder, doesn't valgrind squeal about uninitialized reads?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 25, 2016

Member

I wonder, doesn't valgrind squeal about uninitialized reads?

It does, and I’m pretty sure it also does that for some other tests where Buffer.allocUnsafe() is being used. I’ve changed it to Buffer.alloc(), but I don’t feel super strongly about it.

new CI: https://ci.nodejs.org/job/node-test-commit/4758/

Member

addaleax commented Aug 25, 2016

I wonder, doesn't valgrind squeal about uninitialized reads?

It does, and I’m pretty sure it also does that for some other tests where Buffer.allocUnsafe() is being used. I’ve changed it to Buffer.alloc(), but I don’t feel super strongly about it.

new CI: https://ci.nodejs.org/job/node-test-commit/4758/

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 27, 2016

Member

Landed in 4863f6a

Member

addaleax commented Aug 27, 2016

Landed in 4863f6a

@addaleax addaleax closed this Aug 27, 2016

@addaleax addaleax deleted the addaleax:fix-efault-8251 branch Aug 27, 2016

addaleax added a commit that referenced this pull request Aug 27, 2016

net: make holding the buffer in memory more robust
Set the `req.buffer` property, which serves as a way of keeping
a `Buffer` alive that is being written to a stream, on the C++
side instead of the JS side.

This closes a hole where buffers that were temporarily created
in order to write strings with uncommon encodings (e.g. `hex`)
were passed to the native side without being set as `req.buffer`.

Fixes: #8251
PR-URL: #8252
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

addaleax added a commit to addaleax/node that referenced this pull request Aug 27, 2016

net: make holding the buffer in memory more robust
Set the `req.buffer` property, which serves as a way of keeping
a `Buffer` alive that is being written to a stream, on the C++
side instead of the JS side.

This closes a hole where buffers that were temporarily created
in order to write strings with uncommon encodings (e.g. `hex`)
were passed to the native side without being set as `req.buffer`.

Fixes: nodejs#8251
PR-URL: nodejs#8252
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Sep 4, 2016

net: make holding the buffer in memory more robust
Set the `req.buffer` property, which serves as a way of keeping
a `Buffer` alive that is being written to a stream, on the C++
side instead of the JS side.

This closes a hole where buffers that were temporarily created
in order to write strings with uncommon encodings (e.g. `hex`)
were passed to the native side without being set as `req.buffer`.

Fixes: #8251
PR-URL: #8252
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

net: make holding the buffer in memory more robust
Set the `req.buffer` property, which serves as a way of keeping
a `Buffer` alive that is being written to a stream, on the C++
side instead of the JS side.

This closes a hole where buffers that were temporarily created
in order to write strings with uncommon encodings (e.g. `hex`)
were passed to the native side without being set as `req.buffer`.

Fixes: nodejs#8251
PR-URL: nodejs#8252
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

net: make holding the buffer in memory more robust
Set the `req.buffer` property, which serves as a way of keeping
a `Buffer` alive that is being written to a stream, on the C++
side instead of the JS side.

This closes a hole where buffers that were temporarily created
in order to write strings with uncommon encodings (e.g. `hex`)
were passed to the native side without being set as `req.buffer`.

Fixes: #8251
PR-URL: #8252
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Sep 28, 2016

net: make holding the buffer in memory more robust
Set the `req.buffer` property, which serves as a way of keeping
a `Buffer` alive that is being written to a stream, on the C++
side instead of the JS side.

This closes a hole where buffers that were temporarily created
in order to write strings with uncommon encodings (e.g. `hex`)
were passed to the native side without being set as `req.buffer`.

Fixes: #8251
PR-URL: #8252
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

rvagg added a commit that referenced this pull request Oct 18, 2016

net: make holding the buffer in memory more robust
Set the `req.buffer` property, which serves as a way of keeping
a `Buffer` alive that is being written to a stream, on the C++
side instead of the JS side.

This closes a hole where buffers that were temporarily created
in order to write strings with uncommon encodings (e.g. `hex`)
were passed to the native side without being set as `req.buffer`.

Fixes: #8251
PR-URL: #8252
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Oct 26, 2016

net: make holding the buffer in memory more robust
Set the `req.buffer` property, which serves as a way of keeping
a `Buffer` alive that is being written to a stream, on the C++
side instead of the JS side.

This closes a hole where buffers that were temporarily created
in order to write strings with uncommon encodings (e.g. `hex`)
were passed to the native side without being set as `req.buffer`.

Fixes: #8251
PR-URL: #8252
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@MylesBorins MylesBorins referenced this pull request Oct 26, 2016

Closed

V4.6.2 proposal #9298

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment