From e1f671a6948996cd0e8f5e3b030a29b2106a127c Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Tue, 10 Feb 2026 23:23:31 +0000 Subject: [PATCH] NetAcceptAction::cancel() use-after-free fix: part 2 This is a revised version of PR #12803 which was reverted in PR #12841 because it lacked the idempotent cancel guard, causing its own set of crashes. There is a race between NetAcceptAction::cancel() and NetAccept::acceptEvent() where the server pointer could be dereferenced after the NetAccept object was deleted. Thread A calls cancel(), which sets cancelled=true via Action::cancel(). Thread B running acceptEvent() sees cancelled==true and deletes the NetAccept (including the embedded Server). Thread A then calls server->close() on freed memory. This is fixed by making the server pointer in NetAcceptAction atomic and using exchange(nullptr) so only one thread can obtain and close the server. Additionally, cancel() is made idempotent by checking !cancelled before calling Action::cancel(), which prevents ink_assert(!cancelled) assertion failures when cancel is called from both external callers (TSActionCancel) and internal error paths (acceptEvent, acceptFastEvent, acceptLoopEvent). --- src/iocore/net/P_NetAccept.h | 28 +++++++++++++++++++--------- src/iocore/net/QUICNetProcessor.cc | 4 +--- src/iocore/net/UnixNetAccept.cc | 8 +++++--- src/iocore/net/UnixNetProcessor.cc | 4 +--- 4 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/iocore/net/P_NetAccept.h b/src/iocore/net/P_NetAccept.h index 163d5578d52..89650e34fa6 100644 --- a/src/iocore/net/P_NetAccept.h +++ b/src/iocore/net/P_NetAccept.h @@ -43,6 +43,7 @@ #include "iocore/net/NetAcceptEventIO.h" #include "Server.h" +#include #include struct NetAccept; @@ -60,21 +61,30 @@ AcceptFunction net_accept; class UnixNetVConnection; -// TODO fix race between cancel accept and call back struct NetAcceptAction : public Action, public RefCountObjInHeap { - Server *server; + std::atomic server{nullptr}; - void - cancel(Continuation *cont = nullptr) override + NetAcceptAction(Continuation *cont, Server *s) { - Action::cancel(cont); - server->close(); + continuation = cont; + if (cont != nullptr) { + mutex = cont->mutex; + } + server.store(s, std::memory_order_release); } - Continuation * - operator=(Continuation *acont) + void + cancel(Continuation *cont = nullptr) override { - return Action::operator=(acont); + // Use atomic exchange so only one thread closes the server, preventing + // use-after-free races between cancel() and acceptEvent() cleanup. + Server *s = server.exchange(nullptr, std::memory_order_acq_rel); + if (s != nullptr) { + s->close(); + } + if (!cancelled) { + Action::cancel(cont); + } } ~NetAcceptAction() override diff --git a/src/iocore/net/QUICNetProcessor.cc b/src/iocore/net/QUICNetProcessor.cc index 55592ef6f36..d7f7012606a 100644 --- a/src/iocore/net/QUICNetProcessor.cc +++ b/src/iocore/net/QUICNetProcessor.cc @@ -251,9 +251,7 @@ QUICNetProcessor::main_accept(Continuation *cont, SOCKET fd, AcceptOptions const na->server.sock = UnixSocket{fd}; ats_ip_copy(&na->server.accept_addr, &accept_ip); - na->action_ = new NetAcceptAction(); - *na->action_ = cont; - na->action_->server = &na->server; + na->action_ = new NetAcceptAction(cont, &na->server); na->init_accept(); return na->action_.get(); diff --git a/src/iocore/net/UnixNetAccept.cc b/src/iocore/net/UnixNetAccept.cc index cea9df78792..66e73096fa2 100644 --- a/src/iocore/net/UnixNetAccept.cc +++ b/src/iocore/net/UnixNetAccept.cc @@ -317,9 +317,7 @@ NetAccept::init_accept_per_thread() void NetAccept::stop_accept() { - if (!action_->cancelled) { - action_->cancel(); - } + action_->cancel(); server.close(); } @@ -479,6 +477,7 @@ NetAccept::acceptEvent(int event, void *ep) MUTEX_TRY_LOCK(lock, m, e->ethread); if (lock.is_locked()) { if (action_->cancelled) { + // Server was already closed by whoever called cancel(). e->cancel(); Metrics::Gauge::decrement(net_rsb.accepts_currently_open); delete this; @@ -487,6 +486,7 @@ NetAccept::acceptEvent(int event, void *ep) int res; if ((res = net_accept(this, e, false)) < 0) { + action_->cancel(); Metrics::Gauge::decrement(net_rsb.accepts_currently_open); /* INKqa11179 */ Warning("Accept on port %d failed with error no %d", ats_ip_port_host_order(&server.addr), res); @@ -637,6 +637,7 @@ NetAccept::acceptFastEvent(int event, void *ep) return EVENT_CONT; Lerror: + action_->cancel(); server.close(); e->cancel(); Metrics::Gauge::decrement(net_rsb.accepts_currently_open); @@ -656,6 +657,7 @@ NetAccept::acceptLoopEvent(int event, Event *e) } // Don't think this ever happens ... + action_->cancel(); Metrics::Gauge::decrement(net_rsb.accepts_currently_open); delete this; return EVENT_DONE; diff --git a/src/iocore/net/UnixNetProcessor.cc b/src/iocore/net/UnixNetProcessor.cc index 15630281c5c..5c8d5111e1b 100644 --- a/src/iocore/net/UnixNetProcessor.cc +++ b/src/iocore/net/UnixNetProcessor.cc @@ -133,9 +133,7 @@ UnixNetProcessor::accept_internal(Continuation *cont, int fd, AcceptOptions cons na->proxyPort = sa ? sa->proxyPort : nullptr; na->snpa = dynamic_cast(cont); - na->action_ = new NetAcceptAction(); - *na->action_ = cont; - na->action_->server = &na->server; + na->action_ = new NetAcceptAction(cont, &na->server); if (opt.frequent_accept) { // true if (accept_threads > 0 && listen_per_thread == 0) {