From ca9c1b94ea7556e22304a8e2a9b9bb4c52c904b5 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Wed, 24 Aug 2016 15:14:04 +0200
Subject: [PATCH] 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
PR-URL: https://github.com/nodejs/node/pull/8252
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
---
 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 b51c05dd407b45..53128f93e444bb 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<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();
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..04fd1231fdc322
--- /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.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();
+  }));
+}));
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..f3115d8d2f795e
--- /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.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();
+  }));
+}));