Skip to content

Conversation

@starwarsivan
Copy link
Contributor

Summary

This PR adds validation for the Modbus TCP Protocol ID in FramerSocket
and ensures only compliant frames (Protocol ID = 0) are accepted.

Changes

  • Added a Protocol ID check in FramerSocket to reject non-zero values
  • Updated existing test vectors to use Protocol ID = 0, per the Modbus TCP specification
  • Added a test to verify that invalid (non-zero Protocol ID) messages produce empty results

Rationale

According to the Modbus TCP specification, the Protocol ID field must be
set to 0. Previously, non-zero values were accepted and some tests used
invalid protocol IDs, which could mask malformed input.

This change enforces spec compliance and improves robustness when handling
invalid Modbus TCP frames.

Testing

  • Updated unit tests pass
  • New test confirms invalid Protocol ID frames are rejected

Add protocol ID check in FramerSocket to reject non-zero values,
and include a test to verify that invalid messages produce empty results.
Updated two test vectors that previously used fixed, nonzero
protocol IDs. Now the vectors use Protocol ID = 0, in compliance
with the Modbus TCP specification
Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks.

Good catch, looking forward to see more PRs.

Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes are ok, but you have a pylint problem and a pytest problem that you need to solve.

Use ./check_ci.sh to run the tests locally or push to github.

Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@janiversen janiversen merged commit 30a2605 into pymodbus-dev:dev Dec 17, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants