search

IGSIO / OpenIGTLinkIO

Library for interfacing to openigtlink/OpenIGTLink, dependent on VTK and Qt. Based on openigtlink/OpenIGTLinkIF
Apache License 2.0
13 stars 24 forks source link

IGTL Message Header Corruption #65

Open Sunderlandkyl opened 6 years ago

Sunderlandkyl commented 6 years ago

Since adding a timeout to the sockets, I've observed that the message header can sometimes become corrupted when it times out during: https://github.com/IGSIO/OpenIGTLinkIO/blob/ab439c20952efc5bac214d3486c5c2977b44b4f5/Logic/igtlioConnector.cxx#L430

This results in the message being filled with garbage, and when the message buffer is allocated, it will be be allocated using a random (often large!) size.

This means that OpenIGTLink needs to be able to determine from the header contents if a message header is valid, and if it isn't, decide how to recover from the failure (Read all remaining data before resuming communication?).

What are everyone's thoughts?

lassoan commented 6 years ago

Checksum is only computed for message body, so probably there is no sure way of detecting corruption.

We could also use some heuristics in detecting invalid header, e.g., if header version is not 1, 2, or 3 then we consider the header invalid.

We might change the message header to include a checksum.

We could also close the socket if there is a timeout (because most likely the next receive call will still read remaining body bytes, which will be tried to be interpreted as header, leading to errors).

adamrankin commented 6 years ago

What is the value of r when it times out?

adamrankin commented 6 years ago

Is it not as simple as changing 434 to be r == -1 || r != headerMsg->GetPackSize()?

Or if we don't want to access headerMsg at all, write it to a temporary buffer, then transfer it to the header message?

adamrankin commented 6 years ago

or possibly if( r == -1 || r != IGTL_HEADER_SIZE )

lassoan commented 6 years ago

A message read times out, then reading of the next message header is complete, but it contains garbage (some remaining bytes of the previous message).

Sunderlandkyl commented 6 years ago

r is 58, so the same as IGTL_HEADER_SIZE

adamrankin commented 6 years ago

A message read times out, then reading of the next message header is complete, but it contains garbage (some remaining bytes of the previous message).

Is this a bug in the socket at the OS level?