diff --git a/.release-notes/fix-iocp-toctou.md b/.release-notes/fix-iocp-toctou.md new file mode 100644 index 0000000000..bb4c3a7302 --- /dev/null +++ b/.release-notes/fix-iocp-toctou.md @@ -0,0 +1,5 @@ +## Fix use-after-free in IOCP ASIO system + +We fixed a pair of use-after-free races in the Windows IOCP event system. A previous fix introduced a token mechanism to prevent IOCP callbacks from accessing freed events, but missed two windows where raw pointers could outlive the event they pointed to. One was between the callback and event destruction, the other between a queued message and event destruction. + +This is the hard part that Pony protects you from. Concurrent access to mutable data across threads is genuinely difficult to get right, even when you have a mechanism designed specifically to handle it. diff --git a/src/libponyrt/actor/actor.c b/src/libponyrt/actor/actor.c index 4ae791fd85..7321a5cec5 100644 --- a/src/libponyrt/actor/actor.c +++ b/src/libponyrt/actor/actor.c @@ -1,6 +1,7 @@ #define PONY_WANT_ATOMIC_DEFS #include "actor.h" +#include "../asio/event.h" #include "../sched/scheduler.h" #include "../sched/cpu.h" #include "../mem/pool.h" @@ -472,6 +473,30 @@ static bool handle_message(pony_ctx_t* ctx, pony_actor_t* actor, DTRACE3(ACTOR_MSG_RUN, (uintptr_t)ctx->scheduler, (uintptr_t)actor, msg->id); actor->type->dispatch(ctx, actor, msg); + +#ifdef PLATFORM_IS_WINDOWS + // Release the IOCP message reference taken in pony_asio_event_send. + // The event pointer in the message must stay alive through dispatch; + // now that the behavior has returned, we can release it. + if(actor->type->event_notify != (uint32_t)-1 && + msg->id == actor->type->event_notify) + { + asio_msg_t* am = (asio_msg_t*)msg; + asio_event_t* ev = am->event; + iocp_token_t* token = ev->iocp_token; + + if(atomic_fetch_sub_explicit(&token->refcount, 1, + memory_order_acq_rel) == 1) + { + if(atomic_load_explicit(&token->dead, memory_order_acquire)) + { + POOL_FREE(asio_event_t, ev); + POOL_FREE(iocp_token_t, token); + } + } + } +#endif + return true; } } diff --git a/src/libponyrt/asio/event.c b/src/libponyrt/asio/event.c index 67dbcd3b55..4ee01f6cab 100644 --- a/src/libponyrt/asio/event.c +++ b/src/libponyrt/asio/event.c @@ -35,7 +35,10 @@ PONY_API asio_event_t* pony_asio_event_create(pony_actor_t* owner, int fd, #ifdef PLATFORM_IS_WINDOWS ev->iocp_token = POOL_ALLOC(iocp_token_t); atomic_store_explicit(&ev->iocp_token->dead, false, memory_order_relaxed); - atomic_store_explicit(&ev->iocp_token->refcount, 0, memory_order_relaxed); + // Start at 1: the event itself holds a reference. IOCP operations add more. + // Destroy releases the event's reference; callbacks release theirs. + // The last releaser (refcount 1 -> 0) frees both the event and the token. + atomic_store_explicit(&ev->iocp_token->refcount, 1, memory_order_relaxed); #endif owner->live_asio_events = owner->live_asio_events + 1; @@ -72,7 +75,6 @@ PONY_API void pony_asio_event_destroy(asio_event_t* ev) ev->flags = ASIO_DESTROYED; #ifdef PLATFORM_IS_WINDOWS - // Grab the token pointer before freeing the event. iocp_token_t* token = ev->iocp_token; // Mark the token as dead. Any IOCP callback that hasn't yet checked the @@ -90,13 +92,18 @@ PONY_API void pony_asio_event_destroy(asio_event_t* ev) pony_assert(ev->owner->live_asio_events > 0); ev->owner->live_asio_events = ev->owner->live_asio_events - 1; - POOL_FREE(asio_event_t, ev); - #ifdef PLATFORM_IS_WINDOWS - // Free the token if no IOCP callbacks are outstanding. If callbacks are - // still in flight, the last one to complete will free the token. - if(atomic_load_explicit(&token->refcount, memory_order_acquire) == 0) + // Release the event's reference (created with refcount 1). If all IOCP + // callbacks have already completed, we're the last releaser and free the + // event and token. Otherwise the last callback to complete will free them. + if(atomic_fetch_sub_explicit(&token->refcount, 1, + memory_order_acq_rel) == 1) + { + POOL_FREE(asio_event_t, ev); POOL_FREE(iocp_token_t, token); + } +#else + POOL_FREE(asio_event_t, ev); #endif } @@ -162,6 +169,13 @@ PONY_API void pony_asio_event_send(asio_event_t* ev, uint32_t flags, // scheduler index if this is run on a normal scheduler thread and that would // be not good. pony_register_thread(); + + // Hold a token reference for the message. The event pointer stored in the + // message must remain valid until the actor finishes processing it. + // The corresponding release happens in handle_message (actor.c) after + // the behavior dispatch returns. + atomic_fetch_add_explicit(&ev->iocp_token->refcount, 1, + memory_order_relaxed); #endif asio_msg_t* m = (asio_msg_t*)pony_alloc_msg(POOL_INDEX(sizeof(asio_msg_t)), diff --git a/src/libponyrt/asio/event.h b/src/libponyrt/asio/event.h index 2f12695696..ccef03f351 100644 --- a/src/libponyrt/asio/event.h +++ b/src/libponyrt/asio/event.h @@ -14,8 +14,14 @@ PONY_EXTERN_C_BEGIN * IOCP completion callbacks fire on Windows thread pool threads after the * owning actor may have destroyed the event. Each in-flight IOCP operation * holds a pointer to this token. The callback checks the dead flag before - * touching the event; the refcount tracks how many callbacks are outstanding - * so the token itself can be freed when no longer needed. + * touching the event. + * + * The refcount starts at 1 (the event's own reference) and tracks three + * kinds of holders: each IOCP operation adds 1 (released on callback + * completion); each message sent via pony_asio_event_send adds 1 + * (released in handle_message after the behavior dispatch returns); + * destroy marks dead and subtracts the event's 1. Whoever decrements + * to zero frees both the event and the token. */ typedef struct iocp_token_t { diff --git a/src/libponyrt/lang/socket.c b/src/libponyrt/lang/socket.c index bf6f353c50..b3721b157f 100644 --- a/src/libponyrt/lang/socket.c +++ b/src/libponyrt/lang/socket.c @@ -228,24 +228,28 @@ static iocp_t* iocp_create(iocp_op_t op, asio_event_t* ev) return iocp; } -static void iocp_release_token(iocp_token_t* token) +static void iocp_release_token(iocp_token_t* token, asio_event_t* ev) { if(atomic_fetch_sub_explicit(&token->refcount, 1, memory_order_acq_rel) == 1) { // We were the last outstanding operation. If the event has been destroyed, - // nobody else will free the token — we do it. + // nobody else will free the event and token — we do it. if(atomic_load_explicit(&token->dead, memory_order_acquire)) + { + POOL_FREE(asio_event_t, ev); POOL_FREE(iocp_token_t, token); + } } } static void iocp_destroy(iocp_t* iocp) { iocp_token_t* token = iocp->token; + asio_event_t* ev = iocp->ev; POOL_FREE(iocp_t, iocp); if(token != NULL) - iocp_release_token(token); + iocp_release_token(token, ev); } static iocp_accept_t* iocp_accept_create(SOCKET s, asio_event_t* ev) @@ -265,10 +269,11 @@ static iocp_accept_t* iocp_accept_create(SOCKET s, asio_event_t* ev) static void iocp_accept_destroy(iocp_accept_t* iocp) { iocp_token_t* token = iocp->iocp.token; + asio_event_t* ev = iocp->iocp.ev; POOL_FREE(iocp_accept_t, iocp); if(token != NULL) - iocp_release_token(token); + iocp_release_token(token, ev); } static void CALLBACK iocp_callback(DWORD err, DWORD bytes, OVERLAPPED* ov) @@ -276,8 +281,9 @@ static void CALLBACK iocp_callback(DWORD err, DWORD bytes, OVERLAPPED* ov) iocp_t* iocp = (iocp_t*)ov; iocp_token_t* token = iocp->token; - // Check whether the event has been destroyed. If so, the event and its - // owning actor may already be freed — don't touch iocp->ev. + // Check whether the event has been destroyed. If so, skip the event and + // just release our reference (iocp_destroy will free the event if we're + // the last holder). // token is NULL for IOCP_NOP (e.g. UDP sendto) which has no event. if((token != NULL) && atomic_load_explicit(&token->dead, memory_order_acquire))