pymodbus-dev__pymodbus-1923

The user is reporting an issue where `pymodbus` fails to handle reassembled TCP segments.
Specifically, a Modbus response split over two TCP frames causes a failure.
The first frame is incomplete and fails the frame check.
The second frame is then processed as a separate response, which also fails (index out of range or unable to decode).

From the logs:
- `recv: 0x0 0x1 0x0 0x0 0x0 0xb 0x1 0x3 0x8 0x0 0xb5 0x12 0x2f 0x37 0x21 0x0 old_data:  addr=None`
- `Processing: 0x0 0x1 0x0 0x0 0x0 0xb 0x1 0x3 0x8 0x0 0xb5 0x12 0x2f 0x37 0x21 0x0`
- `Frame check failed, ignoring!!`
- `Resetting frame - Current Frame in buffer - 0x0 0x1 0x0 0x0 0x0 0xb 0x1 0x3 0x8 0x0 0xb5 0x12 0x2f 0x37 0x21 0x0`
- `recv: 0x3 old_data:  addr=None`
- `Processing: 0x3`
- `Factory Response[ReadHoldingRegistersResponse': 3]`
- `ERROR logging:115 General exception: index out of range`
- `pymodbus.exceptions.ModbusIOException: Modbus Error: [Input/Output] Unable to decode request`

The Modbus TCP frame format is:
- Transaction Identifier: 2 bytes
- Protocol Identifier: 2 bytes (0 for Modbus)
- length: 2 bytes (number of following bytes)
- Unit Identifier: 1 byte
- PDU: variable

In the log:
`0x0 0x1` (Transaction ID: 1)
`0x0 0x0` (Protocol ID: 0)
`0x0 0xb` (Length: 11)
`0x1` (Unit ID: 1)
The remaining bytes in the first frame are `0x3 0x8 0x0 0xb5 0x12 0x2f 0x37 0x21 0x0`.
That's 9 bytes.
Total bytes after length field: 1 + 9 = 10 bytes.
But the length field said 11 bytes.
So the first frame is missing 1 byte.
Then the second frame arrives: `0x3`.
Total bytes = 10 + 1 = 11.
The framer should have buffered the first frame and waited for the last byte before trying to decode.

The logs point to:
- `pymodbus/transport/transport.py`, `data_received` -> `datagram_received`
- `pymodbus/client/base.py`, `callback_data`
- `pymodbus/framer/base.py`, `processIncomingPacket`
- `pymodbus/framer/socket_framer.py`, `frameProcessIncomingPacket` and `_process`.

I need to investigate how `socket_framer.py` handles incoming packets and buffering.

Phase 1: Pinpoint Code & Tests.

{
  "command": "grep -rn --include=\"*.py\" \"processIncomingPacket\" pymodbus/framer/"
}

{
  "status": "ok",
  "stdout": "pymodbus/framer/base.py:115:    def processIncomingPacket(self, data, callback, slave, **kwargs):\n",
  "stderr": "",
  "exit_code": 0
}

Wait, `grep` only returned `base.py`. But the stack trace mentioned `socket_framer.py`. Let me check `socket_framer.py` specifically.

{
  "command": "grep -rn \"frameProcessIncomingPacket\" pymodbus/framer/socket_framer.py"
}

{
  "status": "ok",
  "stdout": "119:    def frameProcessIncomingPacket(self, single, callback, slave, tid=None, **kwargs):\n",
  "stderr": "",
  "exit_code": 0
}

Okay, let's see how `socket_framer.py` is implemented.
I'll read `pymodbus/framer/base.py` and `pymodbus/framer/socket_framer.py`.

{
  "filepath": "pymodbus/framer/base.py",
  "start_line": 100
}

{
  "status": "ok",
  "filepath": "pymodbus/framer/base.py",
  "content": "            \"tid\": 0,\n        }\n\n    def populateResult(self, result):\n        \"\"\"Populate the modbus result header.\n\n        The serial packets do not have any header information\n        that is copied.\n\n        :param result: The response packet\n        \"\"\"\n        result.slave_id = self._header.get(\"uid\", 0)\n        result.transaction_id = self._header.get(\"tid\", 0)\n        result.protocol_id = self._header.get(\"pid\", 0)\n\n    def processIncomingPacket(self, data, callback, slave, **kwargs):\n        \"\"\"Process new packet pattern.\n\n        This takes in a new request packet, adds it to the current\n        packet stream, and performs framing on it. That is, checks\n        for complete messages, and once found, will process all that\n        exist.  This handles the case when we read N + 1 or 1 // N\n        messages at a time instead of 1.\n\n        The processed and decoded messages are pushed to the callback\n        function to process and send.\n\n        :param data: The new packet data\n        :param callback: The function to send results to\n        :param slave: Process if slave id matches, ignore otherwise (could be a\n               list of slave ids (server) or single slave id(client/server))\n        :param kwargs:\n        :raises ModbusIOException:\n        \"\"\"\n        Log.debug(\"Processing: {}\", data, \":hex\")\n        self._buffer += data\n        if not isinstance(slave, (list, tuple)):\n            slave = [slave]\n        single = kwargs.pop(\"single\", False)\n        self.frameProcessIncomingPacket(single, callback, slave, **kwargs)\n\n    def frameProcessIncomingPacket(\n        self, _single, _callback, _slave, _tid=None, **kwargs\n    ):\n        \"\"\"Process new packet pattern.\"\"\"",
  "start_line": 100,
  "end_line": 144,
  "total_lines": 144,
  "is_truncated": false
}

Now let's read `pymodbus/framer/socket_framer.py`.

{
  "filepath": "pymodbus/framer/socket_framer.py"
}

{
  "status": "ok",
  "filepath": "pymodbus/framer/socket_framer.py",
  "content": "\"\"\"Socket framer.\"\"\"\n# pylint: disable=missing-type-doc\nimport struct\n\nfrom pymodbus.exceptions import (\n    InvalidMessageReceivedException,\n    ModbusIOException,\n)\nfrom pymodbus.framer.base import SOCKET_FRAME_HEADER, ModbusFramer\nfrom pymodbus.logging import Log\n\n\n# --------------------------------------------------------------------------- #\n# Modbus TCP Message\n# --------------------------------------------------------------------------- #\n\n\nclass ModbusSocketFramer(ModbusFramer):\n    \"\"\"Modbus Socket Frame controller.\n\n    Before each modbus TCP message is an MBAP header which is used as a\n    message frame.  It allows us to easily separate messages as follows::\n\n        [         MBAP Header         ] [ Function Code] [ Data ] \\\n        [ tid ][ pid ][ length ][ uid ]\n          2b     2b     2b        1b           1b           Nb\n\n        while len(message) > 0:\n            tid, pid, length`, uid = struct.unpack(\">HHHB\", message)\n            request = message[0:7 + length - 1`]\n            message = [7 + length - 1:]\n\n        * length = uid + function code + data\n        * The -1 is to account for the uid byte\n    \"\"\"\n\n    method = \"socket\"\n\n    def __init__(self, decoder, client=None):\n        \"\"\"Initialize a new instance of the framer.\n\n        :param decoder: The decoder factory implementation to use\n        \"\"\"\n        super().__init__(decoder, client)\n        self._hsize = 0x07\n\n    # ----------------------------------------------------------------------- #\n    # Private Helper Functions\n    # ----------------------------------------------------------------------- #\n    def checkFrame(self):\n        \"\"\"Check and decode the next frame.\n\n        Return true if we were successful.\n        \"\"\"\n        if self.isFrameReady():\n            (\n                self._header[\"tid\"],\n                self._header[\"pid\"],\n                self._header[\"len\"],\n                self._header[\"uid\"],\n            ) = struct.unpack(\">HHHB\", self._buffer[0 : self._hsize])\n\n            # someone sent us an error? ignore it\n            if self._header[\"len\"] < 2:\n                self.advanceFrame()\n            # we have at least a complete message, continue\n            elif len(self._buffer) - self._hsize + 1 >= self._header[\"len\"]:\n                return True\n        # we don't have enough of a message yet, wait\n        return False\n\n    def advanceFrame(self):\n        \"\"\"Skip over the current framed message.\n\n        This allows us to skip over the current message after we have processed\n        it or determined that it contains an error. It also has to reset the\n        current frame header handle\n        \"\"\"\n        length = self._hsize + self._header[\"len\"] - 1\n        self._buffer = self._buffer[length:]\n        self._header = {\"tid\": 0, \"pid\": 0, \"len\": 0, \"uid\": 0}\n\n    def isFrameReady(self):\n        \"\"\"Check if we should continue decode logic.\n\n        This is meant to be used in a while loop in the decoding phase to let\n        the decoder factory know that there is still data in the buffer.\n\n        :returns: True if ready, False otherwise\n        \"\"\"\n        return len(self._buffer) > self._hsize\n\n    def getFrame(self):\n        \"\"\"Return the next frame from the buffered data.\n\n        :returns: The next full frame buffer\n        \"\"\"\n        length = self._hsize + self._header[\"len\"] - 1\n        return self._buffer[self._hsize : length]\n\n    # ----------------------------------------------------------------------- #\n    # Public Member Functions\n    # ----------------------------------------------------------------------- #\n    def decode_data(self, data):\n        \"\"\"Decode data.\"\"\"\n        if len(data) > self._hsize:\n            tid, pid, length, uid, fcode = struct.unpack(\n                SOCKET_FRAME_HEADER, data[0 : self._hsize + 1]\n            )\n            return {\n                \"tid\": tid,\n                \"pid\": pid,\n                \"length\": length,\n                \"slave\": uid,\n                \"fcode\": fcode,\n            }\n        return {}\n\n    def frameProcessIncomingPacket(self, single, callback, slave, tid=None, **kwargs):\n        \"\"\"Process new packet pattern.\n\n        This takes in a new request packet, adds it to the current\n        packet stream, and performs framing on it. That is, checks\n        for complete messages, and once found, will process all that\n        exist.  This handles the case when we read N + 1 or 1 // N\n        messages at a time instead of 1.\n\n        The processed and decoded messages are pushed to the callback\n        function to process and send.\n        \"\"\"\n        while True:\n            if not self.isFrameReady():\n                if len(self._buffer):\n                    # Possible error ???\n                    if self._header[\"len\"] < 2:\n                        self._process(callback, tid, error=True)\n                break\n            if not self.checkFrame():\n                Log.debug(\"Frame check failed, ignoring!!\")\n                self.resetFrame()\n                continue\n            if not self._validate_slave_id(slave, single):\n                header_txt = self._header[\"uid\"]\n                Log.debug(\"Not a valid slave id - {}, ignoring!!\", header_txt)\n                self.resetFrame()\n                continue\n            self._process(callback, tid)\n\n    def _process(self, callback, tid, error=False):\n        \"\"\"Process incoming packets irrespective error condition.\"\"\"",
  "start_line": 1,
  "end_line": 150,
  "total_lines": 182,
  "is_truncated": true
}

I need to read the rest of `pymodbus/framer/socket_framer.py`, especially `_process` and `resetFrame`.

{
  "filepath": "pymodbus/framer/socket_framer.py",
  "start_line": 151
}

{
  "status": "ok",
  "filepath": "pymodbus/framer/socket_framer.py",
  "content": "        data = self._buffer if error else self.getFrame()\n        if (result := self.decoder.decode(data)) is None:\n            self.resetFrame()\n            raise ModbusIOException(\"Unable to decode request\")\n        if error and result.function_code < 0x80:\n            raise InvalidMessageReceivedException(result)\n        self.populateResult(result)\n        self.advanceFrame()\n        if tid and tid != result.transaction_id:\n            self.resetFrame()\n        else:\n            callback(result)  # defer or push to a thread?\n\n    def buildPacket(self, message):\n        \"\"\"Create a ready to send modbus packet.\n\n        :param message: The populated request/response to send\n        \"\"\"\n        data = message.encode()\n        packet = struct.pack(\n            SOCKET_FRAME_HEADER,\n            message.transaction_id,\n            message.protocol_id,\n            len(data) + 2,\n            message.slave_id,\n            message.function_code,\n        )\n        packet += data\n        return packet\n\n\n# __END__",
  "start_line": 151,
  "end_line": 182,
  "total_lines": 182,
  "is_truncated": false
}

Wait, I missed `resetFrame`. Let me find it. It's not in the parts I read. Looking at `frameProcessIncomingPacket`:

```python
    def frameProcessIncomingPacket(self, single, callback, slave, tid=None, **kwargs):
        """Process new packet pattern.
        ...
        """
        while True:
            if not self.isFrameReady():
                if len(self._buffer):
                    # Possible error ???
                    if self._header["len"] < 2:
                        self._process(callback, tid, error=True)
                break
            if not self.checkFrame():
                Log.debug("Frame check failed, ignoring!!")
                self.resetFrame()
                continue
            if not self._validate_slave_id(slave, single):
                header_txt = self._header["uid"]
                Log.debug("Not a valid slave id - {}, ignoring!!", header_txt)
                self.resetFrame()
                continue
            self._process(callback, tid)
```

And `checkFrame`:

```python
    def checkFrame(self):
        """Check and decode the next frame.

        Return true if we were successful.
        """
        if self.isFrameReady():
            (
                self._header["tid"],
                self._header["pid"],
                self._header["len"],
                self._header["uid"],
            ) = struct.unpack(">HHHB", self._buffer[0 : self._hsize])

            # someone sent us an error? ignore it
            if self._header["len"] < 2:
                self.advanceFrame()
            # we have at least a complete message, continue
            elif len(self._buffer) - self._hsize + 1 >= self._header["len"]:
                return True
        # we don't have enough of a message yet, wait
        return False
```

And `isFrameReady`:

```python
    def isFrameReady(self):
        """Check if we should continue decode logic.
        ...
        """
        return len(self._buffer) > self._hsize
```

Wait, if `len(self._buffer) <= self._hsize` (7 bytes), `isFrameReady` returns `False`.
If `len(self._buffer) > self._hsize`, `checkFrame` is called.
Inside `checkFrame`:
1. Unpack header (7 bytes).
2. If `len < 2`, `advanceFrame` and return `False`.
3. If `len(self._buffer) - 7 + 1 >= len`, return `True`.
   Wait, `len(self._buffer) - self._hsize + 1`... let's re-examine.
   `self._hsize` is 7.
   `len(self._buffer)` is total bytes.
   Modbus TCP frame: `TID (2) + PID (2) + Len (2) + UID (1) + PDU (Len - 1)`.
   Total size = `7 + (Len - 1) = 6 + Len`.
   So if `len(self._buffer) >= 6 + Len`, the frame is ready.
   `len(self._buffer) - 7 + 1 >= Len` is `len(self._buffer) - 6 >= Len`, which is `len(self._buffer) >= 6 + Len`. Correct.

However, if `isFrameReady()` is `True` but `checkFrame()` returns `False` (meaning we have at least 8 bytes, but not enough to complete the frame based on the length in the header), it goes:

```python
            if not self.checkFrame():
                Log.debug("Frame check failed, ignoring!!")
                self.resetFrame()
                continue
```

AHA! `resetFrame()` is called when the frame is not yet complete!
This is exactly the bug. If we have some bytes of a frame but not all, `checkFrame()` returns `False`, and instead of waiting for more data, it calls `resetFrame()` and continues the loop.

Wait, I still don't see `resetFrame()` in the file. Let me search for it. It might be in `base.py`.

{
  "command": "grep -rn \"def resetFrame\" pymodbus/framer/"
}

{
  "status": "ok",
  "stdout": "pymodbus/framer/base.py:81:    def resetFrame(self):\npymodbus/framer/rtu_framer.py:107:    def resetFrame(self):\n",
  "stderr": "",
  "exit_code": 0
}

Let's look at `resetFrame` in `base.py`.

{
  "filepath": "pymodbus/framer/base.py"
}

{
  "status": "ok",
  "filepath": "pymodbus/framer/base.py",
  "content": "\"\"\"Framer start.\"\"\"\n# pylint: disable=missing-type-doc\nfrom __future__ import annotations\n\nfrom typing import Any\n\nfrom pymodbus.factory import ClientDecoder, ServerDecoder\nfrom pymodbus.logging import Log\n\n\n# Unit ID, Function Code\nBYTE_ORDER = \">\"\nFRAME_HEADER = \"BB\"\n\n# Transaction Id, Protocol ID, Length, Unit ID, Function Code\nSOCKET_FRAME_HEADER = BYTE_ORDER + \"HHH\" + FRAME_HEADER\n\n# Function Code\nTLS_FRAME_HEADER = BYTE_ORDER + \"B\"\n\n\nclass ModbusFramer:\n    \"\"\"Base Framer class.\"\"\"\n\n    name = \"\"\n\n    def __init__(\n        self,\n        decoder: ClientDecoder | ServerDecoder,\n        client=None,\n    ) -> None:\n        \"\"\"Initialize a new instance of the framer.\n\n        :param decoder: The decoder implementation to use\n        \"\"\"\n        self.decoder = decoder\n        self.client = client\n        self._header: dict[str, Any] = {\n            \"lrc\": \"0000\",\n            \"len\": 0,\n            \"uid\": 0x00,\n            \"tid\": 0,\n            \"pid\": 0,\n            \"crc\": b\"\\x00\\x00\",\n        }\n        self._buffer = b\"\"\n\n    def _validate_slave_id(self, slaves: list, single: bool) -> bool:\n        \"\"\"Validate if the received data is valid for the client.\n\n        :param slaves: list of slave id for which the transaction is valid\n        :param single: Set to true to treat this as a single context\n        :return:\n        \"\"\"\n        if single:\n            return True\n        if 0 in slaves or 0xFF in slaves:\n            # Handle Modbus TCP slave identifier (0x00 0r 0xFF)\n            # in asynchronous requests\n            return True\n        return self._header[\"uid\"] in slaves\n\n    def sendPacket(self, message):\n        \"\"\"Send packets on the bus.\n\n        With 3.5char delay between frames\n        :param message: Message to be sent over the bus\n        :return:\n        \"\"\"\n        return self.client.send(message)\n\n    def recvPacket(self, size):\n        \"\"\"Receive packet from the bus.\n\n        With specified len\n        :param size: Number of bytes to read\n        :return:\n        \"\"\"\n        return self.client.recv(size)\n\n    def resetFrame(self):\n        \"\"\"Reset the entire message frame.\n\n        This allows us to skip ovver errors that may be in the stream.\n        It is hard to know if we are simply out of sync or if there is\n        an error in the stream as we have no way to check the start or\n        end of the message (python just doesn't have the resolution to\n        check for millisecond delays).\n        \"\"\"\n        Log.debug(\n            \"Resetting frame - Current Frame in buffer - {}\", self._buffer, \":hex\"\n        )\n        self._buffer = b\"\"\n        self._header = {\n            \"lrc\": \"0000\",\n            \"crc\": b\"\\x00\\x00\",\n            \"len\": 0,\n            \"uid\": 0x00,\n            \"pid\": 0,\n            \"tid\": 0,\n        }\n\n    def populateResult(self, result):\n        \"\"\"Populate the modbus result header.\n\n        The serial packets do not have any header information\n        that is copied.\n\n        :param result: The response packet\n        \"\"\"\n        result.slave_id = self._header.get(\"uid\", 0)\n        result.transaction_id = self._header.get(\"tid\", 0)\n        result.protocol_id = self._header.get(\"pid\", 0)\n\n    def processIncomingPacket(self, data, callback, slave, **kwargs):\n        \"\"\"Process new packet pattern.\n\n        This takes in a new request packet, adds it to the current\n        packet stream, and performs framing on it. That is, checks\n        for complete messages, and once found, will process all that\n        exist.  This handles the case when we read N + 1 or 1 // N\n        messages at a time instead of 1.\n\n        The processed and decoded messages are pushed to the callback\n        function to process and send.\n\n        :param data: The new packet data\n        :param callback: The function to send results to\n        :param slave: Process if slave id matches, ignore otherwise (could be a\n               list of slave ids (server) or single slave id(client/server))\n        :param kwargs:\n        :raises ModbusIOException:\n        \"\"\"\n        Log.debug(\"Processing: {}\", data, \":hex\")\n        self._buffer += data\n        if not isinstance(slave, (list, tuple)):\n            slave = [slave]\n        single = kwargs.pop(\"single\", False)\n        self.frameProcessIncomingPacket(single, callback, slave, **kwargs)\n\n    def frameProcessIncomingPacket(\n        self, _single, _callback, _slave, _tid=None, **kwargs\n    ):\n        \"\"\"Process new packet pattern.\"\"\"",
  "start_line": 1,
  "end_line": 144,
  "total_lines": 144,
  "is_truncated": false
}

`resetFrame` clears the `_buffer`.

```python
    def resetFrame(self):
        """Reset the entire message frame.
        ...
        """
        Log.debug(
            "Resetting frame - Current Frame in buffer - {}", self._buffer, ":hex"
        )
        self._buffer = b""
        self._header = {
            "lrc": "0000",
            "crc": b"\x00\x00",
            "len": 0,
            "uid": 0x00,
            "pid": 0,
            "tid": 0,
        }
```

Now let's look at `frameProcessIncomingPacket` in `socket_framer.py` again.

```python
    def frameProcessIncomingPacket(self, single, callback, slave, tid=None, **kwargs):
        """Process new packet pattern.
        ...
        """
        while True:
            if not self.isFrameReady():
                if len(self._buffer):
                    # Possible error ???
                    if self._header["len"] < 2:
                        self._process(callback, tid, error=True)
                break
            if not self.checkFrame():
                Log.debug("Frame check failed, ignoring!!")
                self.resetFrame()
                continue
            # ...
```

If `checkFrame()` returns `False`, it means the frame is not yet ready. But `frameProcessIncomingPacket` assumes that if `isFrameReady()` is `True` (meaning we have more than `_hsize` bytes), and `checkFrame()` is `False`, it's a frame check failure and resets the buffer.

But `checkFrame()` returns `False` in two cases:
1. `isFrameReady()` is `False` (already handled by `if not self.isFrameReady(): break`).
2. `len(self._buffer) - self._hsize + 1 < self._header["len"]`.

Wait, if `len(self._buffer) > self._hsize`, we call `checkFrame()`.
`checkFrame()` un-packs the header. Then it checks if the buffer is long enough for the declared length. If it's NOT long enough, it returns `False`.
Then `frameProcessIncomingPacket` calls `resetFrame()`. This is clearly wrong for TCP segments where the frame can arrive in pieces.

The fix should be that if `checkFrame()` returns `False` because the frame is incomplete, it should `break` the loop and wait for more data, instead of resetting the frame.

Wait, let's look at `checkFrame()` again.

```python
    def checkFrame(self):
        """Check and decode the next frame.

        Return true if we were successful.
        """
        if self.isFrameReady():
            (
                self._header["tid"],
                self._header["pid"],
                self._header["len"],
                self._header["uid"],
            ) = struct.unpack(">HHHB", self._buffer[0 : self._hsize])

            # someone sent us an error? ignore it
            if self._header["len"] < 2:
                self.advanceFrame()
            # we have at least a complete message, continue
            elif len(self._buffer) - self._hsize + 1 >= self._header["len"]:
                return True
        # we don't have enough of a message yet, wait
        return False
```

If `isFrameReady()` is `True`, `checkFrame` reads the header.
If `len < 2`, it calls `advanceFrame()` (which consumes some bytes) and returns `False`.
If `len >= 2` and we have enough bytes, it returns `True`.
If `len >= 2` and we DON'T have enough bytes, it returns `False`.

In `frameProcessIncomingPacket`:
```python
            if not self.checkFrame():
                Log.debug("Frame check failed, ignoring!!")
                self.resetFrame()
                continue
```

If `checkFrame` returns `False` because we are waiting for more bytes, we are resetting the buffer!

We need a way to distinguish between "frame is incomplete" and "frame is invalid".
But `checkFrame` doesn't distinguish.

Actually, if `isFrameReady()` is `True` and `checkFrame()` is `False`, it could mean:
1. The header was read, but we need more bytes for the PDU.
2. The header was read, it was invalid (`len < 2`), and `advanceFrame()` was called.

Wait, if `len < 2`, `advanceFrame()` is called. `advanceFrame()` consumes `_hsize + len - 1` bytes.
Then `checkFrame()` returns `False`.
Then `frameProcessIncomingPacket` calls `resetFrame()`. This clears everything.

The real problem is when `len >= 2` but we don't have enough bytes.

If `checkFrame()` returns `False`, we should only `resetFrame()` if we actually have a reason to believe the frame is invalid, not just incomplete.

But wait, if `checkFrame` returns `False`, we don't know if it's because we need more bytes or because `advanceFrame` was called.

Actually, look at this:
```python
    def isFrameReady(self):
        """Check if we should continue decode logic.
        ...
        """
        return len(self._buffer) > self._hsize
```

If `len(self._buffer) <= 7`, it returns `False`.

In `frameProcessIncomingPacket`:
```python
        while True:
            if not self.isFrameReady():
                if len(self._buffer):
                    # Possible error ???
                    if self._header["len"] < 2:
                        self._process(callback, tid, error=True)
                break
```

If we have 0-7 bytes, we break. This is fine.

If we have > 7 bytes, we call `checkFrame()`.
If `checkFrame()` returns `False`, it means we either:
a) had `len < 2` (so we skipped some bytes), or
b) had `len >= 2` but we need more bytes.

In case (b), we MUST NOT call `resetFrame()`. We should just `break` and wait for more data.

But if we just `break`, what happens to case (a)?
In case (a), we already called `advanceFrame()`, which consumed the bad header. We can continue the loop to see if there's another frame in the buffer.

So, if `checkFrame()` returns `False`:
- Was it because of `len < 2`? If so, we already advanced the buffer, and we should continue to see if there are more frames.
- Was it because we need more bytes? If so, we should break.

How can we tell?
`checkFrame` could return a tri-state or we can check the buffer length before and after.

Wait, if `len < 2`, `advanceFrame()` is called, which modifies `self._buffer`.
If we are in the "need more bytes" case, `self._buffer` is not modified.

But the simplest fix is:
If `checkFrame()` returns `False`, and the buffer hasn't changed, it means we are waiting for more bytes. We should `break`.
If the buffer HAS changed, it means `advanceFrame()` was called, so we can `continue`.

Wait, but `checkFrame` is calling `struct.unpack`, which might throw `struct.error` if it's not enough bytes? No, `isFrameReady()` ensures we have at least `_hsize` bytes.

Let's look at `checkFrame` again.

```python
    def checkFrame(self):
        if self.isFrameReady():
            (
                self._header["tid"],
                self._header["pid"],
                self._header["len"],
                self._header["uid"],
            ) = struct.unpack(">HHHB", self._buffer[0 : self._hsize])

            # someone sent us an error? ignore it
            if self._header["len"] < 2:
                self.advanceFrame()
            # we have at least a complete message, continue
            elif len(self._buffer) - self._hsize + 1 >= self._header["len"]:
                return True
        # we don't have enough of a message yet, wait
        return False
```

Wait, there's a bug in `checkFrame` if `isFrameReady()` is `False`. It returns `False`.
But `frameProcessIncomingPacket` only calls `checkFrame` if `isFrameReady()` is `True`.

So if `checkFrame` returns `False`, it's either `len < 2` (advanceFrame called) or `not enough bytes`.

If we change `frameProcessIncomingPacket` to:

```python
            if not self.checkFrame():
                # If we have a header but not enough bytes for the full frame, 
                # we should stop and wait for more data.
                # If advanceFrame was called (len < 2), the buffer size changed.
                # But we can't easily know that here without checking buffer size.
                
                # Let's think. If checkFrame returns False, and we didn't 
                # advance the buffer, we're just waiting.
                # If we DID advance the buffer, we should check again.
                
                # Actually, just breaking is safer for the "need more bytes" case.
                # For the "len < 2" case, it will be handled when more data arrives.
                # But wait, if we have multiple frames and one is len < 2, 
                # we want to get to the next one immediately.
                
                # If we just 'break', we lose that optimization.
                # But the 'resetFrame' is definitely wrong.
```

Actually, if `checkFrame` returns `False` and we just `break`, we are not clearing the buffer. When more data arrives, `processIncomingPacket` is called again, `_buffer` is appended to, and we try `checkFrame` again. This is correct for TCP.

Is there any case where `checkFrame` returns `False` and we MUST `resetFrame`?
The original code:
```python
            if not self.checkFrame():
                Log.debug("Frame check failed, ignoring!!")
                self.resetFrame()
                continue
```
It seems the author thought that if `isFrameReady` is `True` (we have > 7 bytes) but `checkFrame` is `False`, it MUST be an invalid frame. But that's completely wrong for TCP segments! You can have 8 bytes, but the header says you need 100 bytes. That's not an invalid frame, it's just an incomplete one.

So, removing `resetFrame()` and calling `break` should fix it.

Wait, let's check if we need to handle the `len < 2` case specially.
If `len < 2`, `advanceFrame()` is called. This consumes the bad header.
Then `checkFrame()` returns `False`.
If we `break`, we wait for more data.
When more data arrives, we try again.
If there were multiple frames in the buffer, and the first one was `len < 2`, we skip it and then we might find the next frame.
But if we `break` immediately, we only process one `len < 2` frame per `data_received` call. That's probably acceptable, but not ideal.

Actually, if we want to be efficient:
```python
            if not self.checkFrame():
                # we might have advanced the frame (len < 2), or just waiting for more data.
                # if we have more than _hsize bytes, we could still have more frames.
                # But we can only know if we advanced if we check buffer length.
                
                # Let's see:
                # current_buffer_len = len(self._buffer)
                # if not self.checkFrame():
                #     if len(self._buffer) == current_buffer_len:
                #         break # waiting for more data
                #     else:
                #         continue # advanced the buffer, try again
```

But let's look at `checkFrame` again.
It returns `False` if `len(self._buffer) - self._hsize + 1 < self._header["len"]`.
If `len < 2`, `advanceFrame()` is called, and it returns `False`.

If we just `break`, we are correct for TCP.

Is there anything else?
The logs said:
`2024-01-04 11:16:16,635 DEBUG logging:103 Frame check failed, ignoring!!`
`2024-01-04 11:16:16,635 DEBUG logging:103 Resetting frame - Current Frame in buffer - 0x0 0x1 0x0 0x0 0x0 0xb 0x1 0x3 0x8 0x0 0xb5 0x12 0x2f 0x37 0x21 0x0`
`2024-01-04 11:16:16,719 DEBUG logging:103 recv: 0x3 old_data:  addr=None`
`2024-01-04 11:16:16,719 DEBUG logging:103 Processing: 0x3`
`2024-01-04 11:16:16,719 DEBUG logging:103 Factory Response[ReadHoldingRegistersResponse': 3]`
`2024-01-04 11:16:16,719 ERROR logging:115 General exception: index out of range`

The second call to `processIncomingPacket` had `data = 0x3`.
`_buffer` was empty because of `resetFrame()`.
Then `_buffer` became `0x3`.
`isFrameReady()` returned `False` (since `len(0x3) <= 7`).
Then the `while True` loop:
```python
        while True:
            if not self.isFrameReady():
                if len(self._buffer):
                    # Possible error ???
                    if self._header["len"] < 2:
                        self._process(callback, tid, error=True)
                break
```
Wait! `self._header` still has the value from the PREVIOUS frame because `resetFrame` is supposed to clear it. But wait, if `resetFrame` was called, `self._header["len"]` should be 0.
If `self._header["len"]` is 0, then `0 < 2` is `True`.
Then `self._process(callback, tid, error=True)` is called!
And inside `_process(callback, tid, error=True)`:
```python
    def _process(self, callback, tid, error=False):
        data = self._buffer if error else self.getFrame()
        if (result := self.decoder.decode(data)) is None:
            self.resetFrame()
            raise ModbusIOException("Unable todecode request")
```
`data` is `0x3`. `self.decoder.decode(b'\x03')` is called.
If the decoder for ReadHoldingRegistersResponse tries to decode a single byte `0x03`, it probably fails with "index out of range".
And that's exactly what happened in the logs: `ERROR logging:115 General exception: index out of range`.

So the combination of `resetFrame()` and the error handling in `frameProcessIncomingPacket` causes the crash.

The fix is:
1. Do NOT call `resetFrame()` when `checkFrame()` returns `False`. Just `break`.
2. Also, review the "Possible error ???" block.

```python
        while True:
            if not self.isFrameReady():
                if len(self._buffer):
                    # Possible error ???
                    if self._header["len"] < 2:
                        self._process(callback, tid, error=True)
                break
```

If `isFrameReady()` is `False`, it means we have 0-7 bytes.
If we have 1-7 bytes, and `self._header["len"] < 2` (which is true if we just started or just reset), it calls `_process(error=True)`.
This is very suspicious. If we only have 3 bytes, why would we try to decode it as an error frame?

Actually, the Modbus TCP frame must have at least 7 bytes for the header.
If we have less than 7 bytes, we can't even know what the `len` is.
But `self._header` is a state of the framer.

If we just started, `self._header["len"]` is 0.
So if any data arrives (1-7 bytes), `isFrameReady()` is `False`, `len(_buffer)` is > 0, `_header["len"]` is 0, and it calls `_process(error=True)`.
This seems like it would happen on every single connection if the first packet is small!

Wait, maybe `self._header` is only populated after the first successful `isFrameReady`?
No, it's initialized in `__init__` or `resetFrame`.

Let's check if there's a test that reproduces this.
The problem statement says they use `AsyncModbusTcpClient`.

I'll create a reproduction script.
I need a mock server that sends the response in two segments.

{
  "content": "\nimport asyncio\nimport pytest\nfrom pymodbus.client import AsyncModbusTcpClient\nfrom pymodbus.framer import ModbusSocketFramer\nfrom pymodbus.factory import ServerDecoder\nfrom pymodbus.pdu import ReadHoldingRegistersResponse\n\nasync def test_tcp_reassembly():\n    # Mock decoder\n    decoder = ServerDecoder()\n    # we want to simulate receiving a frame in two parts\n    # Frame: TID(2) PID(2) LEN(2) UID(1) PDU(...)\n    # TID=1, PID=0, LEN=3, UID=1, PDU=0x03 (ReadHoldingRegistersResponse, 1 byte for length)\n    # Length of PDU is 3 bytes: [function code(1), data length(1), data(N)]\n    # For ReadHoldingRegistersResponse, PDU is: [0x03, byte_count(1), registers(N)]\n    # Total frame: 00 01 00 00 00 03 01 03 02 00 00\n    # Wait, LEN is the number of bytes following the LEN field.\n    # UID(1) + PDU(N) = 1 + 3 = 4 bytes.\n    # So LEN should be 4.\n    # Total frame: 00 01 00 00 00 04 01 03 02 00 00\n    \n    frame = b'\\x00\\x01\\x00\\x00\\x00\\x04\\x01\\x03\\x02\\x00\\x00'\n    part1 = frame[:-1] # Missing last byte\n    part2 = frame[-1:] # The last byte\n    \n    framer = ModbusSocketFramer(decoder)\n    \n    # We need a callback\n    results = []\n    def callback(result):\n        results.append(result)\n        \n    # Process first part\n    framer.processIncomingPacket(part1, callback, slave=[1])\n    print(f\"After part 1, buffer: {framer._buffer.hex()}\")\n    assert len(results) == 0, \"Should not have produced result yet\"\n    \n    # Process second part\n    framer.processIncomingPacket(part2, callback, slave=[1])\n    print(f\"After part 2, buffer: {framer._buffer.hex()}\")\n    assert len(results) == 1, \"Should have produced result after part 2\"\n    \n    res = results[0]\n    assert res.function_code == 3\n    print(\"Test passed!\")\n\nif __name__ == \"__main__\":\n    asyncio.run(test_tcp_reassembly())\n",
  "filepath": "/tmp/repro.py"
}

{
  "status": "error",
  "error_type": "FileWriteError",
  "error_message": "Path traversal detected: '/tmp/repro.py' escapes workspace root."
}