fix(s7commplus): correlate request responses - #883
Merged
Merged
Conversation
This was referenced Sep 11, 2026
Owner
Author
|
@bvanelli Could you review the S7CommPlus protocol/security changes as one batch? The independent roots are #882, this PR, #887, and #885; #884 and #888 form the stacked continuation of this PR. Your earlier protocol traces and authentication reviews are especially relevant to the response correlation, integrity-ID/HMAC validation, and session-key fallback/renewal paths. Reviewing the roots first is enough; we can update the stacked branches after any findings. |
bvanelli
approved these changes
Sep 14, 2026
bvanelli
left a comment
Contributor
There was a problem hiding this comment.
LGTM, i checked some behavioural properties:
- The
stale_responsesresets, so an otherwise healthy subscription cannot easility be killed. - Code repetition in
_incoming_frame_opcodeand_incoming_response_sequencegoes through different branches, not worth generalising. - Lock now guarantees ordering for
send_request, and there is no deadlock possibility due to the timeout.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Real-hardware evidence
The S7-1511C trace in #872 showed a DB-read request at sequence 7 consuming the delayed protection-level reply for sequence 6. The added regression covers that ordering for both sync and async clients. PR #881 is now stacked on this branch so the combined behavior can be tested on that PLC.
Validation
uv run --frozen pytest -q: 2087 passed, 82 skippeduv run --frozen pre-commit run --all-files: passeduv build --no-sources: passedFixes #833