From b864a8c5663172f3f655b0360c007b1b02537876 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Wed, 9 Nov 2016 15:02:54 +0100 Subject: [PATCH] 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 | 38 +++++---- .../tests/unit/test_netutils.py | 80 ++++++++++++++++++- .../lldp-error-handling-5b6576b378ef9c3a.yaml | 6 ++ 3 files changed, 100 insertions(+), 24 deletions(-) create mode 100644 releasenotes/notes/lldp-error-handling-5b6576b378ef9c3a.yaml diff --git a/ironic_python_agent/netutils.py b/ironic_python_agent/netutils.py index 801833a39..c5a8bfb9d 100644 --- a/ironic_python_agent/netutils.py +++ b/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, ' - 'attempting to close any opened sockets.') - if self.__exit__(*sys.exc_info()): - return [] - else: - LOG.exception('Could not successfully close all opened ' - 'RawPromiscuousSockets.') - raise + LOG.error('Failed to open all RawPromiscuousSockets, ' + 'attempting to close any opened sockets.') + self.__exit__(*sys.exc_info()) + 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: - LOG.exception('Error while using raw socket: %(type)s: %(val)s', - {'type': exception_type, 'val': exception_val}) - - for _name, sock, ifr in self.interfaces: + 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 - fcntl.ioctl(sock.fileno(), SIOCSIFFLAGS, ifr) - sock.close() - # Return True to signify exit correctly, only used internally - return True + try: + fcntl.ioctl(sock.fileno(), SIOCSIFFLAGS, ifr) + sock.close() + except Exception: + 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) diff --git a/ironic_python_agent/tests/unit/test_netutils.py b/ironic_python_agent/tests/unit/test_netutils.py index 257a556e4..f9e0a59a6 100644 --- a/ironic_python_agent/tests/unit/test_netutils.py +++ b/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: - # Ensure this isn't run - self.assertEqual([], sockets) + def _run_with_bind_fail(): + with netutils.RawPromiscuousSockets(interfaces, protocol): + 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)) diff --git a/releasenotes/notes/lldp-error-handling-5b6576b378ef9c3a.yaml b/releasenotes/notes/lldp-error-handling-5b6576b378ef9c3a.yaml new file mode 100644 index 000000000..3355d0009 --- /dev/null +++ b/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).