How to safely stop a IOCP WSARecv() task, and free the WSAOVERLAPPED structure?

Question

My IOCP server program will cost more and more memory while running. After tracking memory leak ,I found that some WSAOVERLAPPED structue feed to WSARecv() is never recycled. I think this is because some malignant client socket only establish connection but never send data or close. So i set a TimerQueueTimer() on every client socket to identify the timeout socket and delete them. But if I free the WSAOVERLAPPED structure when deleting the malignant socket, after a little while ,I got "Free Heap block 014C7D80 modified at 014C7DA8 after it was freed".

Here is some of the relevant code:

typedef struct _SocketState
{
   char operation; 
   SOCKET socket;   
   DWORD length;
   HANDLE hTimer;
   HANDLE hCompletion;
   WSAOVERLAPPED* thisOvl;
   char buf[MAX_BUF];
} SocketState;

static WSAOVERLAPPED* new_overlapped(void)
{
   return (WSAOVERLAPPED*)calloc(1, sizeof(WSAOVERLAPPED));
}

static void create_io_completion_port(void)
{
   cpl_port = CreateIoCompletionPort(INVALID_HANDLE_VALUE, NULL, 0, 0);
   if (!cpl_port)
   {
        int err = WSAGetLastError();
        exit(1);
   }
}

static void post_reading(SocketState* socketState, WSAOVERLAPPED* ovl)
{
   DWORD flags = 0;
   WSABUF wsabuf = { MAX_BUF, socketState->buf };
   int err=0;
   memset(ovl, 0, sizeof(WSAOVERLAPPED));
   socketState->operation = OP_READ;
   socketState->thisOvl=ovl;
   if (WSARecv(socketState->socket, &wsabuf, 1, NULL, &flags, ovl, NULL)== SOCKET_ERROR)
   {
      err = WSAGetLastError();
      if (err != WSA_IO_PENDING)
      {
        printf("[%s:%d]WSARecv error\n", __FUNCTION__, __LINE__);
        destroy_connection(socketState, ovl);
        return ;
      }
   }
   c_WSARecv++;
}

static void destroy_connection(SocketState* socketState, WSAOVERLAPPED* ovl)
{
    int err=0;
    if(socketState->hTimer != NULL)
    {
        DeleteTimerQueueTimer(hTimerQueue,socketState->hTimer,INVALID_HANDLE_VALUE);
        socketState->hTimer = NULL; 
    }
    socketState->hCompletion=NULL; //newSocketState->hCompletion = cpl_port
    closesocket(socketState->socket);
    free(socketState);
    if(ovl!=0)
    {
       free(ovl);
    }
}
VOID CALLBACK TimerRoutine(PVOID lpParam, BOOLEAN TimerOrWaitFired)
{
    SocketState* clientSocketState=(SocketState*)lpParam;
    if (lpParam != NULL)
    {
        if(clientSocketState->hCompletion != NULL)
        {
            PostQueuedCompletionStatus(clientSocketState->hCompletion,-2,(ULONG_PTR)clientSocketState,clientSocketState->thisOvl);
            //should last parameter be NULL?
            //the "-2" is for identify this timeout io after GetQueuedCompletionStatus()
        }
    }
}

As I am using C rather then C++ for my server program, I am putting my self in very awkward situation. Basically I can't find a very good IOCP example in C :(.

Answer 1

OVERLAPPED we can free only after I/O completed. so really all what need cancel I/O operation. this can be done by call CancelIoEx or better by call closesocket (The closesocket function will initiate cancellation on the outstanding I/O operations). when I/O completed - you got pointer to OVERLAPPED passed to this I/O and after handle I/O result - you can free or reuse OVERLAPPED

---

you have structure associated with every socket - SocketState this is correct. but what must and must not have this structure ?

it mandatory must implement reference counting (because it accessing from multiple threads in complex and unpredictable order). and hold handle to the socket. mandatory have some kind of rundown protection for socket handle for not use it after closesocket/CancelIoEx calls (it implementation already separate question). this is need because A Winsock client must never issue closesocket on socket concurrently with another Winsock function call but we need at any time be able call closesocket for cancel I/O on lost remote side.

from another side it must not have pointer to OVERLAPPED (or it shell class) because multiple I/O can be at once on socket. we can in parallel read and write. it must not have operation member too by same reason - read and write operations for example can be active in parallel. so code like

socketState->operation = OP_READ;
socketState->thisOvl=ovl;

is wrong by design. also no sense have hCompletion inside SocketState because hCompletion is not per socket. here is wrong place for store it.

also we need mandatory use not naked OVERLAPPED structure for pass it to I/O but self class inherited from OVERLAPPED. you need have additional members here - referenced pointer to your SocketState - because when I/O completed - you got back pointer to OVERLAPPED and need from it got pointer to your socketState. the operation also mandatory must be here (instead SocketState) because operation is per I/O but not per socket. so in very general can be next:

struct SocketState
{
    SOCKET socket;   
    HANDLE hTimer;
    ULONG dwRefCount;

    void AddRef();
    void Release();

    _NODISCARD SOCKET LockHandle();
    void Rundown();
    void UnlockHandle();// call closesocket on last unlock

    void OnIoComplete(ULONG operation, ULONG dwErrorCode, ULONG dwBytesTransfered, PVOID buf);

    void StartSomeIo(ULONG operation, PVOID buf, ULONG cb);

    void Close()
    {
        if (LockHandle())
        {
            Rundown();
            UnlockHandle();
        }
    }
};

struct UIrp : OVERLAPPED
{
    SocketState* socketState;
    ULONG operation;
    PVOID buf;

    UIrp(SocketState* socketState, ULONG operation, PVOID buf) 
        : socketState(socketState), operation(operation), buf(buf)
    {
        RtlZeroMemory(static_cast<OVERLAPPED*>(this), sizeof(OVERLAPPED));
        socketState->AddRef();
    }

    ~UIrp()
    {
        socketState->Release();
    }

    void OnIoComplete(ULONG dwErrorCode, ULONG dwBytesTransfered)
    {
        socketState->OnIoComplete(operation, dwErrorCode, dwBytesTransfered, buf);
        delete this;
    }
};

void SocketState::StartSomeIo(ULONG operation, PVOID buf, ULONG cb)
{
    if (UIrp* irp = new UIrp(this, operation, buf))
    {
        ULONG dwError = ERROR_INVALID_HANDLE;

        if (SOCKET s = LockHandle())
        {
            dwError = WSA*(s,... irp, 0) == 0 ? NOERROR : WSAGetLastError();
            UnlockHandle();
        }

        switch (dwError)
        {
        case NOERROR:
        case WSA_IO_PENDING:
            break;
        default:
            irp->OnIoComplete(dwError, 0);
        }
    }
}

void PortLoop(HANDLE hCompletionPort)
{
    for (;;)
    {
        OVERLAPPED* lpOverlapped;
        ULONG dwBytesTransfered;
        ULONG_PTR CompletionKey;

        ULONG dwError = GetQueuedCompletionStatus(hCompletionPort, &dwBytesTransfered, 
            &CompletionKey, &lpOverlapped, INFINITE) ? NOERROR : GetLastError();

        // probably somehow use CompletionKey

        if (!lpOverlapped)
        {
            break;
        }

        static_cast<UIrp*>(lpOverlapped)->OnIoComplete(dwBytesTransfered, dwError);
    }
}

about

set set a TimerQueueTimer() on every client socket

this is possible and correct by design but think not the best in case you have many sockets. i be inherit SocketState from LIST_ENTRY and insert all active sockets to the some list. and by timer periodically check timeout and close sockets