Write to freed memory area with IoCompletionPort (IOCP) ioqueue backend may lead to application crash

[pjsipbot]

2009-11-05 13:40:55: @bennylp created the issue on trac ticket 985

Using IoCompletionPort (IOCP) ioqueue backend, which is the default backend on Win32 on Release build, may lead to write access to memory area that has been freed before. This potentially could crash the application, or random memory corruption.

This symptom can be detected with Visual Studio 2005 (but not with Visual Studio 6).

Explanation about this bug is as follows.

1. When socket is closed, Windows automatically unregisters the socket from the IOCP. Any pending operations to the socket are also cancelled.
2. But when the next time the IOCP is polled with GetQueuedCompletionStatus(), Windows would still report an event to the pending operations that have been cancelled above.
3. If the memory that corresponds to the WSAOVERLAPPED structure that was registered for the asynchronous operation has been freed (for example, the pool is released), Windows will write some values to these area that was previously occupied by the WSAOVERLAPPED structure.

---

2010-02-25 11:54:47: @bennylp changed milestone from release-1.6 to release-1.7

---

2010-04-28 01:16:16: @bennylp changed status from new to closed

---

2010-04-28 01:16:16: @bennylp set resolution to worksforme

---

2010-04-28 01:16:16: @bennylp changed milestone from release-1.7 to release-1.5.5

---

2010-04-28 01:16:16: @bennylp commented

Turns out this has been "fixed" in milestone:release-1.6 by ticket #1015, by disabling IOCP backend altogether.

Changing the milestone to 1.5.5 and closing the ticket.

---

2010-04-28 01:16:52: @bennylp commented

(The above should read milestone:release-1.5.5 instead of 1.6).

[jimying]

Any plan to fix?

[nanangizz]

I am afraid currently no, we haven't got an idea for solution to the problem described in the bug explanation above.

[jimying]

I have an idea:
Use CancelIoEx() to cancel pending read/write before close socket
(CancelIoEx not support XP)

like this:

fnCancelIoEx = (FnCancelIoEx)GetProcAddress(GetModuleHandle(PJ_T("Kernel32.dll")), "CancelIoEx");
for (readlist)
    // fnCancelIoEx(icop, overlapped)   cancel pending read
for (writelist)
    // fnCancelIoEx(icop, overlapped)   cancel pending write

// finally close socket
pj_sock_close();

[jimying]

I have tested CancelIoEx(), not working, GetQueuedCompletionStatus() still report an event after closesocket.

I have an new idea:
we can add a pending_list in struct pj_ioqueue_key_t to manage overlapp memory self (replace use pj_ioqueue_op_key_t.internal__ to store overlapp),
which's item struct like this:

struct pending_op {
     union operation_key  pending_key;   //  overlapp memory store here
     pj_ioqueue_op_key_t *op_key;    // application op key
     pj_pool_t *pool;   // the new pool to stored this struct, release after pending operation  complete
}

in pj_ioqueue_recvfrom(), pj_ioqueue_recv(), pj_ioqueue_sendto(), pj_ioqueue_accept()
alloc a new pending_op and insert into pending_list,
when GetQueuedCompletionStatus() report an event, we free pending_op.

In this solution, we manage WSAOVERLAPPED memory (pending_op) in the ioqueue internal, not use top level application pool.

[nanangizz]

I have tested CancelIoEx(), not working, GetQueuedCompletionStatus() still report an event after closesocket.

Thanks for letting us know, haven't got a chance to setup test env myself.

Btw from sample code here, it looks like the cancelation is not synchronous (needs to be acknowledged or something?).

I have an new idea: we can add a pending_list in struct pj_ioqueue_key_t to manage overlapp memory self (replace use pj_ioqueue_op_key_t.internal__ to store overlapp), which's item struct like this:

struct pending_op {
     union operation_key  pending_key;   //  overlapp memory store here
     pj_ioqueue_op_key_t *op_key;    // application op key
     pj_pool_t *pool;   // the new pool to stored this struct, release after pending operation  complete
}

in pj_ioqueue_recvfrom(), pj_ioqueue_recv(), pj_ioqueue_sendto(), pj_ioqueue_accept() alloc a new pending_op and insert into pending_list, when GetQueuedCompletionStatus() report an event, we free pending_op.

In this solution, we manage WSAOVERLAPPED memory (pending_op) in the ioqueue internal, not use top level application pool.

Right, we cannot use app-allocated WSAOVERLAPPED for socket operations as app may release the memory anytime after pj_ioqueue_unregister() returns, while the OS may still access/write to it after.

The pj_ioqueue_unregister() should not free pj_ioqueue_key_t immediately, it must wait any pending operations to complete (perhaps the existing group lock can be used for this, i.e: register to group lock handler to free the key).

Few questions:

• does the event always come after the socket is closed (so the pool will always be released)?
• how about the data buffer (e.g: receive buffer), which may be freed immediately by app after pj_ioqueue_unregister(), won't it be a problem?

[jimying]

• does the event always come after the socket is closed (so the pool will always be released)?
  sure, we need free all pool.
  I think we can use CancelioEx() to cancel all pending operation in pending_list before closesocket.
  If CancelioEx() success, GetQueuedCompletionStatus() report an event(cancel event), so we can make sure all pool released
• how about the data buffer (e.g: receive buffer), which may be freed immediately by app after pj_ioqueue_unregister(), won't it be a problem?
  key->closing already is true, so callback never triggered.

[nanangizz]

• does the event always come after the socket is closed (so the pool will always be released)?
  sure, we need free all pool.
  I think we can use CancelioEx() to cancel all pending operation in pending_list before closesocket.
  If CancelioEx() success, GetQueuedCompletionStatus() report an event(cancel event), so we can make sure all pool released

Have you tried it? Each pending operation receives its event (either cancelation or completion) so every pool is freed?

If there is possibility that the event never comes, perhaps we need to use somekind of timeout mechanism, but then it also means this solution is still not very safe (OS may still write to overlapped after timeout, e.g: when the CPU load is high).

* how about the data buffer (e.g: receive buffer), which may be freed immediately by app after pj_ioqueue_unregister(), won't it be a problem?
  key->closing already is true, so callback never triggered.

I mean if the OS still writes to the freed data buffer (just as to the overlapped), it will cause problems.

Update:
Sorry, ignore the above, I forgot that the app data buffer should only be written after read event is triggered & socket recv() is called.