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: #32486 (comment) PR-URL: #32491 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
nodejs · Apr 11, 2020 · 3c8bf9022a805234f9172a57e88dab6ce8ba354d · 3c8bf90
1 parent 9ee2afa
commit 3c8bf9022a805234f9172a57e88dab6ce8ba354d
Showing with 39 additions and 6 deletions.

+10 −0 lib/internal/stream_base_commons.js

+3 −3 lib/net.js

+1 −3 test/async-hooks/test-graph.tls-write.js

+25 −0 test/parallel/test-net-allow-half-open.js
diff --git 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
@@ -631,9 +631,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
@@ -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
@@ -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());
+}));