This is an automated email from the ASF dual-hosted git repository. cmcfarlen pushed a commit to branch 10.2.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit ed4f0b636300fe787bb8c779865387e2b7efa554 Author: Brian Neradt <[email protected]> AuthorDate: Fri Feb 13 17:14:45 2026 -0600 NetAcceptAction::cancel() use-after-free fix: part 2 (#12874) 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). (cherry picked from commit 6112a78356a86fbf86c466932d554ba8e932d3e4) --- 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 163d5578d5..89650e34fa 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 <atomic> #include <vector> 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 *> 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 55592ef6f3..d7f7012606 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 cea9df7879..66e73096fa 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 @@ Ldone: 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 15630281c5..5c8d5111e1 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<SSLNextProtocolAccept *>(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) {
