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).

Author: bneradt

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

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();

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;

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) {