Fix several errors in LLDP handling code

* Stop silencing exceptions in raw socket context manager
* Correctly handle receiving packages with odd size and too small ones
* Fix a unit test that was testing nothing due to bad mocking

Change-Id: Ic8626d10618f52d50667d2698f34a92f5dcac33e
Closes-Bug: #1640238

ironic_python_agent/netutils.py

@@ -72,30 +72,24 @@ class RawPromiscuousSockets(object):
                                         'proto': self.protocol})
                 sock.bind((interface_name, self.protocol))
             except Exception:
-                LOG.warning('Failed to open all RawPromiscuousSockets, '
+                LOG.error('Failed to open all RawPromiscuousSockets, '
-                            'attempting to close any opened sockets.')
+                          'attempting to close any opened sockets.')
-                if self.__exit__(*sys.exc_info()):
+                self.__exit__(*sys.exc_info())
-                    return []
+                raise
-                else:
+
-                    LOG.exception('Could not successfully close all opened '
-                                  'RawPromiscuousSockets.')
-                    raise
         # No need to return each interfaces ifreq.
         return [(sock[0], sock[1]) for sock in self.interfaces]
 
     def __exit__(self, exception_type, exception_val, trace):
-        if exception_type:
+        for name, sock, ifr in self.interfaces:
-            LOG.exception('Error while using raw socket: %(type)s: %(val)s',
-                          {'type': exception_type, 'val': exception_val})
-
-        for _name, sock, ifr in self.interfaces:
             # bitwise or with the opposite of promiscuous mode to remove
             ifr.ifr_flags &= ~IFF_PROMISC
-            # If these raise, they shouldn't be caught
+            try:
-            fcntl.ioctl(sock.fileno(), SIOCSIFFLAGS, ifr)
+                fcntl.ioctl(sock.fileno(), SIOCSIFFLAGS, ifr)
-            sock.close()
+                sock.close()
-        # Return True to signify exit correctly, only used internally
+            except Exception:
-        return True
+                LOG.exception('Failed to close raw socket for interface %s',
+                              name)
 
     def _get_socket(self):
         return socket.socket(socket.AF_PACKET, socket.SOCK_RAW, self.protocol)
@@ -128,7 +122,7 @@ def _parse_tlv(buff):
                  14 bytes)
     """
     lldp_info = []
-    while buff:
+    while len(buff) >= 2:
         # TLV structure: type (7 bits), length (9 bits), val (0-511 bytes)
         tlvhdr = struct.unpack('!H', buff[:2])[0]
         tlvtype = (tlvhdr & 0xfe00) >> 9
@@ -136,6 +130,10 @@ def _parse_tlv(buff):
         tlvdata = buff[2:tlvlen + 2]
         buff = buff[tlvlen + 2:]
         lldp_info.append((tlvtype, tlvdata))
+
+    if buff:
+        LOG.warning("Trailing byte received in an LLDP package: %r", buff)
+
     return lldp_info
 
 
@@ -148,7 +146,7 @@ def _receive_lldp_packets(sock):
     pkt = sock.recv(1600)
     # Filter invalid packets
     if not pkt or len(pkt) < 14:
-        return
+        return []
     # Skip header (dst MAC, src MAC, ethertype)
     pkt = pkt[14:]
     return _parse_tlv(pkt)

ironic_python_agent/tests/unit/test_netutils.py

@@ -220,6 +220,52 @@ class TestNetutils(test_base.BaseTestCase):
         # 2 interfaces, 2 calls to enter promiscuous mode, 1 to leave
         self.assertEqual(6, fcntl_mock.call_count)
 
+    @mock.patch('fcntl.ioctl')
+    @mock.patch('select.select')
+    @mock.patch('socket.socket')
+    def test_get_lldp_info_malformed(self, sock_mock, select_mock, fcntl_mock):
+        expected_lldp = {
+            'eth1': [],
+            'eth0': [],
+        }
+
+        interface_names = ['eth0', 'eth1']
+
+        sock1 = mock.Mock()
+        sock1.recv.return_value = b'123456789012345'  # odd size
+        sock1.fileno.return_value = 4
+        sock2 = mock.Mock()
+        sock2.recv.return_value = b'1234'  # too small
+        sock2.fileno.return_value = 5
+
+        sock_mock.side_effect = [sock1, sock2]
+
+        select_mock.side_effect = [
+            ([sock1], [], []),
+            ([sock2], [], []),
+        ]
+
+        lldp_info = netutils.get_lldp_info(interface_names)
+        self.assertEqual(expected_lldp, lldp_info)
+
+        sock1.bind.assert_called_with(('eth0', netutils.LLDP_ETHERTYPE))
+        sock2.bind.assert_called_with(('eth1', netutils.LLDP_ETHERTYPE))
+
+        sock1.recv.assert_called_with(1600)
+        sock2.recv.assert_called_with(1600)
+
+        self.assertEqual(1, sock1.close.call_count)
+        self.assertEqual(1, sock2.close.call_count)
+
+        # 2 interfaces, 2 calls to enter promiscuous mode, 1 to leave
+        self.assertEqual(6, fcntl_mock.call_count)
+
+        expected_calls = [
+            mock.call([sock1, sock2], [], [], cfg.CONF.lldp_timeout),
+            mock.call([sock2], [], [], cfg.CONF.lldp_timeout),
+        ]
+        self.assertEqual(expected_calls, select_mock.call_args_list)
+
     @mock.patch('fcntl.ioctl')
     @mock.patch('socket.socket')
     def test_raw_promiscuous_sockets(self, sock_mock, fcntl_mock):
@@ -251,11 +297,37 @@ class TestNetutils(test_base.BaseTestCase):
         sock2 = mock.Mock()
 
         sock_mock.side_effect = [sock1, sock2]
-        sock_mock.bind.side_effects = [None, Exception]
+        sock2.bind.side_effect = RuntimeError()
 
-        with netutils.RawPromiscuousSockets(interfaces, protocol) as sockets:
+        def _run_with_bind_fail():
-            # Ensure this isn't run
+            with netutils.RawPromiscuousSockets(interfaces, protocol):
-            self.assertEqual([], sockets)
+                self.fail('Unreachable code')
+
+        self.assertRaises(RuntimeError, _run_with_bind_fail)
+
+        sock1.bind.assert_called_once_with(('eth0', protocol))
+        sock2.bind.assert_called_once_with(('ens9f1', protocol))
+
+        self.assertEqual(6, fcntl_mock.call_count)
+
+        sock1.close.assert_called_once_with()
+        sock2.close.assert_called_once_with()
+
+    @mock.patch('fcntl.ioctl')
+    @mock.patch('socket.socket')
+    def test_raw_promiscuous_sockets_exception(self, sock_mock, fcntl_mock):
+        interfaces = ['eth0', 'ens9f1']
+        protocol = 3
+        sock1 = mock.Mock()
+        sock2 = mock.Mock()
+
+        sock_mock.side_effect = [sock1, sock2]
+
+        def _run_with_exception():
+            with netutils.RawPromiscuousSockets(interfaces, protocol):
+                raise RuntimeError()
+
+        self.assertRaises(RuntimeError, _run_with_exception)
 
         sock1.bind.assert_called_once_with(('eth0', protocol))
         sock2.bind.assert_called_once_with(('ens9f1', protocol))

releasenotes/notes/lldp-error-handling-5b6576b378ef9c3a.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - Fix LLDP discovery to not fail completely when odd number of bytes is
+    received in an LLDP package.
+  - Fix raw sockets code to properly propagate exceptions to a caller instead
+    of silencing them and returning None (causing failures later).