Skip to content

Commit b864a8c

Browse files
dtantsurdtantsur
committed
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
1 parent bcc98df commit b864a8c

3 files changed

Lines changed: 100 additions & 24 deletions

File tree

‎ironic_python_agent/netutils.py‎

Lines changed: 18 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -72,30 +72,24 @@ def __enter__(self):
7272
'proto': self.protocol})
7373
sock.bind((interface_name, self.protocol))
7474
except Exception:
75-
LOG.warning('Failed to open all RawPromiscuousSockets, '
76-
'attempting to close any opened sockets.')
77-
if self.__exit__(*sys.exc_info()):
78-
return []
79-
else:
80-
LOG.exception('Could not successfully close all opened '
81-
'RawPromiscuousSockets.')
82-
raise
75+
LOG.error('Failed to open all RawPromiscuousSockets, '
76+
'attempting to close any opened sockets.')
77+
self.__exit__(*sys.exc_info())
78+
raise
79+
8380
# No need to return each interfaces ifreq.
8481
return [(sock[0], sock[1]) for sock in self.interfaces]
8582

8683
def __exit__(self, exception_type, exception_val, trace):
87-
if exception_type:
88-
LOG.exception('Error while using raw socket: %(type)s: %(val)s',
89-
{'type': exception_type, 'val': exception_val})
90-
91-
for _name, sock, ifr in self.interfaces:
84+
for name, sock, ifr in self.interfaces:
9285
# bitwise or with the opposite of promiscuous mode to remove
9386
ifr.ifr_flags &= ~IFF_PROMISC
94-
# If these raise, they shouldn't be caught
95-
fcntl.ioctl(sock.fileno(), SIOCSIFFLAGS, ifr)
96-
sock.close()
97-
# Return True to signify exit correctly, only used internally
98-
return True
87+
try:
88+
fcntl.ioctl(sock.fileno(), SIOCSIFFLAGS, ifr)
89+
sock.close()
90+
except Exception:
91+
LOG.exception('Failed to close raw socket for interface %s',
92+
name)
9993

10094
def _get_socket(self):
10195
return socket.socket(socket.AF_PACKET, socket.SOCK_RAW, self.protocol)
@@ -128,14 +122,18 @@ def _parse_tlv(buff):
128122
14 bytes)
129123
"""
130124
lldp_info = []
131-
while buff:
125+
while len(buff) >= 2:
132126
# TLV structure: type (7 bits), length (9 bits), val (0-511 bytes)
133127
tlvhdr = struct.unpack('!H', buff[:2])[0]
134128
tlvtype = (tlvhdr & 0xfe00) >> 9
135129
tlvlen = (tlvhdr & 0x01ff)
136130
tlvdata = buff[2:tlvlen + 2]
137131
buff = buff[tlvlen + 2:]
138132
lldp_info.append((tlvtype, tlvdata))
133+
134+
if buff:
135+
LOG.warning("Trailing byte received in an LLDP package: %r", buff)
136+
139137
return lldp_info
140138

141139

@@ -148,7 +146,7 @@ def _receive_lldp_packets(sock):
148146
pkt = sock.recv(1600)
149147
# Filter invalid packets
150148
if not pkt or len(pkt) < 14:
151-
return
149+
return []
152150
# Skip header (dst MAC, src MAC, ethertype)
153151
pkt = pkt[14:]
154152
return _parse_tlv(pkt)

‎ironic_python_agent/tests/unit/test_netutils.py‎

Lines changed: 76 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,52 @@ def test_get_lldp_info_empty(self, sock_mock, select_mock, fcntl_mock):
220220
# 2 interfaces, 2 calls to enter promiscuous mode, 1 to leave
221221
self.assertEqual(6, fcntl_mock.call_count)
222222

223+
@mock.patch('fcntl.ioctl')
224+
@mock.patch('select.select')
225+
@mock.patch('socket.socket')
226+
def test_get_lldp_info_malformed(self, sock_mock, select_mock, fcntl_mock):
227+
expected_lldp = {
228+
'eth1': [],
229+
'eth0': [],
230+
}
231+
232+
interface_names = ['eth0', 'eth1']
233+
234+
sock1 = mock.Mock()
235+
sock1.recv.return_value = b'123456789012345' # odd size
236+
sock1.fileno.return_value = 4
237+
sock2 = mock.Mock()
238+
sock2.recv.return_value = b'1234' # too small
239+
sock2.fileno.return_value = 5
240+
241+
sock_mock.side_effect = [sock1, sock2]
242+
243+
select_mock.side_effect = [
244+
([sock1], [], []),
245+
([sock2], [], []),
246+
]
247+
248+
lldp_info = netutils.get_lldp_info(interface_names)
249+
self.assertEqual(expected_lldp, lldp_info)
250+
251+
sock1.bind.assert_called_with(('eth0', netutils.LLDP_ETHERTYPE))
252+
sock2.bind.assert_called_with(('eth1', netutils.LLDP_ETHERTYPE))
253+
254+
sock1.recv.assert_called_with(1600)
255+
sock2.recv.assert_called_with(1600)
256+
257+
self.assertEqual(1, sock1.close.call_count)
258+
self.assertEqual(1, sock2.close.call_count)
259+
260+
# 2 interfaces, 2 calls to enter promiscuous mode, 1 to leave
261+
self.assertEqual(6, fcntl_mock.call_count)
262+
263+
expected_calls = [
264+
mock.call([sock1, sock2], [], [], cfg.CONF.lldp_timeout),
265+
mock.call([sock2], [], [], cfg.CONF.lldp_timeout),
266+
]
267+
self.assertEqual(expected_calls, select_mock.call_args_list)
268+
223269
@mock.patch('fcntl.ioctl')
224270
@mock.patch('socket.socket')
225271
def test_raw_promiscuous_sockets(self, sock_mock, fcntl_mock):
@@ -251,11 +297,37 @@ def test_raw_promiscuous_sockets_bind_fail(self, sock_mock, fcntl_mock):
251297
sock2 = mock.Mock()
252298

253299
sock_mock.side_effect = [sock1, sock2]
254-
sock_mock.bind.side_effects = [None, Exception]
300+
sock2.bind.side_effect = RuntimeError()
255301

256-
with netutils.RawPromiscuousSockets(interfaces, protocol) as sockets:
257-
# Ensure this isn't run
258-
self.assertEqual([], sockets)
302+
def _run_with_bind_fail():
303+
with netutils.RawPromiscuousSockets(interfaces, protocol):
304+
self.fail('Unreachable code')
305+
306+
self.assertRaises(RuntimeError, _run_with_bind_fail)
307+
308+
sock1.bind.assert_called_once_with(('eth0', protocol))
309+
sock2.bind.assert_called_once_with(('ens9f1', protocol))
310+
311+
self.assertEqual(6, fcntl_mock.call_count)
312+
313+
sock1.close.assert_called_once_with()
314+
sock2.close.assert_called_once_with()
315+
316+
@mock.patch('fcntl.ioctl')
317+
@mock.patch('socket.socket')
318+
def test_raw_promiscuous_sockets_exception(self, sock_mock, fcntl_mock):
319+
interfaces = ['eth0', 'ens9f1']
320+
protocol = 3
321+
sock1 = mock.Mock()
322+
sock2 = mock.Mock()
323+
324+
sock_mock.side_effect = [sock1, sock2]
325+
326+
def _run_with_exception():
327+
with netutils.RawPromiscuousSockets(interfaces, protocol):
328+
raise RuntimeError()
329+
330+
self.assertRaises(RuntimeError, _run_with_exception)
259331

260332
sock1.bind.assert_called_once_with(('eth0', protocol))
261333
sock2.bind.assert_called_once_with(('ens9f1', protocol))

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
fixes:
3+
- Fix LLDP discovery to not fail completely when odd number of bytes is
4+
received in an LLDP package.
5+
- Fix raw sockets code to properly propagate exceptions to a caller instead
6+
of silencing them and returning None (causing failures later).

0 commit comments

Comments
 (0)