Skip to content

Fix non-deterministic convert_from_registers mutating the caller's registers list#2951

Open
gaoflow wants to merge 1 commit into
pymodbus-dev:devfrom
gaoflow:fix-convert-from-registers-mutation
Open

Fix non-deterministic convert_from_registers mutating the caller's registers list#2951
gaoflow wants to merge 1 commit into
pymodbus-dev:devfrom
gaoflow:fix-convert-from-registers-mutation

Conversation

@gaoflow

@gaoflow gaoflow commented Jun 30, 2026

Copy link
Copy Markdown

The bug

ModbusClientMixin.convert_from_registers(..., word_order="little") reverses the caller's registers list in place for STRING/BITS, so the input is corrupted and a second identical call returns garbage — the decode is non-deterministic:

from pymodbus.client.mixin import ModbusClientMixin as M
DT = M.DATATYPE

regs = M.convert_to_registers("Hello", DT.STRING, word_order="little")  # [28416, 27756, 18533]
a = M.convert_from_registers(regs, DT.STRING, word_order="little")      # 'Hello'   (regs now reversed!)
b = M.convert_from_registers(regs, DT.STRING, word_order="little")      # 'o\x00llHe'
assert a == b   # AssertionError — same args, different result

The same happens for BITS once the value spans more than one register. The numeric types (INT*/UINT*/FLOAT*) are not affected — they already decode without mutating the input.

Why this is a bug

  • A pure decode helper must be side-effect free and deterministic: calling it twice with the same arguments must give the same result and must not alter the arguments. Here the first call silently rewrites the caller's buffer.
  • It's internally inconsistent: the numeric branch slices a copy (regs = registers[i:i+data_len]) before reversing, so it never touches the input; only the data_len == 0 (STRING/BITS) branch reverses the original.
  • The docstring documents no mutation of registers.

Root cause

pymodbus/client/mixin.py, in the data_len == 0 branch of convert_from_registers:

if word_order == "little":
    registers.reverse()        # in-place reverse of the caller's list

list.reverse() mutates in place. The fix reverses a copy instead, mirroring what the numeric branch below already does:

if word_order == "little":
    registers = list(reversed(registers))

Tests

Added test_client_mixin_convert_from_registers_no_mutation to test/client/test_client.py, parametrized over STRING / multi-register BITS / INT32 × big/little. It asserts the input list is unchanged after decoding and that two consecutive identical calls return equal results. The three little STRING/BITS cases fail on dev and pass with the fix; the rest (and the existing convert tests) stay green.

The existing test_client_mixin_convert didn't catch this because it builds a fresh reversed list and calls convert_from_registers only once per input.

The STRING/BITS branch of convert_from_registers reversed the registers
list in place when word_order="little", corrupting the caller's input so
a second identical call decoded garbage (non-deterministic output). The
numeric branch already avoids this by reversing a slice copy; reverse a
copy here too so decoding is side-effect free.
Comment thread pymodbus/client/mixin.py
byte_list = bytearray()
if word_order == "little":
registers.reverse()
# Reverse a copy; the numeric branch below also works on a

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No need for this comment, it does not provide more information than the the code

):
"""convert_from_registers must not mutate input and stay deterministic.

Regression: for STRING/BITS with word_order="little" the registers list

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We do not explain bugs in comments! doing that would make the comments larger than the code and with nothing more than historic value.

Seems to continue, please remove all comments that either only have historic value, or explain what the code does without adding value.

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