Fix NetAcceptAction::cancel() use-after-free race condition

Fix a race condition between NetAcceptAction::cancel() and
NetAccept::acceptEvent() where the server pointer could be dereferenced
after the NetAccept object was deleted.

The race occurred as follows:
1. Thread T4 calls NetAcceptAction::cancel(), sets cancelled=true
2. Thread T3 running acceptEvent() sees cancelled==true
3. Thread T3 deletes the NetAccept (including embedded Server)
4. Thread T4 tries to call server->close() on freed memory

The fix uses std::atomic<Server*> with atomic exchange to ensure only
one thread can successfully obtain and use the server pointer. Both
cancel() and the cleanup paths before delete this atomically exchange
the pointer with nullptr - whichever succeeds first closes the server,
the other becomes a no-op.

This addresses the TODO comment that was in the code:
"// TODO fix race between cancel accept and call back"

ASAN report this fixes (seen intermittently on rocky CI builds):

==8850==ERROR: AddressSanitizer: heap-use-after-free on address 0x616000028cb4 at pc 0x000001346739 bp 0x7fa40fd2f580 sp 0x7fa40fd2f570
WRITE of size 4 at 0x616000028cb4 thread T4 ([ET_NET 2])
    #0 0x1346738 in UnixSocket::close() ../src/iocore/eventsystem/UnixSocket.cc:138
    #1 0x12b44ed in Server::close() ../src/iocore/net/Server.cc:88
    #2 0x121fb95 in NetAcceptAction::cancel(Continuation*) ../src/iocore/net/P_NetAccept.h:71
    #3 0x7fa41686d082 in TSActionCancel(tsapi_action*) ../src/api/InkAPI.cc:5828
    ...

0x616000028cb4 is located 308 bytes inside of 576-byte region [0x616000028b80,0x616000028dc0)
freed by thread T3 ([ET_NET 1]) here:
    #0 0x7fa416d2036f in operator delete(void*, unsigned long)
    #1 0x12593c4 in NetAccept::~NetAccept() ../src/iocore/net/P_NetAccept.h:128
    #2 0x12bebf0 in NetAccept::acceptEvent(int, void*) ../src/iocore/net/UnixNetAccept.cc:484
    ...

Author: bneradt

.cursor/commands/apply-pr-comments.md

@@ -0,0 +1,13 @@
+# Evaluate and Apply Review Comments
+
+Using gh, view the review comments on the PR. Evalaute what is valuable to
+apply changes from and how to reply to comments that are not valuable to make
+changes from.
+
+- Do not push replies to the PR. Exlain to me what is valuable to reply and I
+  will evaluate your feedback.
+- Do any changes from the review coments.
+- Build: @build-ats.md.
+- Run any autests: @run-autests.md
+- Do not commit the changes, leave them unstaged for me to review.
+

.cursor/commands/build-ats.md

@@ -0,0 +1,42 @@
+# Build Apache Traffic Server
+
+ATS is a cmake project that I build using ninja.
+
+- I have helper scripts in ~/bin/ to run the initial cmake command and build it.
+- Subsequent builds, after the `build` directory is created can be built simply with cmake --build
+
+## First build
+
+Use this generally for the initial build:
+~/bin/build_ats
+
+Use this if unit tests are required:
+~/bin/build_ats_ut
+
+## Subsequent builds
+
+If the `build` directory exists already, then don't waste time with a full
+cmake -B command via those `build_ats` scripts. Simply do the following:
+
+```
+cmake --build build
+cmake --install build
+```
+
+## Formatting
+
+Before doing any commit, make sure the code is formatted. A pre-commit hook
+will prevent the commit if it is not formatted.
+
+```
+cmake --build build --target format
+```
+
+## Docs Build
+
+Doc builds are easier:
+
+```
+cmake -B docs-build --preset ci-docs
+cmake --build docs-build --target generate_docs -v
+```

.cursor/commands/commit.md

@@ -0,0 +1,11 @@
+# Generate a git commit
+
+When creating a git commit for a patch:
+
+[ ] Run `cmake --build build --target format` if not run since last change. A pre-commit hook will prevent commits unless the code is formatted.
+[ ] Create a short one line summary. Probably less than 60 characters.
+[ ] Write a concise couple paragraphs: describe the problem being addressed and the solution.
+[ ] Describe the solution at a high level. If people want code details, they can look at the patch.
+[ ] If the commit addresses an issue, end with: `Fixes: #<issue_number>`
+
+Do not push the commit unless explicitly asked to.

.cursor/commands/plan-issue.md

@@ -0,0 +1,11 @@
+# Write a plan to fix an issue.
+
+Using gh, view the content of the issue. Make a plan for how to address it.
+
+[ ] Read the description and the relevant code. Understand the issue.
+[ ] Plan for how to write tests for the issue, if applicable. Consider catch2 tests as well as autests (see @writing-autests.mdc).
+[ ] Write the plan for the production fix.
+[ ] Plan to build it using /build-ats.
+[ ] Plan to run any added autests with /run-autests
+
+Unless asked otherwise, do not post to the issue. I will do this myself.

.cursor/commands/review-pr.md

@@ -0,0 +1,12 @@
+# Review a PR
+
+Using gh, view the contents of the PR. Some good things to check for:
+
+[ ] Expensive pass by copy rather than reference or pointer.
+[ ] Opportunities for using modern C++ features.
+[ ] Opportunities for using the lib/libswoc/ API, such as TextView.
+[ ] Resource leaks
+[ ] Duplicated code
+
+And any other generic software issues you might see or issues with the
+domain-specific patch.

.cursor/commands/run-autests.md

@@ -0,0 +1,23 @@
+# Run the autests
+
+The autests are automated end to end tests, see @writing-autests.mdc.
+
+They are not run via directly calling the python interpreter, despite being .py
+files. That won't work. Rather they must be run via the autest.sh script in the
+build directory.
+
+After building and installing ATS (see @build-ats.md):
+
+```
+cd build/tests
+./autest.sh --sandbox /tmp/sb --clean=none -f test_name...
+```
+
+The `test_name` passed there is the name of the autest without the .test.py
+exension. Thus, to run the `show_ssl_multicert.test.py` test, you would:
+
+
+```
+cd build/tests
+./autest.sh --sandbox /tmp/sb --clean=none -f show_ssl_multicert
+```

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,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 *> 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

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

@@ -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;

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