SRec: Fix record matching and entry point parsing

cle/backends/srec.py

@@ -14,7 +14,7 @@
 
 __all__ = ("SRec",)
 
-srec_regex = "S([0-9])([0-9a-fA-F]{{2}})([0-9a-fA-F]{{{addr_size}}})([0-9a-fA-F]{{4,64}})([0-9a-fA-F]{{2}})"
+srec_regex = "S([0-9])([0-9a-fA-F]{{2}})([0-9a-fA-F]{{{addr_size}}})([0-9a-fA-F]*)([0-9a-fA-F]{{2}})"
 SREC_ADDR_SIZE = {"0": 16, "1": 16, "5": 16, "9": 16, "2": 24, "6": 24, "8": 24, "3": 32, "7": 32}
 
 
@@ -41,28 +41,37 @@ def calc_checksum(data):
         sum(x.to for x in data)
 
     @staticmethod
-    def parse_record(line):
+    def parse_record(line: bytes) -> tuple[int, int, bytes]:
+        # The record type determines the width of the address field, so it has to be read before the
+        # rest of the record can be matched.
+        if len(line) < 2 or not line.startswith(b"S"):
+            raise CLEError(f"Invalid SRec record: {line}")
+        if chr(line[1]) not in SREC_ADDR_SIZE:
+            raise CLEError(f"Invalid SRec record type: {line}")
         addr_size = SREC_ADDR_SIZE[chr(line[1])]
         srec_re = re.compile(srec_regex.format(addr_size=addr_size // 4).encode())
-        m = srec_re.match(line)
+        # A record occupies exactly one line. Matching only a prefix would leave the groups describing
+        # something other than the bytes the checksum is computed over below.
+        m = srec_re.fullmatch(line)
         if not m:
             raise CLEError(f"Invalid SRec record: {line}")
-        my_cksum = 0
         rectype, count, addr, data, cksum = m.groups()
+        try:
+            # The checksum covers the byte count, the address and the data, that is, everything but the
+            # record type and the checksum itself. An odd number of digits there is not valid hex.
+            counted = binascii.unhexlify(line[2:-2])
+        except binascii.Error as error:
+            raise CLEError(f"Invalid SRec hexadecimal data: {line}") from error
         cksum = int(cksum, 16)
-        for d in binascii.unhexlify(line[2:-2]):
-            my_cksum = (my_cksum + d) % 256
-        my_cksum = 0xFF - my_cksum
+        my_cksum = 0xFF - (sum(counted) % 256)
         if my_cksum != cksum:
             raise CLEError(f"Invalid checksum: Computed {hex(my_cksum)}, found {hex(cksum)}")
-        count = int(count, 16) - ((addr_size // 8) + 1)
-        addr = int(addr, 16)
-        rectype = int(rectype, 16)
-        if data:
-            data = binascii.unhexlify(data)
-        if data and count != len(data):
-            raise CLEError("Data length field does not match length of actual data: " + line)
-        return rectype, addr, data
+        # The byte count covers the address, the data and the checksum.
+        data_size = int(count, 16) - (addr_size // 8 + 1)
+        data = binascii.unhexlify(data)
+        if data_size != len(data):
+            raise CLEError(f"Data length field does not match length of actual data: {line}")
+        return int(rectype, 16), int(addr, 16), data
 
     @staticmethod
     def coalesce_regions(regions):
@@ -115,6 +124,10 @@ def __init__(self, *args, ignore_missing_arch: bool = False, **kwargs):
             if rectype == SREC_HEADER:
                 continue
             if rectype in SREC_DATA:
+                if not data:
+                    # A data record whose data field is empty carries nothing to load. Backing it would
+                    # add an empty region and drag max_addr one byte below the record's own address.
+                    continue
                 addr += self._base_address
                 # l.debug("Loading %d bytes at " % len(data) + hex(addr))
                 # Raw data.  Put the bytes
@@ -124,7 +137,8 @@ def __init__(self, *args, ignore_missing_arch: bool = False, **kwargs):
                 max_addr = max(max_addr, addr + len(data) - 1)
             elif rectype in SREC_START_EXEC:
                 got_entry = True
-                self._entry = int.from_bytes(data, "big")
+                # A termination record carries the start address in its address field and has no data.
+                self._entry = addr
                 log.debug("Found entry point at %#x", self._entry)
                 self._initial_ip = self._entry
             else:

tests/test_srec.py

@@ -0,0 +1,125 @@
+from __future__ import annotations
+
+import io
+
+import pytest
+
+import cle
+from cle.errors import CLEError
+
+# Width of the address field of each record type, in bytes.
+_ADDRESS_BYTES = {
+    0: 2,
+    1: 2,
+    2: 3,
+    3: 4,
+    5: 2,
+    6: 3,
+    7: 4,
+    8: 3,
+    9: 2,
+}
+
+
+def _record(record_type: int, address: int, data: bytes = b"", *, byte_count_delta: int = 0) -> bytes:
+    """
+    Build one S-record. ``byte_count_delta`` corrupts the byte count field without touching anything else.
+    """
+    address_bytes = address.to_bytes(_ADDRESS_BYTES[record_type], "big")
+    byte_count = len(address_bytes) + len(data) + 1 + byte_count_delta
+    body = bytes([byte_count]) + address_bytes + data
+    checksum = ~sum(body) & 0xFF
+    return f"S{record_type}".encode() + body.hex().upper().encode() + f"{checksum:02X}".encode()
+
+
+def _load(*records: bytes) -> cle.Loader:
+    return cle.Loader(io.BytesIO(b"\n".join(records)), auto_load_libs=False, main_opts={"arch": "X86"})
+
+
+def test_srec_loads_one_byte_data_record():
+    loader = _load(_record(1, 0x8020, b"\xe9"), _record(9, 0x8020))
+
+    assert isinstance(loader.main_object, cle.SRec)
+    assert loader.main_object.max_addr == 0x8020
+    assert loader.memory.load(0x8020, 1) == b"\xe9"
+
+
+def test_srec_loads_objcopy_output():
+    # Three verbatim lines of `objcopy -O srec` output: the header naming the output file, a data
+    # record, and the termination record that carries the start address.
+    loader = _load(
+        b"S010000066617578776172652E73726563B1",
+        b"S3110804A0240000000000000000C0870408CB",
+        b"S70508048470FA",
+    )
+
+    assert loader.main_object.entry == 0x8048470
+    assert loader.memory.load(0x804A02C, 4) == b"\xc0\x87\x04\x08"
+
+
+def test_srec_loads_long_records():
+    # objcopy writes the output file name into the S0 header record, so a header record alone routinely
+    # carries more data than a record was previously allowed to hold.
+    data = bytes(range(40))
+    loader = _load(
+        _record(0, 0, b"a-file-name-long-enough-to-need-a-big-record"),
+        _record(1, 0x1000, data),
+        _record(9, 0x1000),
+    )
+
+    assert loader.memory.load(0x1000, len(data)) == data
+    assert loader.main_object.max_addr == 0x1000 + len(data) - 1
+
+
+def test_srec_ignores_a_data_record_that_carries_no_data():
+    # An empty data field is well formed; the record simply describes no memory.
+    loader = _load(_record(1, 0x2000), _record(1, 0x1000, b"\x90\x90"), _record(9, 0x1000))
+
+    srec = loader.main_object
+    assert isinstance(srec, cle.SRec)
+    assert srec.regions == [(0x1000, 2)]
+    assert srec.max_addr == 0x1001
+
+
+@pytest.mark.parametrize("record_type", (1, 2, 3))
+def test_srec_loads_data_record_of_every_address_width(record_type):
+    loader = _load(_record(record_type, 0x1000, b"\x90\x90"), _record(9, 0x1000))
+
+    assert loader.memory.load(0x1000, 2) == b"\x90\x90"
+
+
+@pytest.mark.parametrize(
+    ("record_type", "entry"),
+    (
+        (7, 0x12345678),
+        (8, 0x123456),
+        (9, 0x1234),
+    ),
+)
+def test_srec_termination_record_supplies_the_entry_point(record_type, entry):
+    # The start address lives in the address field of the termination record; its data field is empty.
+    loader = _load(_record(1, 0x1000, b"\x90"), _record(record_type, entry))
+
+    assert loader.main_object.entry == entry
+
+
+@pytest.mark.parametrize(
+    ("record", "message"),
+    (
+        (_record(1, 0x1000, b"\x90", byte_count_delta=1), "Data length field does not match"),
+        (b"S10410009000", "Invalid checksum"),
+        (b"S4041000905B", "Invalid SRec record type"),
+        (b"S104100009A5B", "Invalid SRec hexadecimal data"),
+        (_record(1, 0x1000, b"\x90\x90") + b" trailing", "Invalid SRec record"),
+        (b"", "Invalid SRec record"),
+    ),
+    ids=("byte-count-too-large", "bad-checksum", "unknown-record-type", "odd-digit-count", "trailing-junk", "empty"),
+)
+def test_srec_rejects_malformed_records(record, message):
+    with pytest.raises(CLEError, match=message):
+        cle.SRec.parse_record(record)
+
+
+def test_srec_load_reports_a_malformed_record():
+    with pytest.raises(CLEError, match="Invalid checksum"):
+        _load(_record(1, 0x1000, b"\x90"), b"S10410009000")