master: fix crashes in identification due to buggy nodes

- check address conflicts
- on invalid values, reject peer instead of dying

neo/master/handlers/__init__.py

@@ -18,7 +18,7 @@ from neo.lib import logging
 from neo.lib.exception import StoppedOperation
 from neo.lib.handler import EventHandler
 from neo.lib.protocol import (uuid_str, NodeTypes, NodeStates, Packets,
-    BrokenNodeDisallowedError,
+    BrokenNodeDisallowedError, ProtocolError,
 )
 
 class MasterHandler(EventHandler):
@@ -33,12 +33,11 @@ class MasterHandler(EventHandler):
         app = self.app
         node = app.nm.getByUUID(uuid)
         if node:
-            assert node_type is not NodeTypes.MASTER or node.getAddress() in (
-                address, None), (node, address)
+            if node_type is NodeTypes.MASTER and not (
+               None != address == node.getAddress()):
+                raise ProtocolError
             if node.isBroken():
                 raise BrokenNodeDisallowedError
-        else:
-            node = app.nm.getByAddress(address)
         peer_uuid = self._setupNode(conn, node_type, uuid, address, node)
         if app.primary:
             primary_address = app.server

neo/master/handlers/election.py

@@ -126,8 +126,8 @@ class ServerElectionHandler(BaseElectionHandler, MasterHandler):
             logging.info('reject a connection from a non-master')
             raise NotReadyError
 
-        if node is None:
-            node = app.nm.createMaster(address=address)
+        if node is None is app.nm.getByAddress(address):
+            app.nm.createMaster(address=address)
 
         self.elect(conn, address)
         return uuid

neo/master/handlers/identification.py

@@ -30,18 +30,32 @@ class IdentificationHandler(MasterHandler):
 
     def _setupNode(self, conn, node_type, uuid, address, node):
         app = self.app
-        if node:
-            if node.isRunning():
-                if uuid > 0:
-                    # cloned/evil/buggy node connecting to us
-                    raise ProtocolError('already connected')
+        by_addr = address and app.nm.getByAddress(address)
+        while 1:
+            if by_addr:
+                if not by_addr.isConnected():
+                    if node is by_addr:
+                        break
+                    if not node or uuid < 0:
+                        # In case of address conflict for a peer with temporary
+                        # ids, we'll generate a new id.
+                        node = by_addr
+                        break
+            elif node:
+                if node.isConnected():
+                    if uuid < 0:
                         # The peer wants a temporary id that's already assigned.
                         # Let's give it another one.
                         node = uuid = None
+                        break
                 else:
-                assert not node.isConnected()
                     node.setAddress(address)
-                node.setRunning()
+                    break
+                # Id conflict for a storage node.
+            else:
+                break
+            # cloned/evil/buggy node connecting to us
+            raise ProtocolError('already connected')
 
         state = NodeStates.RUNNING
         if node_type == NodeTypes.CLIENT:
@@ -68,13 +82,14 @@ class IdentificationHandler(MasterHandler):
             handler = app.administration_handler
             human_readable_node_type = 'n admin '
         else:
-            raise NotImplementedError(node_type)
+            raise ProtocolError
 
         uuid = app.getNewUUID(uuid, address, node_type)
         logging.info('Accept a' + human_readable_node_type + uuid_str(uuid))
         if node is None:
             node = app.nm.createFromNodeType(node_type,
                 uuid=uuid, address=address)
+        else:
             node.setUUID(uuid)
         node.setState(state)
         node.setConnection(conn)

[Kirill Smelkov]

@jm tests?