From 6e05dc1523bf4257f810ad593b5242cd934eb937 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 30 Jun 2026 14:24:47 +0000 Subject: [PATCH] =?UTF-8?q?fix(libxce):=20endpoint=20reference=20count=20?= =?UTF-8?q?=E2=80=94=20kill=20teardown-vs-dispatch=20UAF=20(bug-1298,=20ta?= =?UTF-8?q?sk=205.8)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The io_worker freed an endpoint (endpoint_killer, cluster.c) on connection teardown while the vtask dispatcher concurrently read ep->busy (endpoint_is_task_blocked, netio.c) on a queued vtask — a heap-use-after-free that ABORTed a controller mid-chaotic-soak (round 4), dropping the cluster below quorum so no primary could be elected. Replace the accumulation of ad-hoc ep-lifetime patches (bug-1282 lockless busy, bug-1197 dispatch_inflight drain + deferred teardown vtask) with one principled invariant: an endpoint lives until its reference count reaches 0. - endpoint_t.refcnt (_Atomic) + heap_alloc flag; endpoint_get starts a heap ep at refcnt=1 (the owner ref). - endpoint_ref/endpoint_unref: the last unref fini's + frees a heap ep. Embedded eps (client ctx->ep, heap_alloc=0) are never freed here — their container owns them — so a vtask-balanced ref/unref pair is a safe no-op. - vtask_enq takes a per-vtask ref on the bound ep BEFORE publishing the task to the dispatcher; run_task / taskq_item_killer release it. So the dispatcher can read ep->busy on any queued vtask without the ep being freed underneath it. - every HEAP-ep teardown site (endpoint_killer, handle_endpoint_error, the vote/prevote connect-fail paths, the vote-ep loser/teardown paths, the standby reconnect/eviction paths) now drops its owner ref via endpoint_unref instead of raw endpoint_fini+free. Several of those also leaked the ep struct before (endpoint_fini-only); unref fixes the leak too. Validation: controller_unit_test 182 tests / 0 failures (test_ha_vep_teardown updated — its OFF-path test asserted the old struct leak, now fixed). 6-round ASAN chaotic soak: ZERO AddressSanitizer reports, ctrl-3 no longer crashes at round 4, all 6 rounds elect at bounded terms. (The pre-existing iomgr_fini -> SSL_CTX_free abort under openssl 3.6 in the aggregate test binary is unrelated.) Note: with the crash gone, the soak now reaches rounds 4-6 for the first time and surfaces a SEPARATE, pre-existing committed-job-loss/divergence (the bug-795/828 failover catch-up gap; ha-commit-durability domain) — not a refcount regression (ASAN clean; rounds 1-3 are loss-free). --- src/controller/cluster.c | 20 +++--- src/controller/ha.c | 71 ++++++++----------- .../testsuite/test_ha_vep_teardown.c | 17 ++--- src/include/netio.h | 19 +++++ src/lib/netio.c | 36 ++++++++++ src/lib/vtaskmgr.c | 32 ++++++++- 6 files changed, 137 insertions(+), 58 deletions(-) diff --git a/src/controller/cluster.c b/src/controller/cluster.c index 54d3fd5a..c668b048 100644 --- a/src/controller/cluster.c +++ b/src/controller/cluster.c @@ -104,13 +104,14 @@ static void handle_endpoint_error(endpoint_t* ep) } if (ep->idx >= 0) { - /* zarray_del will finalize ep */ + /* zarray_del finalizes ep via endpoint_killer (now endpoint_unref). */ zarray_del(&(c->endpoints), ep->idx); return; } - endpoint_fini(ep); - FREEUP(ep); + /* bug-1298: not in c->endpoints — drop the owner ref (deferred fini+free if a + * pool vtask still references it) instead of freeing inline on the io_worker. */ + endpoint_unref(ep); return; } @@ -757,8 +758,7 @@ static int handle_connection(struct epoll_event* p) if (priv == NULL) { PERROR("ep_get_private"); - endpoint_fini(ep2); - FREEUP(ep2); + endpoint_unref(ep2); /* bug-1298: refcnt 1->0, fini+free (no queued vtasks yet) */ return -ENOMEM; } @@ -774,8 +774,7 @@ static int handle_connection(struct epoll_event* p) PERROR("zarray_store"); FREEUP(priv); - endpoint_fini(ep2); - FREEUP(ep2); + endpoint_unref(ep2); /* bug-1298: refcnt 1->0, fini+free (not yet queued) */ return idx; } @@ -809,8 +808,11 @@ static void endpoint_killer(void* p) zarray_clear(&(c->endpoints), ep->idx); ep->idx = -1; } - endpoint_fini(ep); - FREEUP(ep); + /* bug-1298: drop the creator/owner reference instead of freeing inline. If + * a pool vtask still references this ep (the io_worker error path runs while + * the dispatcher may be reading ep->busy), the fini+free is deferred to the + * last endpoint_unref — never racing the dispatcher. */ + endpoint_unref(ep); return; } diff --git a/src/controller/ha.c b/src/controller/ha.c index 95b2bd8c..659d20d7 100644 --- a/src/controller/ha.c +++ b/src/controller/ha.c @@ -221,7 +221,7 @@ static int raft_cb_send_vote_request(void *ctx, int peer_idx, uint64_t term) int rc = endpoint_connect(ep, peer->hostname, peer->port, DEFAULT_CONNECT_TIMEOUT_MS); if (rc < 0) { - endpoint_fini(ep); + endpoint_unref(ep); /* bug-1298: refcnt 1->0 → fini+free (not monitored) */ if (!c) handle_request_vote_reply(ha, ectx, votes_idx, term, 0, 0, 0); return 1; @@ -229,7 +229,7 @@ static int raft_cb_send_vote_request(void *ctx, int peer_idx, uint64_t term) rc = iomgr_monitor(ep); if (rc < 0) { - endpoint_fini(ep); /* frees ep->data (vctx) via FREEUP */ + endpoint_unref(ep); /* bug-1298: fini frees ep->data (vctx), then frees ep */ handle_request_vote_reply(ha, ectx, votes_idx, term, 0, 0, 0); return 1; } @@ -332,10 +332,10 @@ static int raft_cb_send_pre_vote_request(void *ctx, int peer_idx, int rc = endpoint_connect(ep, peer->hostname, peer->port, DEFAULT_CONNECT_TIMEOUT_MS); - if (rc < 0) { endpoint_fini(ep); return 1; } + if (rc < 0) { endpoint_unref(ep); return 1; } /* bug-1298: fini+free */ rc = iomgr_monitor(ep); - if (rc < 0) { endpoint_fini(ep); return 1; } + if (rc < 0) { endpoint_unref(ep); return 1; } /* bug-1298: fini+free */ return 0; } @@ -1337,10 +1337,10 @@ static void raft_cb_become_follower(void *ctx, uint64_t term, int leader_idx) * last live use, and gate late dispatch (tearing_down). */ vote_ep_teardown_async(ha, ep); } else { - /* Flag OFF / pool unwired: UNCHANGED legacy behavior — - * endpoint_fini only, no free(ep) (the struct is leaked per - * lost election, exactly as before this commit). */ - endpoint_fini(ep); + /* bug-1298: drop the owner ref (deferred fini+free if a pool + * vtask still references this vote-ep). Also fixes the old + * flag-OFF struct leak (was endpoint_fini-only). */ + endpoint_unref(ep); } ectx->votes[i].ep = NULL; } @@ -5451,18 +5451,17 @@ static int vote_ep_teardown_vtask_entry(vtask_t *task) io_task_data_t *iod = &task->data->u.iodata; endpoint_t *ep = iod->ep; - /* Disarm run_task's post-body endpoint_unblock_tasks: it would touch - * ep->lock AFTER we free ep below. NULLing the task's ep makes run_task - * skip the unblock (run_task: `if (ep != NULL) endpoint_unblock_tasks`). */ - iod->ep = NULL; - if (ep != NULL) { #ifdef XCE_TESTING if (ha_vep_teardown_probe) ha_vep_teardown_probe(ep); #endif - endpoint_fini(ep); - free(ep); + /* bug-1298: drop the creator/owner reference. We DON'T fini/free here + * and DON'T NULL iod->ep: this vtask still holds the per-vtask reference + * (taken in vtask_enq), so the ep stays alive through run_task's post-body + * endpoint_unblock_tasks; run_task then drops the per-vtask ref, and that + * last endpoint_unref performs the fini+free. No UAF, no NULL hack. */ + endpoint_unref(ep); } return 0; } @@ -5484,8 +5483,7 @@ static void vote_ep_teardown_enqueue(ha_state_t *ha, endpoint_t *ep) (void *)pool, (void *)data, (void *)task); free(data); free(task); - endpoint_fini(ep); - free(ep); + endpoint_unref(ep); /* bug-1298: no vtask enqueued — drop owner ref → free */ return; } @@ -5501,8 +5499,9 @@ static void vote_ep_teardown_enqueue(ha_state_t *ha, endpoint_t *ep) "(stay alive)"); free(data); free(task); - endpoint_fini(ep); - free(ep); + /* bug-1298: vtask_enq already dropped the per-vtask ref it took on the + * failed enqueue, so refcnt is back to the owner ref — drop it → free. */ + endpoint_unref(ep); } } @@ -7340,11 +7339,11 @@ static void on_standby_connected(endpoint_t *ep) if (stale_same_peer->iomgr) iomgr_unmonitor((iomgr_t *)stale_same_peer->iomgr, stale_same_peer); (void)standby_ep_err_handler_primary(stale_same_peer); - /* err-handler set ep->data = NULL and freed sctx; endpoint_fini then - * reclaims outq/ssl/packets, and we free the calloc'd endpoint itself - * (endpoint_get uses calloc; there is no endpoint_put). */ - endpoint_fini(stale_same_peer); - free(stale_same_peer); + /* err-handler set ep->data = NULL and freed sctx; bug-1298: drop the + * owner ref — endpoint_unref reclaims outq/ssl/packets and frees the + * calloc'd struct when the last reference goes (deferred if a pool vtask + * still references this just-unmonitored ep). */ + endpoint_unref(stale_same_peer); #ifdef XCE_TESTING atomic_fetch_add(&ha_standby_evicted_count_for_test, 1); #endif @@ -7506,8 +7505,7 @@ static reconnect_ctx_t *ha_connect_prep_peer(cluster_t *c, int peer_idx) reconnect_ctx_t *rc_ctx = calloc(1, sizeof(*rc_ctx)); if (!rc_ctx) { ep->data = NULL; - endpoint_fini(ep); - free(ep); /* endpoint_get() calloc'd it; fini frees inner only */ + endpoint_unref(ep); /* bug-1298: refcnt 1->0 → fini+free (not monitored) */ free(sctx); return NULL; } @@ -7561,9 +7559,7 @@ static int ha_connect_finish_peer(reconnect_ctx_t *rc_ctx) * Reverting this to free only on failure re-introduces the 2.1 * GREEN-test endpoint leak masked by detect_leaks=0. */ ep->data = NULL; - endpoint_fini(ep); - free(ep); /* endpoint_get() calloc'd ep; fini frees its inner - * members only, not the struct itself */ + endpoint_unref(ep); /* bug-1298: refcnt 1->0 → fini+free (test path) */ free(sctx); return hrc; } @@ -7580,20 +7576,17 @@ static int ha_connect_finish_peer(reconnect_ctx_t *rc_ctx) if (rc_ctx->should_log) WARN("HA: connect to peer[%d] (%s:%u) failed: %d", peer_idx, peer->hostname, peer->port, rc); - /* endpoint_fini calls FREEUP(ep->data) which frees sctx — do not - * double-free sctx here. endpoint_get() calloc'd ep itself, so free - * the struct after fini (fini reclaims only its inner members). */ + /* bug-1298: ep->data cleared (sctx freed separately below) then drop the + * owner ref — endpoint_unref does fini+free of the calloc'd struct. */ ep->data = NULL; - endpoint_fini(ep); - free(ep); + endpoint_unref(ep); free(sctx); return rc; } rc = iomgr_monitor(ep); /* lock-guarded epoll registration (Req 4.4) */ if (rc < 0) { ep->data = NULL; - endpoint_fini(ep); - free(ep); + endpoint_unref(ep); /* bug-1298: fini+free (deferred if a vtask refs it) */ free(sctx); return rc; } @@ -7688,8 +7681,7 @@ static void ha_connect_to_peer(cluster_t *c, int peer_idx) * set — otherwise no task owns its clear and dedup wedges permanently. * Tear down our duplicate prep; the next tick retries cleanly. */ rc_ctx->ep->data = NULL; - endpoint_fini(rc_ctx->ep); - free(rc_ctx->ep); /* endpoint_get() calloc'd it; fini frees inner only */ + endpoint_unref(rc_ctx->ep); /* bug-1298: fini+free the calloc'd struct */ free(rc_ctx->sctx); free(rc_ctx); atomic_store_explicit(&ha->reconnect_inflight[peer_idx], 0, @@ -7704,8 +7696,7 @@ static void ha_connect_to_peer(cluster_t *c, int peer_idx) WARN("HA: slowlane_submit reconnect peer[%d] failed: %d — retry next tick", peer_idx, srv); rc_ctx->ep->data = NULL; - endpoint_fini(rc_ctx->ep); - free(rc_ctx->ep); /* endpoint_get() calloc'd it; fini frees inner only */ + endpoint_unref(rc_ctx->ep); /* bug-1298: fini+free the calloc'd struct */ free(rc_ctx->sctx); free(rc_ctx); atomic_store_explicit(&ha->reconnect_inflight[peer_idx], 0, diff --git a/src/controller/testsuite/test_ha_vep_teardown.c b/src/controller/testsuite/test_ha_vep_teardown.c index 8d288b10..874e6ebf 100644 --- a/src/controller/testsuite/test_ha_vep_teardown.c +++ b/src/controller/testsuite/test_ha_vep_teardown.c @@ -320,10 +320,10 @@ static void test_vep_late_event_dropped(void) ha_peerctrl_body_probe = NULL; ha_set_peerctrl_dispatch_for_test(0); - /* ep was not torn down by a vtask here — free it ourselves (pkt was freed - * by the drop path). */ - endpoint_fini(ep); - free(ep); + /* ep was not torn down by a vtask here (late event dropped, refcnt==1) — + * drop the owner ref ourselves; endpoint_unref fini's + frees it (pkt was + * freed by the drop path). bug-1298: use unref, not raw fini+free. */ + endpoint_unref(ep); vtaskmgr_fini(&pool); iomgr_fini(&iomgr); @@ -402,11 +402,12 @@ static void test_vep_off_path_uses_plain_fini(void) TEST_ASSERT_EQUAL_INT_MESSAGE(0, atomic_load(&g_teardown_done), "OFF path must NOT run the async teardown vtask"); TEST_ASSERT_NULL_MESSAGE(ectx->votes[0].ep, - "loser loop must clear votes[i].ep after endpoint_fini"); + "loser loop must clear votes[i].ep after endpoint_unref"); - /* OFF path called endpoint_fini(ep) (freed ep->data) but NOT free(ep) — the - * legacy per-lost-election leak. Free the struct here so the test is clean. */ - free(ep); + /* bug-1298: the OFF path now drops the owner ref via endpoint_unref — with no + * pool vtask referencing the ep (refcnt==1) that fini's AND frees it here, + * fixing the legacy per-lost-election struct leak. So the test must NOT + * free(ep) again (that was the old leak-cleanup; it is now a double-free). */ atomic_store_explicit(&ha.elect_ctx, NULL, memory_order_release); free(ectx); diff --git a/src/include/netio.h b/src/include/netio.h index ee0dbc99..497fb873 100644 --- a/src/include/netio.h +++ b/src/include/netio.h @@ -101,6 +101,18 @@ typedef struct endpoint_t { time_t ts_last_in; time_t ts_last_out; void (*on_connected)(struct endpoint_t *ep); /* fire-once; called in iomgr thread after ssl_handshake_done=1 */ + /* bug-1298: reference count governing the lifetime of a HEAP-allocated + * endpoint (endpoint_get). An ep is fini'd + freed exactly when refcnt + * drops to 0. Holders: the creator/owner (endpoint_get sets refcnt=1, the + * teardown path drops it via endpoint_unref) and EVERY ep-bound VTASK_IO + * vtask (vtask_enq +1, run_task / taskq_item_killer -1) — so the vtask + * dispatcher can read ep->busy (endpoint_is_task_blocked) on a queued vtask + * without the io_worker freeing the ep out from under it. Embedded eps + * (struct members like client ctx->ep — heap_alloc==0) participate in the + * count but are NEVER fini'd/freed by endpoint_unref; their owner manages + * their lifetime directly. */ + _Atomic int refcnt; + int heap_alloc; /* 1 iff allocated by endpoint_get (unref may free) */ } endpoint_t; #define INVOKE_MSG_HANDLER(ep) \ @@ -155,6 +167,13 @@ extern int endpoint_set_msg_handler(endpoint_t*, msg_handler_t); extern int endpoint_set_err_handler(endpoint_t*, err_handler_t); extern int endpoint_set_on_connected(endpoint_t*, void (*cb)(endpoint_t *)); extern void endpoint_fini(endpoint_t*); +/* bug-1298: endpoint refcount. endpoint_ref takes a reference; endpoint_unref + * drops one and, when the last reference is released, fini's the ep and (for + * heap_alloc eps) frees it. Use endpoint_unref in place of endpoint_fini+free + * on every HEAP-ep teardown path so a free never races a vtask still reading + * ep->busy. Safe on NULL. */ +extern void endpoint_ref(endpoint_t*); +extern void endpoint_unref(endpoint_t*); extern int endpoint_set_peer(endpoint_t*, xce_peerinfo_t*); extern int io_fsm_start(io_fsm_t*, io_packet_t*); diff --git a/src/lib/netio.c b/src/lib/netio.c index 54aa1f1d..df44fcda 100644 --- a/src/lib/netio.c +++ b/src/lib/netio.c @@ -754,9 +754,45 @@ endpoint_t* endpoint_get() do_endpoint_init(ep); + /* bug-1298: a heap ep starts with the single creator/owner reference. Set + * AFTER do_endpoint_init (which never touches refcnt/heap_alloc, so a later + * endpoint_init() re-init on this same heap ep preserves them). */ + ep->heap_alloc = 1; + atomic_init(&ep->refcnt, 1); + return ep; } +/* bug-1298: take/drop an endpoint reference. The last drop fini's the ep and, + * for a heap ep, frees it. Embedded eps (heap_alloc==0) are never fini'd/freed + * here — their owner manages the struct — so the count is purely advisory for + * them. Both are NULL-safe and lockless (the count is _Atomic). */ +void endpoint_ref(endpoint_t* ep) +{ + if (ep == NULL) { + return; + } + atomic_fetch_add_explicit(&ep->refcnt, 1, memory_order_relaxed); +} + +void endpoint_unref(endpoint_t* ep) +{ + if (ep == NULL) { + return; + } + + /* acq_rel: the decrement that observes the last reference must happen-after + * every other holder's release so the fini/free sees no concurrent access. + * fini+free ONLY for heap eps: an embedded ep (heap_alloc==0) is owned by its + * container (e.g. client ctx->ep), which fini's it directly — a vtask-balanced + * ref/unref pair on it must NOT fini or free the live struct. */ + if (atomic_fetch_sub_explicit(&ep->refcnt, 1, memory_order_acq_rel) == 1 && + ep->heap_alloc) { + endpoint_fini(ep); + free(ep); + } +} + int endpoint_init(endpoint_t* ep, int domain, int type, int port, int backlog) { int fd = -1; diff --git a/src/lib/vtaskmgr.c b/src/lib/vtaskmgr.c index 7c1f1ce1..40ddcc3f 100644 --- a/src/lib/vtaskmgr.c +++ b/src/lib/vtaskmgr.c @@ -68,6 +68,11 @@ void run_task(vtask_t* task) if (ep != NULL) { endpoint_unblock_tasks(ep); + /* bug-1298: release the per-vtask reference taken in vtask_enq. If + * this was the last reference (owner already dropped on a torn-down + * ep), endpoint_unref fini's + frees it here — safely, because no + * other vtask references it and the unblock above already completed. */ + endpoint_unref(ep); } } @@ -187,6 +192,14 @@ static void taskq_item_killer(data_item_t* it) task->data->u.iodata.pkt = NULL; } + /* bug-1298: a queued-but-never-executed VTASK_IO still holds the per-vtask + * endpoint reference taken in vtask_enq (run_task never ran to release it). + * Drop it here so the ep's refcount can reach 0 and it is freed. */ + if (task->data != NULL && task->data->vtype == VTASK_IO && + task->data->u.iodata.ep != NULL) { + endpoint_unref(task->data->u.iodata.ep); + } + vtask_fini(task); free(task); } @@ -483,13 +496,30 @@ int vtask_enq_classified(vtaskmgr_t* vtaskmgr, vtask_t* task, vtask_class_t cls) int vtask_enq(vtaskmgr_t* vtaskmgr, vtask_t* task) { - int rc = 0; + int rc = 0; + endpoint_t* ep = NULL; + + /* bug-1298: an ep-bound VTASK_IO holds a reference on its endpoint for the + * whole time it sits in the taskq (the dispatcher reads ep->busy via + * vtask_eligible) and while it runs. Acquire BEFORE dataq_enq publishes the + * task to the dispatcher; release in run_task (executed) or taskq_item_killer + * (dropped at fini). This keeps the io_worker from freeing the ep out from + * under a queued vtask. */ + if ((task->data != NULL) && (task->data->vtype == VTASK_IO) && + (task->data->u.iodata.ep != NULL)) { + ep = task->data->u.iodata.ep; + endpoint_ref(ep); + } rc = dataq_enq(&(vtaskmgr->taskq), &(task->link)); if (rc >= 0) { /* wake up task dispatcher */ task->owner = vtaskmgr; vtaskmgr_wakeup(vtaskmgr); + } else if (ep != NULL) { + /* enqueue failed — the task never entered the queue; drop the ref we + * just took so the caller's own teardown (endpoint_unref) balances. */ + endpoint_unref(ep); } return rc;