Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TCP length header included device ID? #59

Open
muhlemmer opened this issue May 16, 2020 · 0 comments
Open

TCP length header included device ID? #59

muhlemmer opened this issue May 16, 2020 · 0 comments

Comments

@muhlemmer
Copy link

I wanted to learn more about the MODBUS TCP protocol and found this library and started reading the sources to get a better understanding. I came across this in the tcpTransporter.Send() method:

modbus/tcpclient.go

Lines 173 to 178 in 606c02f

var data [tcpMaxLength]byte
if _, err = io.ReadFull(mb.conn, data[:tcpHeaderSize]); err != nil {
return
}
// Read length, ignore transaction & protocol id (4 bytes)
length := int(binary.BigEndian.Uint16(data[4:]))

Where:

modbus/tcpclient.go

Lines 22 to 23 in 606c02f

tcpHeaderSize = 7
tcpMaxLength = 260

Unless I'm not seeing it right: After io.ReadFull, data has 7 bytes populated, remaining 253 bytes are 0x00. Decoding it with data[4:] practically passes 3 populated bytes.

This will result in length to contain a larger value if the device ID is non zero. This could then lead to a false positive on the length error check further down the code.

Also, the aduResponse might end up being longer. This wouldn't be a problem if the PDU unpacking takes proper precautions on lengths, haven't checked.

aduResponse = data[:length]

I'm not actually using this project, so I'm not able to confirm if this an actual bug and I might be completely wrong about my assumption. Just trying to learn new things :D.

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

No branches or pull requests

1 participant