Skip to content

fix(server): harden COTP connection framing - #893

Open
gijzelaerr wants to merge 2 commits into
masterfrom
fix/889-890-server-transport
Open

gijzelaerr wants to merge 2 commits into
masterfrom
fix/889-890-server-transport

Conversation

@gijzelaerr

Copy link
Copy Markdown
Owner

Summary

  • echo calling and called TSAP parameters in COTP Connection Confirm frames for Snap7-derived clients
  • start a fresh receive deadline after each complete TPKT header
  • treat payload timeouts as connection errors to prevent stream desynchronisation
  • add exact wire-format and deadline regression coverage

Verification

  • uv run pytest — 2076 passed, 82 skipped
  • uv run pre-commit run --all-files
  • uv build

Closes #889
Closes #890

@gijzelaerr

Copy link
Copy Markdown
Owner Author

@esternu Could you rerun the two pure-Python compatibility reproducers from #889 and #890 against commit c998860? This PR changes the COTP connection-confirmation framing and fragmented/coalesced request handling specifically around those failures. No physical PLC is needed; a pass or remaining traceback from both reproducers would close the validation loop.

@esternu

esternu commented Sep 14, 2026

Copy link
Copy Markdown

Thanks for the quick fix! I reran both against c998860 - both pass.

Setup: pure-Python server from the PR checkout, no PLC. Two clients: Telegraf 1.30.3 inputs.s7comm (gos7), i.e. the client from the original reports, and a small raw-socket script with deterministic timing (below). tests/test_server.py: 77 passed.

before (4984721) c998860
#889 Connection Confirm length 14 bytes 22 bytes
#889 Telegraf connects and reads ISO : Invalid PDU received reads values
#890 header just before / rest just after the deadline Invalid TPKT version: 2, connection reset valid response
#890 Telegraf polling every 5 s for 65 s 3 aborts, Invalid TPKT version: 2 (measured with only the #889 part applied, since plain master cannot connect) 13 reads, 0 errors

One edge case you may want to look at - not a blocker for #889/#890. A request split into two COTP fragments, with a gap longer than RECEIVE_DEADLINE before the second fragment's TPKT header arrives. With c998860 the header wait for the second fragment starts a fresh deadline and then raises a plain TimeoutError. The client handler treats that as idle (except socket.timeout: continue), so the already received first fragment is silently dropped, the connection stays open and the client never gets a response. That is the same behaviour as before the PR. Since the header of a subsequent fragment is by definition mid-request, raising S7ConnectionError there - as the PR already does for the payload read - would close the connection explicitly instead. Real clients rarely stall that long between fragments, so this is mostly about consistency with the "prevent stream desynchronisation" goal. A minimal variant:

        started = False
        while True:
            header_deadline = time.monotonic() + self.RECEIVE_DEADLINE
            try:
                tpkt_header = self._recv_exact(4, header_deadline)
            except TimeoutError as e:
                if started:
                    raise S7ConnectionError("Receive deadline exceeded between COTP fragments") from e
                raise
            started = True

About your comments on #889 and #890 asking for PRs: since #893 already covers both, I don't want to open duplicates. I do have fixes for both on my fork - esternu:fix/#889-cotp-cc-tsap-echo and esternu:fix/#890-receive-deadline-idle - and the #890 one covers the fragment case above, with regression tests for all three timeout situations (request arriving as the idle wait runs out, timeout after the header, idle timeout without any byte). Happy to open them, or to contribute just the tests to #893, whichever you prefer.

Co-authored-by: Codex <noreply@openai.com>
@gijzelaerr

Copy link
Copy Markdown
Owner Author

Thanks for the detailed validation and reproducer. I added the between-fragments timeout handling and a regression test in 363374c, and merged current master into this branch. The PR is conflict-free and all checks are green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants