From a89b6535433d72636d7a9888358975c588a89935 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 24 Aug 2016 15:14:04 +0200 Subject: [PATCH 1/2] 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: https://github.com/nodejs/node/issues/8251 --- lib/net.js | 1 - src/env.h | 1 + src/stream_base.cc | 1 + .../test-net-write-fully-async-buffer.js | 34 +++++++++++++++++++ .../test-net-write-fully-async-hex-string.js | 32 +++++++++++++++++ 5 files changed, 68 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-net-write-fully-async-buffer.js create mode 100644 test/parallel/test-net-write-fully-async-hex-string.js diff --git a/lib/net.js b/lib/net.js index a218ad17b48f43..298e7186aa6d60 100644 --- a/lib/net.js +++ b/lib/net.js @@ -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; diff --git a/src/env.h b/src/env.h index d8b6b8bb5fdfa2..0e301fb2741c97 100644 --- a/src/env.h +++ b/src/env.h @@ -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") \ diff --git a/src/stream_base.cc b/src/stream_base.cc index 105c4ad45895c0..585b84885c77ea 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -227,6 +227,7 @@ int StreamBase::WriteBuffer(const FunctionCallbackInfo& 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(); diff --git a/test/parallel/test-net-write-fully-async-buffer.js b/test/parallel/test-net-write-fully-async-buffer.js new file mode 100644 index 00000000000000..df7a8e7f0d8d61 --- /dev/null +++ b/test/parallel/test-net-write-fully-async-buffer.js @@ -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.allocUnsafe(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 write_loop() { + 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', write_loop); + + write_loop(); + })); +})); diff --git a/test/parallel/test-net-write-fully-async-hex-string.js b/test/parallel/test-net-write-fully-async-hex-string.js new file mode 100644 index 00000000000000..7460790ab16a37 --- /dev/null +++ b/test/parallel/test-net-write-fully-async-hex-string.js @@ -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.allocUnsafe(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 write_loop() { + 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', write_loop); + + write_loop(); + })); +})); From 332b6a00cf599e9dfbaed84d5412f9881dbb4075 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 25 Aug 2016 06:48:44 +0200 Subject: [PATCH 2/2] [squash] address nits --- test/parallel/test-net-write-fully-async-buffer.js | 8 ++++---- test/parallel/test-net-write-fully-async-hex-string.js | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/test/parallel/test-net-write-fully-async-buffer.js b/test/parallel/test-net-write-fully-async-buffer.js index df7a8e7f0d8d61..04fd1231fdc322 100644 --- a/test/parallel/test-net-write-fully-async-buffer.js +++ b/test/parallel/test-net-write-fully-async-buffer.js @@ -7,7 +7,7 @@ const common = require('../common'); const net = require('net'); -const data = Buffer.allocUnsafe(1000000); +const data = Buffer.alloc(1000000); const server = net.createServer(common.mustCall(function(conn) { conn.resume(); @@ -15,7 +15,7 @@ const server = net.createServer(common.mustCall(function(conn) { const conn = net.createConnection(this.address().port, common.mustCall(() => { let count = 0; - function write_loop() { + function writeLoop() { if (count++ === 200) { conn.destroy(); server.close(); @@ -27,8 +27,8 @@ const server = net.createServer(common.mustCall(function(conn) { // The buffer allocated above should still be alive. } - conn.on('drain', write_loop); + conn.on('drain', writeLoop); - write_loop(); + writeLoop(); })); })); diff --git a/test/parallel/test-net-write-fully-async-hex-string.js b/test/parallel/test-net-write-fully-async-hex-string.js index 7460790ab16a37..f3115d8d2f795e 100644 --- a/test/parallel/test-net-write-fully-async-hex-string.js +++ b/test/parallel/test-net-write-fully-async-hex-string.js @@ -5,7 +5,7 @@ const common = require('../common'); const net = require('net'); -const data = Buffer.allocUnsafe(1000000).toString('hex'); +const data = Buffer.alloc(1000000).toString('hex'); const server = net.createServer(common.mustCall(function(conn) { conn.resume(); @@ -13,7 +13,7 @@ const server = net.createServer(common.mustCall(function(conn) { const conn = net.createConnection(this.address().port, common.mustCall(() => { let count = 0; - function write_loop() { + function writeLoop() { if (count++ === 20) { conn.destroy(); server.close(); @@ -25,8 +25,8 @@ const server = net.createServer(common.mustCall(function(conn) { // The buffer allocated inside the .write() call should still be alive. } - conn.on('drain', write_loop); + conn.on('drain', writeLoop); - write_loop(); + writeLoop(); })); }));