fix cancel/accept race in NetAccept that causes EBADF abort - #12925
JakeChampion wants to merge 2 commits into
Conversation
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`
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 👍 |
JosiahWI
left a comment
JosiahWI
left a comment
There was a problem hiding this comment.
Please review Copilot's comments and resolve with a comment if they are incorrect.
|
@JakeChampion I see that you replied to Copilot with "applied". Were you intending to push changes? |
I did push but it looks like github is confused about it |
|
I'll open a new PR for this to see if that helps |
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