From ed21d32a7c37d030ce4668415832a89dc8ead34c Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Wed, 25 Mar 2020 22:03:42 +0100
Subject: [PATCH] net: wait for shutdown to complete before closing

When not allowing half open, handle.close would be
invoked before shutdown has been called and
completed causing a potential data race.

Fixes: https://github.com/nodejs/node/issues/32486#issuecomment-604072559

PR-URL: https://github.com/nodejs/node/pull/32491
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
---
 lib/internal/stream_base_commons.js       | 10 +++++++++
 lib/net.js                                |  6 +++---
 test/async-hooks/test-graph.tls-write.js  |  4 +---
 test/parallel/test-net-allow-half-open.js | 25 +++++++++++++++++++++++
 4 files changed, 39 insertions(+), 6 deletions(-)
 create mode 100644 test/parallel/test-net-allow-half-open.js

diff --git a/lib/internal/stream_base_commons.js b/lib/internal/stream_base_commons.js
index 800ba4cefd6370..2a31221f11d2d6 100644
--- a/lib/internal/stream_base_commons.js
+++ b/lib/internal/stream_base_commons.js
@@ -213,6 +213,16 @@ function onStreamRead(arrayBuffer) {
     if (stream[kMaybeDestroy])
       stream.on('end', stream[kMaybeDestroy]);
 
+    // TODO(ronag): Without this `readStop`, `onStreamRead`
+    // will be called once more (i.e. after Readable.ended)
+    // on Windows causing a ECONNRESET, failing the
+    // test-https-truncate test.
+    if (handle.readStop) {
+      const err = handle.readStop();
+      if (err)
+        return stream.destroy(errnoException(err, 'read'));
+    }
+
     // Push a null to signal the end of data.
     // Do it before `maybeDestroy` for correct order of events:
     // `end` -> `close`
diff --git a/lib/net.js b/lib/net.js
index 21d17702b53798..39dc526641e216 100644
--- a/lib/net.js
+++ b/lib/net.js
@@ -629,9 +629,9 @@ function onReadableStreamEnd() {
     this.write = writeAfterFIN;
     if (this.writable)
       this.end();
-  }
-
-  if (!this.destroyed && !this.writable && !this.writableLength)
+    else if (!this.writableLength)
+      this.destroy();
+  } else if (!this.destroyed && !this.writable && !this.writableLength)
     this.destroy();
 }
 
diff --git a/test/async-hooks/test-graph.tls-write.js b/test/async-hooks/test-graph.tls-write.js
index f8bee6a879d0b4..078dd27c36088c 100644
--- a/test/async-hooks/test-graph.tls-write.js
+++ b/test/async-hooks/test-graph.tls-write.js
@@ -65,9 +65,7 @@ function onexit() {
       { type: 'TCPCONNECTWRAP',
         id: 'tcpconnect:1', triggerAsyncId: 'tcp:1' },
       { type: 'TCPWRAP', id: 'tcp:2', triggerAsyncId: 'tcpserver:1' },
-      { type: 'TLSWRAP', id: 'tls:2', triggerAsyncId: 'tcpserver:1' },
-      { type: 'Immediate', id: 'immediate:1', triggerAsyncId: 'tcp:2' },
-      { type: 'Immediate', id: 'immediate:2', triggerAsyncId: 'tcp:1' },
+      { type: 'TLSWRAP', id: 'tls:2', triggerAsyncId: 'tcpserver:1' }
     ]
   );
 }
diff --git a/test/parallel/test-net-allow-half-open.js b/test/parallel/test-net-allow-half-open.js
new file mode 100644
index 00000000000000..8f68c0105a15dc
--- /dev/null
+++ b/test/parallel/test-net-allow-half-open.js
@@ -0,0 +1,25 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const net = require('net');
+
+const server = net.createServer(common.mustCall((socket) => {
+  socket.end(Buffer.alloc(1024));
+})).listen(0, common.mustCall(() => {
+  const socket = net.connect(server.address().port);
+  assert.strictEqual(socket.allowHalfOpen, false);
+  socket.resume();
+  socket.on('end', common.mustCall(() => {
+    process.nextTick(() => {
+      // Ensure socket is not destroyed straight away
+      // without proper shutdown.
+      assert(!socket.destroyed);
+      server.close();
+    });
+  }));
+  socket.on('finish', common.mustCall(() => {
+    assert(!socket.destroyed);
+  }));
+  socket.on('close', common.mustCall());
+}));