This is an automated email from the ASF dual-hosted git repository.
bneradt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 6112a78356 NetAcceptAction::cancel() use-after-free fix: part 2
(#12874)
6112a78356 is described below
commit 6112a78356a86fbf86c466932d554ba8e932d3e4
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).
---
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) {
