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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
