Re: [PR] fix cancel/accept race in NetAccept that causes EBADF abort [trafficserver]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #12925:
URL: https://github.com/apache/trafficserver/pull/12925#discussion_r2875071068


##########
src/iocore/net/UnixNetAccept.cc:
##########
@@ -112,7 +112,7 @@ net_accept(NetAccept *na, void *ep, bool blockable)
       if (res == -EAGAIN || res == -ECONNABORTED || res == -EPIPE) {
         goto Ldone;
       }
-      if (na->server.sock.is_ok() && !na->action_->cancelled) {
+      if (na->action_->server.load(std::memory_order_acquire) != nullptr && na->server.sock.is_ok() && !na->action_->cancelled) {

Review Comment:
   In `net_accept`, the updated condition at line 115 adds `na->action_->server.load(std::memory_order_acquire) != nullptr` as the primary atomic guard, but retains the pre-existing `na->server.sock.is_ok()` check. This is now redundant: when `action_->cancel()` runs, it first atomically exchanges `server` to `nullptr`, then closes the socket. So if the atomic load returns `nullptr`, short-circuit evaluation means `sock.is_ok()` is never reached. If the atomic load returns non-null, the socket is guaranteed to still be open (cancel hasn't run yet).
   
   The other two fixed paths (`do_blocking_accept` and `acceptFastEvent`) correctly use only the atomic server pointer check, without an additional `sock.is_ok()` check, which is the right and consistent approach.
   
   The `na->server.sock.is_ok()` sub-expression also performs a non-atomic read of the socket's file descriptor, which could be written concurrently by another thread calling `cancel()`. While in practice this doesn't create a real correctness problem (the atomic check is authoritative), it is an unnecessary TOCTOU discrepancy between the three accept paths.
   ```suggestion
         if (na->action_->server.load(std::memory_order_acquire) != nullptr && !na->action_->cancelled) {
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org