diff --git a/src/iocore/net/P_NetAccept.h b/src/iocore/net/P_NetAccept.h index 163d5578d52..22a2dd5bca6 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,29 @@ 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); + // Close the server before setting the cancelled flag. This ensures that + // when acceptEvent() sees cancelled == true, the server close is already + // complete, preventing use-after-free races. + Server *s = server.exchange(nullptr, std::memory_order_acq_rel); + if (s != nullptr) { + s->close(); + } + 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..4b69b8641fe 100644 --- a/src/iocore/net/UnixNetAccept.cc +++ b/src/iocore/net/UnixNetAccept.cc @@ -479,6 +479,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 +488,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,7 +639,7 @@ NetAccept::acceptFastEvent(int event, void *ep) return EVENT_CONT; Lerror: - server.close(); + action_->cancel(); e->cancel(); Metrics::Gauge::decrement(net_rsb.accepts_currently_open); delete this; @@ -656,6 +658,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) {