Skip to content

m_CurrentReadIndex in SessionManager::ProcessMessage() increases uncontrollably #271

Description

@eazpiazudeneb

Steps to reproduce the error:

  1. Try to read a message by calling SessionManager::ProcessMessage().
  2. The header is read correctly and a message handler that matches to the message type for reading the body is found.
  3. The message handler's ReceiveMessage method is called. At this point m_CurrentReadIndex = IGTL_HEADER_SIZE.
    igtl_uint64 r = this->m_CurrentMessageHandler->ReceiveMessage(this->m_Socket, this->m_Header, this->m_CurrentReadIndex-IGTL_HEADER_SIZE);
  4. The method igtlUint64 Socket::Receive(void* data, igtlUint64 length, bool& timeout, int readFully/*=1*/) gets called.
  5. If n<0 after calling int n = recv(this->m_SocketDescriptor, buffer + total, readLength, 0);, then timeout = true; return 0;. This happens when we get a WSAETIMEDOUT.
  6. As timeout is true, the handler's ReceiveMessage method returns pos, which is the third input argument, this->m_CurrentReadIndex-IGTL_HEADER_SIZE in this case.
  7. Back into SessionManager::ProcessMessage(), the if evaluates to false as the body has not been read, so m_CurrentReadIndex is incremented in the else. After this, m_CurrentReadIndex = 2 * IGTL_HEADER_SIZE.
if (r == this->m_Header->GetBodySizeToRead())
{
    this->m_CurrentReadIndex = 0;
    this->m_HeaderDeserialized = 0;
}
else
{
    this->m_CurrentReadIndex += IGTL_HEADER_SIZE + r;
}

return 1;
  1. The next time SessionManager::ProcessMessage() is called, still m_CurrentReadIndex = 2 * IGTL_HEADER_SIZE, so both checks to read the header do not evaluate to true and the body is read directly.
if (this->m_CurrentReadIndex == 0)
{
    ...
}
else if (this->m_CurrentReadIndex < IGTL_HEADER_SIZE)
{
    ...
}
  1. Steps 2-8 are repeated, but with m_CurrentReadIndex is increased each time.
  2. At one point, _m_CurrentReadIndex _ is too big and it causes an invalid pointer address when calling igtlUint64 Socket::Receive(void* data, igtlUint64 length, bool& timeout, int readFully/*=1*/), because data = (void*)((char*)this->m_Message->GetBufferBodyPointer()+pos. Because of this invalid pointer, the socket returns a WSAEFAULT which can cause the socket to close and, consequently, return WSAECONNABORTED in the upcoming calls.

We do not understand the point of the following lines of code in SessionManager::ProcessMessage().

if (r == this->m_Header->GetBodySizeToRead())
{
    this->m_CurrentReadIndex = 0;
    this->m_HeaderDeserialized = 0;
}
else
{
    this->m_CurrentReadIndex += IGTL_HEADER_SIZE + r;
}

return 1;

Why not just this->m_CurrentReadIndex = 0; this->m_HeaderDeserialized = 0; to read each message from the first index after an error?

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions