fix cancel/accept race in NetAccept that causes EBADF abort#12925
fix cancel/accept race in NetAccept that causes EBADF abort#12925JakeChampion wants to merge 2 commits into
Conversation
JakeChampion
force-pushed
the
jake/ccc
branch
3 times, most recently
from
27c9a8a to
b259ce1
Compare
`NetAcceptAction::cancel()` closes the server socket before setting the cancelled flag In the window between close and flag set, `net_accept()` can get `EBADF` from `accept4()`, see `cancelled=false`, and dispatch `EVENT_ERROR` to handlers that don't expect it now we check the atomic server pointer in all three accept paths before dispatching `EVENT_ERROR`
JakeChampion
force-pushed
the
jake/ccc
branch
from
b259ce1 to
ad25b64
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a race condition between NetAcceptAction::cancel() and the net_accept() accept paths that could cause an unhandled EBADF abort. The cancel operation atomically closes the server socket (via server.exchange(nullptr, ...)) before setting the cancelled flag, leaving a window where an accept call can fail with EBADF, see cancelled == false, and spuriously dispatch EVENT_ERROR to a continuation that does not expect it.
Changes:
- Adds an atomic
std::atomic<Server *> servercheck (action_->server.load(std::memory_order_acquire) != nullptr) as the primary guard before dispatchingEVENT_ERRORin all three accept code paths (net_accept,do_blocking_accept,acceptFastEvent). - The
NetAcceptAction::cancel()implementation (inP_NetAccept.h) usesserver.exchange(nullptr, std::memory_order_acq_rel)so the null pointer state serves as a reliable cancellation indicator beforecancelledis set.
|
[approve ci] |
|
I think this maybe got missed in our PR scrub somehow. I'm putting it on my list to review. |
|
@JakeChampion Have you considered the Copilot post? Was it intentional to leave the |
apologies, it was not intentional, i've pushed another commit now 👍 |
| goto Ldone; | ||
| } | ||
| if (na->server.sock.is_ok() && !na->action_->cancelled) { | ||
| if (na->action_->server.load(std::memory_order_acquire) != nullptr && !na->action_->cancelled) { |
There was a problem hiding this comment.
This makes some assumptions about invariants for action cancellation. Copilot's justification for its proposed solution is more than insufficient given the risk.
| [[fallthrough]]; | ||
| default: | ||
| if (!action_->cancelled) { | ||
| if (action_->server.load(std::memory_order_acquire) != nullptr && !action_->cancelled) { |
| goto Ldone; | ||
| } | ||
| if (!action_->cancelled) { | ||
| if (action_->server.load(std::memory_order_acquire) != nullptr && !action_->cancelled) { |
NetAcceptAction::cancel()closes the server socket before setting the cancelled flag In the window between close and flag set,net_accept()can getEBADFfromaccept4(), seecancelled=false, and dispatchEVENT_ERRORto handlers that don't expect itnow we check the atomic server pointer in all three accept paths before dispatching
EVENT_ERROR