Issue #368: Bad packet size handling in built-in server handlers

ait/core/server/broker.py

@@ -1,8 +1,8 @@
-import zmq.green as zmq
 import gevent
-import gevent.monkey
+from gevent import monkey
+monkey.patch_all()
 
-gevent.monkey.patch_all()
+import zmq.green as zmq
 
 from typing import List, Any
 

ait/core/server/handlers/ccsds_packet_handler.py

@@ -42,10 +42,8 @@ def __init__(self, input_type=None, output_type=None, **kwargs):
         tlm_dict = tlm.getDefaultDict()
         for packet_name in self.packet_types.values():
             if packet_name not in tlm_dict.keys():
-                msg = "CCSDSPacketHandler: Packet name {} not present in telemetry dictionary.".format(
-                    packet_name
-                )
-                msg += " Available packet types are {}".format(tlm_dict.keys())
+                msg = f"CCSDSPacketHandler: Packet name {packet_name} not present in telemetry dictionary."
+                msg += f" Available packet types are {tlm_dict.keys()}"
                 raise ValueError(msg)
 
     def handle(self, input_data):
@@ -59,24 +57,20 @@ def handle(self, input_data):
         # Check if packet length is at least 7 bytes
         primary_header_length = 6
         if len(input_data) < primary_header_length + 1:
-            ait.core.log.info(
+            ait.core.log.error(
                 "CCSDSPacketHandler: Received packet length is less than minimum of 7 bytes."
             )
             return
 
         # Extract APID from packet
-        packet_apid = str(bin(int(binascii.hexlify(input_data[0:2]), 16) & 0x07FF))[
-            2:
-        ].zfill(11)
+        packet_apid = str(bin(int(binascii.hexlify(input_data[0:2]), 16) & 0x07FF))[2:].zfill(11)
 
         # Check if packet_apid matches with an APID in the config
         config_apid = self.comp_apid(packet_apid)
         if not config_apid:
-            msg = "CCSDSPacketHandler: Packet APID {} not present in config.".format(
-                packet_apid
-            )
-            msg += " Available packet APIDs are {}".format(self.packet_types.keys())
-            ait.core.log.info(msg)
+            msg = f"CCSDSPacketHandler: Packet APID {packet_apid} not present in config."
+            msg += f" Available packet APIDs are {self.packet_types.keys()}"
+            ait.core.log.error(msg)
             return
 
         # Map APID to packet name in config to get UID from telemetry dictionary
@@ -87,7 +81,7 @@ def handle(self, input_data):
         # Extract user data field from packet
         packet_data_length = int(binascii.hexlify(input_data[4:6]), 16) + 1
         if len(input_data) < primary_header_length + packet_data_length:
-            ait.core.log.info(
+            ait.core.log.error(
                 "CCSDSPacketHandler: Packet data length is less than stated length in packet primary header."
             )
             return

ait/core/server/handlers/packet_handler.py

@@ -19,27 +19,48 @@ def __init__(self, input_type=None, output_type=None, **kwargs):
                            If packet is specified but not present in default tlm dict.
         """
         super(PacketHandler, self).__init__(input_type, output_type)
-        self.packet = kwargs.get("packet", None)
+        self.packet_type = kwargs.get("packet", None)
+        self.tlm_dict = tlm.getDefaultDict()
 
-        if not self.packet:
-            msg = 'PacketHandler: No packet name provided in handler config as key "packet"'
+        if not self.packet_type:
+            msg = f'PacketHandler: No packet type provided in handler config as key "packet"'
             raise ValueError(msg)
 
-        tlm_dict = tlm.getDefaultDict()
-        if self.packet not in tlm_dict:
-            msg = "PacketHandler: Packet name {} not present in telemetry dictionary".format(
-                self.packet
-            )
-            msg += " Available packet types are {}".format(tlm_dict.keys())
+        if self.packet_type not in self.tlm_dict:
+            msg = f"PacketHandler: Packet name '{self.packet_type}' not present in telemetry dictionary."
+            msg += f" Available packet types are {self.tlm_dict.keys()}"
             raise ValueError(msg)
 
-        self._pkt_defn = tlm_dict[self.packet]
+        self._pkt_defn = self.tlm_dict[self.packet_type]
 
-    def handle(self, input_data):
+    def get_packet_lengths(self):
+        """
+        Makes a dictionary of packet.name : number of bytes in the packet
+            e.g.  'Ethernet_HS_Packet': 37
+
+        Return:  dictionary
+
+        """
+        pkt_len_dict = {}
+        for i in self.tlm_dict.keys():
+            pkt_len_dict[i] = self.tlm_dict[i].nbytes
+
+        return pkt_len_dict
+
+    def handle(self, packet):
         """
         Params:
-            input_data:   message received by stream
+            packet:   message received by stream (packet)
         Returns:
             tuple of packet UID and message received by stream
         """
-        return pickle.dumps((self._pkt_defn.uid, input_data), 2)
+
+        # TODO validate the packet (if this is the place to do the validation)
+
+        defined_packet_lengths = self.get_packet_lengths()
+
+        if defined_packet_lengths[self.packet_type] != packet.nbytes:
+            msg = f"PacketHandler: Packet length of packet does not match packet definition."
+            raise ValueError(msg)
+
+        return pickle.dumps((self._pkt_defn.uid, packet), 2)

ait/core/server/server.py

@@ -54,15 +54,15 @@ def __init__(self):
 
     def wait(self):
         """
-        Starts all greenlets and plugin-pocesses for concurrent processing.
+        Starts all greenlets and plugin-processes for concurrent processing.
         Joins over all greenlets that are not servers.
         """
-        # Start all of the greenlets managed by this process
+        # Start all greenlets managed by this process
         for greenlet in self.greenlets + self.servers:
             log.info(f"Starting {greenlet} greenlet...")
             greenlet.start()
 
-        # Start all of the separate plugin processes
+        # Start all separate plugin processes
         for plugin_process in self.plugin_processes:
             log.info(f"Spawning {plugin_process} process...")
             plugin_process.spawn_process()

ait/core/tlm.py

@@ -55,7 +55,6 @@ def __getitem__(self, key):
         """Returns the words in this wordarray at the given Python slice
         or word at the given integer index."""
         length = len(self)
-
         if isinstance(key, slice):
             return [self[n] for n in range(*key.indices(length))]
 
@@ -371,6 +370,7 @@ def validate(self, value, messages=None):
         Validation error messages are appended to an optional messages
         array.
         """
+
         valid = True
         primitive = value
 
@@ -520,7 +520,8 @@ def validate(self, messages=None):
         Validation error messages are appended to an optional messages
         array.
         """
-        return self._defn.validate(self, messages)
+
+        # return self._defn.validate(self, messages)
 
 
 class PacketContext:

tests/ait/core/server/test_handler.py

@@ -1,4 +1,5 @@
 import pickle
+import pytest
 import unittest
 from unittest import mock
 
@@ -14,28 +15,32 @@ def test_ccsds_packet_length(self):
         handler = CCSDSPacketHandler(packet_types={"01011100111": "CCSDS_HEADER"})
         data = bytearray(b"\x02\xE7\x40\x00\x00\x00")
         with self.assertLogs("ait", level="INFO") as cm:
-            result = handler.handle(data)
+            handler.handle(data)
         self.assertIn("less than minimum of 7 bytes", cm.output[0])
 
     # Check if APID match between config and packet
     def test_ccsds_packet_apid(self):
         handler = CCSDSPacketHandler(packet_types={"00000000000": "CCSDS_HEADER"})
         data = bytearray(b"\x02\xE7\x40\x00\x00\x00\x01")
         with self.assertLogs("ait", level="INFO") as cm:
-            result = handler.handle(data)
+            handler.handle(data)
         self.assertIn("not present in config", cm.output[0])
 
     # Check packet length vs header reported length
     def test_ccsds_packet_header(self):
         handler = CCSDSPacketHandler(packet_types={"01011100111": "CCSDS_HEADER"})
         data = bytearray(b"\x02\xE7\x40\x00\x00\x0F\x01")
         with self.assertLogs("ait", level="INFO") as cm:
-            result = handler.handle(data)
+            handler.handle(data)
         self.assertIn(
             "Packet data length is less than stated length in packet primary header",
             cm.output[0],
         )
 
+    def test_packet_name_not_present(self):
+        with pytest.raises(ValueError):
+            CCSDSPacketHandler(packet_types={"01011100111": "CCSDS_"})
+
     # Check if dumped uid match expected tlm dictionary uid
     def test_ccsds_packet_uid(self):
         handler = CCSDSPacketHandler(packet_types={"01011100111": "CCSDS_HEADER"})
@@ -62,6 +67,59 @@ def test_execute_handler_returns_handle_return_on_input(self, handle_mock):
         assert returned == "SpecialReturn"
 
 
+class TestHandlerClassWith1553HSPacket(object):
+    tlm_dict = tlm.getDefaultDict()
+    pkt_data = bytearray(b"\x02\xE7\x40\x00\x00\x00\x01")
+    pkt_1553 = tlm_dict['1553_HS_Packet']
+    handler = PacketHandler(pkt_data, packet="1553_HS_Packet")
+
+    def test_word_array(self):
+        packet = tlm.Packet(self.pkt_1553, self.pkt_data)
+        assert packet.words.__len__() == 3.5
+
+    def test_execute_handler_returns_handle_return_on_input(self):
+        packet = tlm.Packet(self.pkt_1553, self.pkt_data)
+        result = self.handler.handle(packet)
+        assert 'Ethernet 1553 packet' in str(result)
+
+    # Test packet length by sending a Ethernet_HS_Packet to a 1553_HS_Packet Handler
+    def test_bad_packet_length(self):
+        ethernet_pkt = self.tlm_dict['Ethernet_HS_Packet']
+        e_packet = tlm.Packet(ethernet_pkt, self.pkt_data)
+        with pytest.raises(ValueError):
+            self.handler.handle(e_packet)
+
+    def test_packet_name_error_and_no_packet_type(self):
+        pkt_data = bytearray(b"\x02\xE7\x40\x00\x00\x00\x01")
+        with pytest.raises(ValueError):
+            PacketHandler(pkt_data, packet="1553_HS_Packe")
+        with pytest.raises(ValueError):
+            PacketHandler(pkt_data)
+
+
+class TestHandlerClassWithEthernetHSPacket(object):
+    tlm_dict = tlm.getDefaultDict()
+    pkt_data = bytearray(b"\x02\xE7\x40\x00\x00\x00\x01\x07\x08\x0a")
+    ethernet_pkt = tlm_dict['Ethernet_HS_Packet']
+    handler = PacketHandler(pkt_data, packet="Ethernet_HS_Packet")
+
+    def test_word_array(self):
+        e_packet = tlm.Packet(self.ethernet_pkt, self.pkt_data)
+        assert e_packet.words.__len__() == 5
+
+    def test_execute_handler_returns_handle_return_on_input(self):
+        e_packet = tlm.Packet(self.ethernet_pkt, self.pkt_data)
+        result = self.handler.handle(e_packet)
+        assert 'Ethernet Health and Status Packet' in str(result)
+
+    # Send a 1553 packet to an Ethernet_HS_Packet Handler
+    def test_bad_packet_length(self):
+        pkt_1553 = self.tlm_dict['1553_HS_Packet']
+        packet = tlm.Packet(pkt_1553, self.pkt_data)
+        with pytest.raises(ValueError):
+            self.handler.handle(packet)
+
+
 class TestHandlerClassWithoutInputOutputTypes(object):
     handler = PacketHandler(packet="CCSDS_HEADER")