Copilot commented on code in PR #3509:
URL: https://github.com/apache/brpc/pull/3509#discussion_r4119730954
##########
src/brpc/ubshm/ub_ring_manager.cpp:
##########
@@ -68,57 +77,123 @@ RETURN_CODE UBRingManager::UbrMgrInit() {
g_ubr_mgr.trx_mgr = (UbrTrx *)malloc(trx_mgr_size);
size_t trx_mgr_status_size = g_ubr_mgr.trx_cap * sizeof(UbrMgrUnitStatus);
g_ubr_mgr.trx_mgr_unit_status = (UbrMgrUnitStatus
*)malloc(trx_mgr_status_size);
- if (UNLIKELY(g_ubr_mgr.trx_mgr == nullptr ||
- g_ubr_mgr.trx_mgr_unit_status == nullptr)) {
+ size_t trx_mgr_id_size = g_ubr_mgr.trx_cap * sizeof(uint64_t);
+ g_ubr_mgr.trx_mgr_unit_id = (uint64_t *)malloc(trx_mgr_id_size);
+ size_t trx_mgr_ctl_size = g_ubr_mgr.trx_cap * sizeof(UbrCleanupCtl *);
+ g_ubr_mgr.trx_mgr_unit_ctl = (UbrCleanupCtl **)malloc(trx_mgr_ctl_size);
+ if (BAIDU_UNLIKELY(g_ubr_mgr.trx_mgr == nullptr ||
+ g_ubr_mgr.trx_mgr_unit_status == nullptr ||
+ g_ubr_mgr.trx_mgr_unit_id == nullptr ||
+ g_ubr_mgr.trx_mgr_unit_ctl == nullptr)) {
LOG(ERROR) << "Ubr manager memory allocation failed.";
UbrMgrFini();
return UBRING_ERR;
}
memset(g_ubr_mgr.trx_mgr, 0, trx_mgr_size);
memset(g_ubr_mgr.trx_mgr_unit_status, UBR_MGR_UNIT_FREE,
trx_mgr_status_size);
+ memset(g_ubr_mgr.trx_mgr_unit_id, 0, trx_mgr_id_size);
+ memset(g_ubr_mgr.trx_mgr_unit_ctl, 0, trx_mgr_ctl_size);
LinkInfoInit();
return UBRING_OK;
}
void UBRingManager::UbrMgrFini() {
+ // Cancel the pending delayed cleanups and wait for the in-flight ones
+ // (each holds one extra reference) to finish, before the pool memory
+ // they touch is freed. A ctl whose timer is still starting can only be
+ // cancelled in a later round, hence the retry-to-stability loop.
+ bool busy = true;
+ while (busy) {
+ busy = false;
+ {
+ BAIDU_SCOPED_LOCK(g_ubr_trx_mgr_mtx);
Review Comment:
These manager locks are declared as `pthread_mutex_t`, but
`BAIDU_SCOPED_LOCK` expands to `std::lock_guard` and therefore requires
`.lock()`/`.unlock()` methods; this does not compile. Keep the existing
pthread-specific `LOCK_GUARD(&g_ubr_trx_mgr_mtx)` (and apply the same
correction to the other manager mutex occurrences), or convert the mutex types
consistently.
##########
src/brpc/ubshm/ub_ring_manager.cpp:
##########
@@ -68,57 +77,123 @@ RETURN_CODE UBRingManager::UbrMgrInit() {
g_ubr_mgr.trx_mgr = (UbrTrx *)malloc(trx_mgr_size);
size_t trx_mgr_status_size = g_ubr_mgr.trx_cap * sizeof(UbrMgrUnitStatus);
g_ubr_mgr.trx_mgr_unit_status = (UbrMgrUnitStatus
*)malloc(trx_mgr_status_size);
- if (UNLIKELY(g_ubr_mgr.trx_mgr == nullptr ||
- g_ubr_mgr.trx_mgr_unit_status == nullptr)) {
+ size_t trx_mgr_id_size = g_ubr_mgr.trx_cap * sizeof(uint64_t);
+ g_ubr_mgr.trx_mgr_unit_id = (uint64_t *)malloc(trx_mgr_id_size);
+ size_t trx_mgr_ctl_size = g_ubr_mgr.trx_cap * sizeof(UbrCleanupCtl *);
+ g_ubr_mgr.trx_mgr_unit_ctl = (UbrCleanupCtl **)malloc(trx_mgr_ctl_size);
+ if (BAIDU_UNLIKELY(g_ubr_mgr.trx_mgr == nullptr ||
+ g_ubr_mgr.trx_mgr_unit_status == nullptr ||
+ g_ubr_mgr.trx_mgr_unit_id == nullptr ||
+ g_ubr_mgr.trx_mgr_unit_ctl == nullptr)) {
LOG(ERROR) << "Ubr manager memory allocation failed.";
UbrMgrFini();
return UBRING_ERR;
}
memset(g_ubr_mgr.trx_mgr, 0, trx_mgr_size);
memset(g_ubr_mgr.trx_mgr_unit_status, UBR_MGR_UNIT_FREE,
trx_mgr_status_size);
+ memset(g_ubr_mgr.trx_mgr_unit_id, 0, trx_mgr_id_size);
+ memset(g_ubr_mgr.trx_mgr_unit_ctl, 0, trx_mgr_ctl_size);
LinkInfoInit();
return UBRING_OK;
}
void UBRingManager::UbrMgrFini() {
+ // Cancel the pending delayed cleanups and wait for the in-flight ones
+ // (each holds one extra reference) to finish, before the pool memory
+ // they touch is freed. A ctl whose timer is still starting can only be
+ // cancelled in a later round, hence the retry-to-stability loop.
+ bool busy = true;
+ while (busy) {
+ busy = false;
+ {
+ BAIDU_SCOPED_LOCK(g_ubr_trx_mgr_mtx);
+ if (g_ubr_mgr.trx_mgr_unit_ctl != nullptr) {
+ for (uint32_t i = 0; i < g_ubr_mgr.trx_cap; ++i) {
+ UbrCleanupCtl* ctl = g_ubr_mgr.trx_mgr_unit_ctl[i];
+ if (ctl == nullptr) {
+ continue;
+ }
+ if (UbrTimerDel(&ctl->timer) == 0) {
+ ctl->ReleaseRef(); // timer/callback reference
+ }
Review Comment:
`UbrTimerDel` returns 0 when it wins the slot, even if the underlying
`bthread_timer_del` returns 1 because the callback was already dispatched. In
that case the callback still owns the timer-side cleanup reference and will
release it; releasing `ctl` here as though the timer was canceled can free the
control object before `UbrPassiveClearCallback`/`UbrAsynClearCallback` returns,
causing a use-after-free or a later refcount underflow. The caller needs a
cancellation result that distinguishes `bthread_timer_del == 0` from an
already-dispatched callback, rather than using `UbrTimerDel`'s slot-ownership
result.
##########
src/brpc/ubshm/shm/shm_ubs.cpp:
##########
@@ -413,55 +414,58 @@ static void DeleteShmToList(ShmList* shm_list)
void *UbsShmCallback(void* args)
{
ShmList *shm_list = (ShmList*)args;
- if (UNLIKELY(shm_list == nullptr)) {
+ if (BAIDU_UNLIKELY(shm_list == nullptr)) {
LOG(ERROR) << "Shm list is null.";
return nullptr;
}
- LOCK_GUARD(shm_list->shm_lock);
- while (shm_list->head != nullptr) {
- SHM shm = shm_list->head->shm;
- if (shm.addr == nullptr) {
- LOG(ERROR) << "Ubs input shm param is invalid, addr is NULL.";
+ // Drain one node per fire and keep the SDK calls outside the lock, so
+ // a slow daemon cannot stall the timer thread for the whole list.
+ SHM shm;
+ {
+ BAIDU_SCOPED_LOCK(shm_list->shm_lock);
Review Comment:
`ShmList::shm_lock` is a `pthread_mutex_t`, so `BAIDU_SCOPED_LOCK` cannot be
instantiated for it for the same reason as the manager locks: it uses
`std::lock_guard` rather than the repository's pthread-mutex guard. This
prevents the changed file from compiling; use the pthread-specific guard for
every `shm_lock` occurrence.
This issue also appears on line 439 of the same file.
##########
src/brpc/ubshm/ub_ring.cpp:
##########
@@ -47,21 +56,130 @@ UBRing::~UBRing()
RETURN_CODE UBRing::UbrTrxMapShm(SHM *local_shm, SHM *remote_shm)
{
RETURN_CODE rc = UbrTrxMapLocalShm(local_shm);
- if (UNLIKELY(rc != UBRING_OK)) {
+ if (BAIDU_UNLIKELY(rc != UBRING_OK)) {
LOG(ERROR) << "Trx map local shared memory failed.";
return rc;
}
rc = UbrTrxMapRemoteShm(remote_shm);
- if (UNLIKELY(rc != UBRING_OK)) {
+ if (BAIDU_UNLIKELY(rc != UBRING_OK)) {
LOG(ERROR) << "Trx map remote shared memory failed.";
return rc;
}
return UBRING_OK;
}
+static void UbrDoAsynClearWork(UbrTrx *trx, uint64_t expect_ubr_id) {
+ if (BAIDU_UNLIKELY(UBRing::UbrTrxFreeShm(trx) != UBRING_OK)) {
+ LOG(ERROR) << "Trx close, wait for local shm " << trx->local_shm.name
<< " free fail.";
+ }
+ if (BAIDU_UNLIKELY(UBRingManager::ReleaseUbrTrxFromMgr(trx, expect_ubr_id)
!= UBRING_OK)) {
+ LOG(ERROR) << "Trx close, release shm " << trx->local_shm.name << "
trx failed.";
+ }
+}
+
+static void UbrDoPassiveClearWork(UbrTrx *trx, uint64_t expect_ubr_id) {
+ int rc = ShmLocalFree(&trx->remote_shm);
+ if (rc != UBRING_OK) {
+ LOG(ERROR) << "Trx passive clear, delete remote shm " <<
trx->remote_shm.name
+ << " failed. ret=" << rc;
+ }
+ rc = ShmLocalFree(&trx->local_shm);
+ if (rc != UBRING_OK) {
+ LOG(ERROR) << "Trx passive clear, delete local shm " <<
trx->local_shm.name
+ << " failed. ret=" << rc;
+ }
+ if (BAIDU_UNLIKELY(UBRingManager::ReleaseUbrTrxFromMgr(trx, expect_ubr_id)
!= UBRING_OK)) {
+ LOG(ERROR) << "Trx passive clear, release shm " << trx->local_shm.name
<< " trx failed.";
+ }
+}
+
+// Schedule the delayed cleanup of `trx'. The cleanup ownership lives in the
+// per-acquisition control object, so exactly one of the delayed-clear
+// callback and a force close ever runs the cleanup. `work' is the cleanup
+// body, used directly when the timer cannot be started.
+static RETURN_CODE UbrScheduleClearTimer(UbrTrx *trx, void* (*cb)(void*),
+ void (*work)(UbrTrx*, uint64_t)) {
+ if (BAIDU_UNLIKELY(trx == nullptr || trx->local_shm.addr == nullptr)) {
+ return UBRING_OK; // released trx, stale event
+ }
+ if (trx->cleanup_ctl.load() != nullptr) {
+ return UBRING_OK; // cleanup already scheduled
Review Comment:
This stale-event guard does not validate the generation that the callback
was created for. After an old timer callback outlives release and the pool slot
is reused, `local_shm.addr` is non-null and `cleanup_ctl` is empty for the new
occupant, so this function captures the new `ubr_id` and schedules cleanup
against the new transaction. Pass the expected generation through the timer
callback path and reject it before changing state or scheduling cleanup.
This issue also appears in the following locations of the same file:
- line 126
- line 165
- line 215
- line 231
- line 491
--
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]
