Huixxi commented on PR #3412:
URL: https://github.com/apache/brpc/pull/3412#issuecomment-5537179502
This fix is on the right track — it addresses the core race condition
described in issue #3410, but it's a targeted patch rather than the more
complete solution the issue actually suggests. Here's the breakdown:
**Is the fix correct?**
- The core problem in the issue: a `PollCq` callback belonging to an
"old-generation" CQ Socket may resume execution *after* `RdmaEndpoint::Reset()`
→ health check → `Revive()` → a fresh handshake (which allocates a new
`_resource` and a new `_cq_sid`). At that point, the `ep` (`RdmaEndpoint*`) it
holds is already "new generation," while `m` (the CQ Socket that triggered the
callback) is still "old generation."
- The new check added:
```cpp
if (m->id() != ep->_cq_sid) {
return;
}
```
This is exactly one of the key validations the issue suggested — "verify
that `m->id()` still equals `ep->_cq_sid`." Since `_cq_sid` is regenerated via
`Socket::Create` every time `AllocateResources()` runs (brpc's `SocketId`
carries a version number, so even if the fd slot is reused, the id differs),
the old-generation CQ Socket's id will necessarily not match the new-generation
`_cq_sid`. This correctly rejects the vast majority of stale callbacks and is a
reasonable, low-cost fix (one integer comparison).
- The added test `stale_cq_callback_does_not_poll_new_generation` directly
constructs the scenario "old CQ socket + mismatched new `_cq_sid`" and verifies
`PollCq` returns early, covering the main scenario described in the issue.
**What isn't fully addressed (the issue itself hints at a more complete
solution):**
1. **A TOCTOU window still exists**: the check happens once at the start of
`PollCq`, but the function later continues to access `ep->_resource` (e.g.,
`ep->_resource->recv_cq`). If `Reset()` happens right *after* this check passes
but *before* `ep->_resource` is dereferenced (setting `_resource` to `NULL`), a
crash could still occur. Ideally, per the issue's suggestion, the callback
should be bound to immutable context such as `{main_socket_id, cq_socket_id,
resource, generation}`, and re-validated right before actually touching
`_resource` — not just once at function entry.
2. **No explicit "generation" field was introduced**: the issue text
explicitly notes that PR #3261 considered adding a generation counter in
`Reset()` and validating it in `PollCq`, but the merged implementation chose CQ
draining only. This fix similarly just adds a `_cq_sid` comparison rather than
an explicit generation. In practice, `_cq_sid`'s regeneration mechanism likely
serves as an effective equivalent to a generation counter (since `SocketId`
carries a version), but semantically it's less explicit than a dedicated
generation field, and it wouldn't cover edge cases like "the same CQ Socket
being reused by later logic" (though the current code doesn't exhibit that
pattern).
3. **Only the shared entry point in `PollCq`** (used by both polling and
non-polling modes) gets this check; `HandleCompletion` itself still has no
secondary validation. If any future call path were to invoke `HandleCompletion`
directly, bypassing `PollCq`, the same issue could resurface (currently no such
path exists, so this is a latent maintenance risk rather than an active bug).
**Conclusion:**
This PR's change is correct in direction and does fix the specific crash
scenario described in issue #3410 (a stale callback incorrectly accessing
new-generation resources after socket revival). The change is small, the
performance overhead is negligible, and the test specifically validates the fix
point. However, it is a "minimally invasive" fix rather than the "complete
solution" envisioned in the issue (e.g., finer-grained generation validation,
or re-checking right before dereferencing `ep->_resource`). If similar TOCTOU
scenarios surface later, further hardening may be needed (e.g., re-checking
`ep->_cq_sid` immediately before actually dereferencing `ep->_resource`, or
introducing an atomic generation field). I'd suggest flagging this in the
review comments, but the fix can reasonably be merged as an incremental
improvement in the meantime.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
