Permalink
Browse files

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>
  • Loading branch information...
addaleax authored and Fishrock123 committed Aug 24, 2016
1 parent 6f20f47 commit 3ef8ba8bdc8426518e500187e5d5ff7707ed31d6
View
@@ -694,7 +694,6 @@ Socket.prototype._writeGeneric = function(writev, data, encoding, cb) {
} else {
var enc;
if (data instanceof Buffer) {
req.buffer = data; // Keep reference alive.
enc = 'buffer';
} else {
enc = encoding;
View
@@ -69,6 +69,7 @@ namespace node {
V(args_string, "args") \
V(async, "async") \
V(async_queue_string, "_asyncQueue") \
V(buffer_string, "buffer") \
V(bytes_string, "bytes") \
V(bytes_parsed_string, "bytesParsed") \
V(bytes_read_string, "bytesRead") \
View
@@ -227,6 +227,7 @@ int StreamBase::WriteBuffer(const FunctionCallbackInfo<Value>& args) {
err = DoWrite(req_wrap, bufs, count, nullptr);
req_wrap_obj->Set(env->async(), True(env->isolate()));
req_wrap_obj->Set(env->buffer_string(), args[1]);
if (err)
req_wrap->Dispose();
@@ -0,0 +1,34 @@
'use strict';
// Flags: --expose-gc
// Note: This is a variant of test-net-write-fully-async-hex-string.js.
// This always worked, but it seemed appropriate to add a test that checks the
// behaviour for Buffers, too.
const common = require('../common');
const net = require('net');
const data = Buffer.alloc(1000000);
const server = net.createServer(common.mustCall(function(conn) {
conn.resume();
})).listen(0, common.mustCall(function() {
const conn = net.createConnection(this.address().port, common.mustCall(() => {
let count = 0;
function writeLoop() {
if (count++ === 200) {
conn.destroy();
server.close();
return;
}
while (conn.write(Buffer.from(data)));
global.gc(true);
// The buffer allocated above should still be alive.
}
conn.on('drain', writeLoop);
writeLoop();
}));
}));
@@ -0,0 +1,32 @@
'use strict';
// Flags: --expose-gc
// Regression test for https://github.com/nodejs/node/issues/8251.
const common = require('../common');
const net = require('net');
const data = Buffer.alloc(1000000).toString('hex');
const server = net.createServer(common.mustCall(function(conn) {
conn.resume();
})).listen(0, common.mustCall(function() {
const conn = net.createConnection(this.address().port, common.mustCall(() => {
let count = 0;
function writeLoop() {
if (count++ === 20) {
conn.destroy();
server.close();
return;
}
while (conn.write(data, 'hex'));
global.gc(true);
// The buffer allocated inside the .write() call should still be alive.
}
conn.on('drain', writeLoop);
writeLoop();
}));
}));

0 comments on commit 3ef8ba8

Please sign in to comment.