net: multiple listen() events fail silently

eduardbme · mcollina · commit b24e269a4828 · 2017-09-04T12:41:54.000+02:00
Problem: It's possible to run listen() on a net.Server that's already listening to a port. The result is silent failure, with the side effect of changing the connectionKey and or pipeName. Solution: throw an error if listen method called more than once. close() method should be called between listen() method calls. Refs: #8294 Fixes: #6190 Fixes: #11685 PR-URL: #13149 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
diff --git a/doc/api/http.md b/doc/api/http.md
@@ -809,8 +809,9 @@ This function is asynchronous. `callback` will be added as a listener for the
 
 Returns `server`.
 
-*Note*: The `server.listen()` method may be called multiple times. Each
-subsequent call will *re-open* the server using the provided options.
+*Note*: The `server.listen()` method can be called again if and only if there was an error
+during the first `server.listen()` call or `server.close()` has been called.
+Otherwise, an `ERR_SERVER_ALREADY_LISTEN` error will be thrown.
 
 ### server.listen(path[, callback])
 <!-- YAML
@@ -825,8 +826,9 @@ Start a UNIX socket server listening for connections on the given `path`.
 This function is asynchronous. `callback` will be added as a listener for the
 [`'listening'`][] event.  See also [`net.Server.listen(path)`][].
 
-*Note*: The `server.listen()` method may be called multiple times. Each
-subsequent call will *re-open* the server using the provided options.
+*Note*: The `server.listen()` method can be called again if and only if there was an error
+during the first `server.listen()` call or `server.close()` has been called.
+Otherwise, an `ERR_SERVER_ALREADY_LISTEN` error will be thrown.
 
 ### server.listen([port][, hostname][, backlog][, callback])
 <!-- YAML
@@ -861,8 +863,9 @@ parameter is 511 (not 512).
 This function is asynchronous. `callback` will be added as a listener for the
 [`'listening'`][] event.  See also [`net.Server.listen(port)`][].
 
-*Note*: The `server.listen()` method may be called multiple times. Each
-subsequent call will *re-open* the server using the provided options.
+*Note*: The `server.listen()` method can be called again if and only if there was an error
+during the first `server.listen()` call or `server.close()` has been called.
+Otherwise, an `ERR_SERVER_ALREADY_LISTEN` error will be thrown.
 
 ### server.listening
 <!-- YAML
diff --git a/doc/api/net.md b/doc/api/net.md
@@ -199,9 +199,9 @@ on Linux. The default value of this parameter is 511 (not 512).
 
 * All [`net.Socket`][] are set to `SO_REUSEADDR` (See [socket(7)][] for
   details).
-
-* The `server.listen()` method may be called multiple times. Each
-  subsequent call will *re-open* the server using the provided options.
+* The `server.listen()` method can be called again if and only if there was an error
+  during the first `server.listen()` call or `server.close()` has been called.
+  Otherwise, an `ERR_SERVER_ALREADY_LISTEN` error will be thrown.
 
 One of the most common errors raised when listening is `EADDRINUSE`.
 This happens when another server is already listening on the requested
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -237,6 +237,8 @@ E('ERR_NO_ICU', '%s is not supported on Node.js compiled without ICU');
 E('ERR_NO_LONGER_SUPPORTED', '%s is no longer supported');
 E('ERR_OUTOFMEMORY', 'Out of memory');
 E('ERR_PARSE_HISTORY_DATA', 'Could not parse history data in %s');
+E('ERR_SERVER_ALREADY_LISTEN',
+  'Listen method has been called more than once without closing.');
 E('ERR_SOCKET_ALREADY_BOUND', 'Socket is already bound');
 E('ERR_SOCKET_BAD_PORT', 'Port should be > 0 and < 65536');
 E('ERR_SOCKET_BAD_TYPE',
diff --git a/lib/net.js b/lib/net.js
@@ -1423,6 +1423,10 @@ Server.prototype.listen = function(...args) {
   var options = normalized[0];
   var cb = normalized[1];
 
+  if (this._handle) {
+    throw new errors.Error('ERR_SERVER_ALREADY_LISTEN');
+  }
+
   var hasCallback = (cb !== null);
   if (hasCallback) {
     this.once('listening', cb);
diff --git a/test/parallel/test-net-server-call-listen-multiple-times.js b/test/parallel/test-net-server-call-listen-multiple-times.js
@@ -0,0 +1,51 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const net = require('net');
+
+// First test. Check that after error event you can listen right away.
+{
+  const dummyServer = net.Server();
+  const server = net.Server();
+
+  // Run some server in order to simulate EADDRINUSE error.
+  dummyServer.listen(common.mustCall(() => {
+    // Try to listen used port.
+    server.listen(dummyServer.address().port);
+  }));
+
+  server.on('error', common.mustCall((e) => {
+    assert.doesNotThrow(
+      () => server.listen(common.mustCall(() => {
+        dummyServer.close();
+        server.close();
+      }))
+    );
+  }));
+}
+
+// Second test. Check that second listen call throws an error.
+{
+  const server = net.Server();
+
+  server.listen(common.mustCall(() => server.close()));
+
+  common.expectsError(() => server.listen(), {
+    code: 'ERR_SERVER_ALREADY_LISTEN',
+    type: Error
+  });
+}
+
+// Third test.
+// Check that after the close call you can run listen method just fine.
+{
+  const server = net.Server();
+
+  server.listen(common.mustCall(() => {
+    server.close();
+    assert.doesNotThrow(
+      () => server.listen(common.mustCall(() => server.close()))
+    );
+  }));
+}
