From 3162cd67f4e91e653681be554e9d0aa58ddb9789 Mon Sep 17 00:00:00 2001 From: Gijs Molenaar Date: Fri, 11 Sep 2026 08:55:20 +0200 Subject: [PATCH 1/2] fix(s7commplus): accept unprefixed response values --- CHANGES.md | 2 ++ s7commplus/connection.py | 41 +++++++++++++++++++++++++++------------- tests/test_s7_v2.py | 15 +++++++++++++-- 3 files changed, 43 insertions(+), 15 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index f664baeb..07221d3d 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -6,6 +6,8 @@ CHANGES Major release: new `s7commplus` package with S7CommPlus protocol support. +* Accept GetVarSubStreamed responses whose PValue immediately follows the + return value, as captured from an S7-1511C during SessionKey legitimation. * Decode corroborating CPU execution attributes so S7CommPlus `get_cpu_state()` distinguishes RUN from STOP on S7-1500 and returns UNKNOWN for absent or inconsistent state attributes, including S7-1200 responses that omit them. diff --git a/s7commplus/connection.py b/s7commplus/connection.py index 39a73833..d458c7ca 100644 --- a/s7commplus/connection.py +++ b/s7commplus/connection.py @@ -46,6 +46,7 @@ import struct import tempfile from collections import deque +from collections.abc import Callable from types import TracebackType from typing import Any, Optional, Type @@ -255,6 +256,23 @@ def _build_get_var_substreamed_payload( return payload +def _response_pvalue_offset(payload: bytes, offset: int, supported: Callable[[int, int], bool]) -> int: + """Locate a response PValue with or without the legacy leading zero marker. + + Captured PLC responses use both layouts. Prefer a PValue beginning directly + after ReturnValue, then accept the older zero-prefixed form emitted by the + emulator and present in earlier captures. + """ + if offset + 2 <= len(payload) and supported(payload[offset], payload[offset + 1]): + return offset + + if payload[offset : offset + 1] == b"\x00": + offset += 1 + if offset + 2 > len(payload): + raise ValueError("missing PValue header") + return offset + + def _parse_get_var_substreamed_response(payload: bytes) -> bytes: """Extract the typed value from a GetVarSubStreamed response payload.""" from snap7.error import S7ConnectionError @@ -264,13 +282,11 @@ def _parse_get_var_substreamed_response(payload: bytes) -> bytes: if return_value != 0: raise S7ConnectionError(f"GetVarSubStreamed failed: return_value=0x{return_value:X}") - offset = consumed - if offset >= len(payload): - raise ValueError("missing response marker") - offset += 1 # protocol-defined unknown byte - - if offset + 2 > len(payload): - raise ValueError("missing PValue header") + offset = _response_pvalue_offset( + payload, + consumed, + lambda flags, datatype: datatype == DataType.BLOB or (datatype == DataType.USINT and bool(flags & 0x10)), + ) flags = payload[offset] datatype = payload[offset + 1] offset += 2 @@ -302,12 +318,11 @@ def _parse_protection_level_response(payload: bytes) -> int: if return_value != 0: raise S7ConnectionError(f"GetVarSubStreamed for the protection level failed: return_value={return_value}") - if offset >= len(payload): - raise ValueError("missing response marker") - offset += 1 # protocol-defined unknown byte - - if offset + 2 > len(payload): - raise ValueError("missing PValue header") + offset = _response_pvalue_offset( + payload, + offset, + lambda flags, datatype: datatype == DataType.UDINT and not flags & 0x10, + ) flags = payload[offset] datatype = payload[offset + 1] offset += 2 diff --git a/tests/test_s7_v2.py b/tests/test_s7_v2.py index 9b70719e..df05f31d 100644 --- a/tests/test_s7_v2.py +++ b/tests/test_s7_v2.py @@ -711,6 +711,12 @@ def test_parse_get_var_substreamed_usint_array(self) -> None: assert _parse_get_var_substreamed_response(response) == challenge + def test_parse_get_var_substreamed_usint_array_without_legacy_marker(self) -> None: + """Accept the response captured from the S7-1511C in GH-872.""" + response = bytes.fromhex("00100214ac214373925d0fc9a8ef730c908fcaa2863f8abe0400000000") + + assert _parse_get_var_substreamed_response(response) == bytes.fromhex("ac214373925d0fc9a8ef730c908fcaa2863f8abe") + def test_parse_get_var_substreamed_blob(self) -> None: challenge = bytes(range(16)) response = bytes([0x00, 0x00, 0x00, DataType.BLOB, 0x00]) @@ -991,12 +997,17 @@ class TestProtectionLevel: def test_parse_scalar_udint(self) -> None: assert _parse_protection_level_response(self.RESPONSE) == AccessLevel.NO_ACCESS + def test_parse_scalar_udint_without_legacy_marker(self) -> None: + response = bytes.fromhex("000004040700000000") + + assert _parse_protection_level_response(response) == AccessLevel.NO_ACCESS + def test_parse_rejects_nonzero_return(self) -> None: with pytest.raises(S7ConnectionError, match="return_value=4660"): _parse_protection_level_response(encode_uint32_vlq(0x1234)) - def test_parse_rejects_missing_response_marker(self) -> None: - with pytest.raises(S7ConnectionError, match="missing response marker"): + def test_parse_rejects_missing_pvalue(self) -> None: + with pytest.raises(S7ConnectionError, match="missing PValue header"): _parse_protection_level_response(bytes([0x00])) def test_parse_rejects_truncated_pvalue_header(self) -> None: From 9f8039093bf017c0ea25d1755df7b0b6de9a4f3d Mon Sep 17 00:00:00 2001 From: Gijs Molenaar Date: Fri, 11 Sep 2026 10:09:35 +0200 Subject: [PATCH 2/2] fix(s7commplus): correct legacy absolute DB reads --- CHANGES.md | 2 ++ s7commplus/client.py | 27 +++++++++++++++++++++++++++ s7commplus/connection.py | 5 +++++ s7commplus/server.py | 9 +++++++-- tests/test_s7_unit.py | 30 ++++++++++++++++++++++++++++++ 5 files changed, 71 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 07221d3d..35df82b9 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,8 @@ Major release: new `s7commplus` package with S7CommPlus protocol support. * Accept GetVarSubStreamed responses whose PValue immediately follows the return value, as captured from an S7-1511C during SessionKey legitimation. +* Encode legacy SessionKey absolute DB reads with one address ID so scalar + reads are not misinterpreted as nested symbolic paths by S7-1500 PLCs. * Decode corroborating CPU execution attributes so S7CommPlus `get_cpu_state()` distinguishes RUN from STOP on S7-1500 and returns UNKNOWN for absent or inconsistent state attributes, including S7-1200 responses that omit them. diff --git a/s7commplus/client.py b/s7commplus/client.py index b1879452..e2ece4af 100644 --- a/s7commplus/client.py +++ b/s7commplus/client.py @@ -219,6 +219,16 @@ def db_read(self, db_number: int, start: int, size: int) -> bytes: if self._connection.requires_substreamed: return self._db_read_substreamed(db_number, start, size) + # Legacy SessionKey PLCs interpret every value after AccessSubArea as + # another nested address ID. A raw range encoded as [offset, size] + # therefore addresses ``size`` below the scalar at ``offset`` and old + # S7-1500 firmware terminates the connection. Compatible clients send + # one absolute ID and let the returned PValue carry the value's size. + if self._connection.session_key_active: + access_area = Ids.DB_ACCESS_AREA_BASE + (db_number & 0xFFFF) + data = self.read_symbolic(access_area, [start + 1]) + return _fit_absolute_read(data, size, db_number, start) + payload = _build_read_payload([(db_number, start, size)], self._connection.protocol_version) response = self._connection.send_request(FunctionCode.GET_MULTI_VARIABLES, payload) results = _parse_read_response(response) @@ -298,6 +308,11 @@ def db_read_multi(self, items: list[tuple[int, int, int]]) -> list[bytes]: if self._connection.requires_substreamed: return [self._db_read_substreamed(db, start, size) for db, start, size in items] + if self._connection.session_key_active: + addresses = [(Ids.DB_ACCESS_AREA_BASE + (db_number & 0xFFFF), [start + 1]) for db_number, start, _size in items] + values = self.read_symbolic_multi(addresses) + return [_fit_absolute_read(value, size, db_number, start) for value, (db_number, start, size) in zip(values, items)] + payload = _build_read_payload(items, self._connection.protocol_version) response = self._connection.send_request(FunctionCode.GET_MULTI_VARIABLES, payload) parsed = _parse_read_response(response) @@ -868,6 +883,18 @@ def __exit__(self, *args: Any) -> None: _SCALAR_RESPONSE_SUFFIX = bytes.fromhex("000400000000") +def _fit_absolute_read(data: bytes | None, size: int, db_number: int, start: int) -> bytes: + """Fit one typed absolute-address value to the ``db_read`` byte contract.""" + if data is None: + raise RuntimeError(f"DB{db_number} offset {start} could not be read") + if len(data) < size: + raise RuntimeError( + f"DB{db_number} offset {start} returned {len(data)} bytes, fewer than the requested {size}; " + "use browse() and read_symbolic() when the range crosses variable boundaries" + ) + return data[:size] + + def _build_read_payload(items: list[tuple[int, int, int]], protocol_version: int = ProtocolVersion.V2) -> bytes: """Build a GetMultiVariables request payload. diff --git a/s7commplus/connection.py b/s7commplus/connection.py index d458c7ca..25591e6b 100644 --- a/s7commplus/connection.py +++ b/s7commplus/connection.py @@ -536,6 +536,11 @@ def session_setup_ok(self) -> bool: """Whether the session setup (ServerSessionVersion echo) succeeded.""" return self._session_setup_ok + @property + def session_key_active(self) -> bool: + """Whether legacy SessionKey authentication protects application traffic.""" + return self._session_key is not None + @property def requires_substreamed(self) -> bool: """Whether data operations must use substreamed function codes. diff --git a/s7commplus/server.py b/s7commplus/server.py index 6794c8e5..40b98527 100644 --- a/s7commplus/server.py +++ b/s7commplus/server.py @@ -945,6 +945,9 @@ def _handle_get_multi_variables(self, seq_num: int, session_id: int, request_dat for i, (db_num, byte_offset, byte_size) in enumerate(items, 1): db = self._data_blocks.get(db_num) if db is not None: + if byte_size == 0: + variable = next((var for var in db.variables.values() if var.byte_offset == byte_offset), None) + byte_size = variable.byte_size if variable is not None else 1 data = db.read(byte_offset, byte_size) response += encode_uint32_vlq(i) # ItemNumber response += encode_pvalue_blob(data) # Value as BLOB @@ -1288,9 +1291,11 @@ def _server_parse_read_request(request_data: bytes) -> list[tuple[int, int, int] # Extract db_number from AccessArea db_num = access_area & 0xFFFF - # Extract byte offset and size from LIDs (LID offsets are 1-based) + # A second ID is the historical emulator's raw-range extension. Real + # PLC absolute addresses contain one 1-based ID; the returned PValue + # determines the scalar's size. byte_offset = (lids[0] - 1) if len(lids) > 0 else 0 - byte_size = lids[1] if len(lids) > 1 else 1 + byte_size = lids[1] if len(lids) > 1 else 0 items.append((db_num, byte_offset, byte_size)) diff --git a/tests/test_s7_unit.py b/tests/test_s7_unit.py index e4673075..6b6e659d 100644 --- a/tests/test_s7_unit.py +++ b/tests/test_s7_unit.py @@ -615,6 +615,36 @@ def test_db_read_multi_not_connected(self) -> None: with pytest.raises(RuntimeError, match="Not connected"): client.db_read_multi([(1, 0, 4)]) + def test_session_key_db_read_uses_one_absolute_address_id(self) -> None: + client = S7CommPlusClient() + connection = MagicMock(requires_substreamed=False, session_key_active=True) + client._connection = connection + client.read_symbolic = MagicMock(return_value=b"\x12\x34") + + assert client.db_read(116, 708, 2) == b"\x12\x34" + client.read_symbolic.assert_called_once_with(Ids.DB_ACCESS_AREA_BASE + 116, [709]) + + def test_session_key_db_read_rejects_cross_variable_range(self) -> None: + client = S7CommPlusClient() + connection = MagicMock(requires_substreamed=False, session_key_active=True) + client._connection = connection + client.read_symbolic = MagicMock(return_value=b"\x12\x34") + + with pytest.raises(RuntimeError, match="crosses variable boundaries"): + client.db_read(116, 708, 4) + + def test_session_key_db_read_multi_preserves_item_count(self) -> None: + client = S7CommPlusClient() + connection = MagicMock(requires_substreamed=False, session_key_active=True) + client._connection = connection + client.read_symbolic_multi = MagicMock(return_value=[b"\x01\x02", None]) + + with pytest.raises(RuntimeError, match="DB2 offset 10 could not be read"): + client.db_read_multi([(1, 0, 2), (2, 10, 1)]) + client.read_symbolic_multi.assert_called_once_with( + [(Ids.DB_ACCESS_AREA_BASE + 1, [1]), (Ids.DB_ACCESS_AREA_BASE + 2, [11])] + ) + def test_db_write_multi_not_connected(self) -> None: client = S7CommPlusClient() with pytest.raises(RuntimeError, match="Not connected"):