altman08 opened a new pull request, #3600:
URL: https://github.com/apache/brpc/pull/3600
````markdown
### What problem does this PR solve?
Issue Number: N/A
Problem Summary:
Late SelectiveChannel subcall callbacks can continue processing results
after the main RPC has finished, as long as other subcalls remain outstanding.
This can modify the completed RPC result and, in the older version identified
by the core dump, trigger a retry that dereferences a cleared load balancer.
The issue was identified from `core-mtable-stablete-270416-1790555285`,
generated by `mtable-stabletest` while calling
`mtable.tableserver.Sdk.ReadTable`.
The main RPC used a `1000ms` timeout, a `996ms` backup request delay, and a
maximum of three retries. Its custom `MTableRetryPolicy` allowed retries for
error `5013` (`PARTITION_NOT_READY`).
The failure sequence was:
1. The request reached the backup request timeout, resulting in two
allocated subcall resources. A subcall returned E5013 and triggered a retry,
still subject to the main RPC’s original `1000ms` deadline.
2. The main RPC timed out with `ERPCTIMEDOUT` (`1008`). In the affected
older version, `Controller::EndRPC()` cleared the main controller’s `_lb`
before invoking `Sender::Run()`.
3. `Sender::Run()` set `_finished=true`. Since subcalls were still
outstanding, it released the main call-id lock, signaled cancellation to the
subcalls, and waited for their resources to be returned.
4. A late subcall callback carrying E5013 acquired the main call-id lock.
Its `saved_error` was `1008`, confirming that the main RPC had already timed
out.
5. `PushFree()` returned the subcall resource, leaving `_nfree=1` and
`_nalloc=2`. The previous implementation returned `false` only when `_finished
&& _nfree == _nalloc`. Consequently, this callback was allowed to continue
processing its result even though `_finished` was already `true`.
6. `SubDone::Run()` wrote E5013 into the main controller and called
`OnVersionedRPCReturned()`. The subcall’s correlation ID matched the current
call, so the stale-call filtering did not reject it.
7. `MTableRetryPolicy::DoRetry()` accepted E5013. The retry count increased
from two to three, and execution entered `Sender::IssueRPC()`.
8. In the affected older version, `Sender::IssueRPC()` accessed the main
controller’s already-cleared `_lb`. The faulting instruction was `mov
0x18(%rbx), %rdi`, with `RBX=0`, causing a SIGSEGV at address `0x18`.
The core contained the following state:
| Field | Value |
| --- | --- |
| Sender `_finished` | `true` |
| Sender `_nfree / _nalloc` | `1 / 2` |
| Main controller `_lb` | `nullptr` |
| Main error saved by the subcall callback | `1008` (`ERPCTIMEDOUT`) |
| Late subcall error | `5013` (`PARTITION_NOT_READY`) |
| Retry count at the crash | `3` |
The main controller’s error text also recorded E5013 after the timeout:
```text
[E1007]Reached backup timeout=996ms
[R1][E5013][10.233.187.5:8000][E5013]failed
[R2][E1008]Reached timeout=1000ms
[R2][E5013][10.233.187.5:8000][E5013]failed
```
The retry start timestamp was `3μs` after the main RPC’s end timestamp and
`75μs` after its deadline. These timestamps support the failure sequence,
although they do not constitute a complete thread execution trace. The main
controller’s `_error_code=0` at the crash resulted from clearing the error for
the new retry; it did not indicate a successful RPC.
The current repository already has an ending-state guard and a separate load
balancer reference in Sender, which protect against the same retry/crash path.
However, late callbacks can still modify the main controller’s error text,
response, and attachment before reaching that guard. This PR fixes the
underlying callback handling issue.
### What is changed and the side effects?
Changed:
- Make `Sender::PushFree()` stop result processing whenever `_finished` is
`true`, regardless of whether all subcall resources have been returned.
- Continue returning resources from late callbacks. Release the main call-id
lock when callbacks remain outstanding, and let the final callback execute
`Clear()`.
- Return immediately after unlocking or cleanup, since either operation may
allow the Sender, SubDone, or main controller to be destroyed.
- Preserve normal subcall error propagation and retry behavior while the
main RPC remains active.
- Add three deterministic regression tests covering:
- Late callbacks after timeout or cancellation, with retryable errors,
non-retryable errors, or successful results.
- Both callback orders and controller destruction inside the user
completion callback.
- Retryable and non-retryable subcall errors before the main RPC finishes.
Validation:
- All **87 tests** in `brpc_channel_unittest` passed.
- The late-callback regression test failed against the original
implementation, confirming that it detects the existing issue.
- The before-completion error propagation and retry tests passed with both
implementations.
Side effects:
- Performance effects: No significant impact expected. Late callbacks skip
unnecessary result processing.
- Breaking backward compatibility: None. No public API changes; active RPC
error propagation and retry behavior are preserved.
---
### Check List:
- [x] Please make sure your changes are compilable.
- [x] When providing us with a new feature, it is best to add related tests.
- [x] Please follow [Contributor Covenant Code of
Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
````
--
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]
