wdgzyjx/myxce
1
0
Fork 0
forked from lei.chen/myxce
Code Pull requests Activity

fix(libxce): endpoint reference count — kill teardown-vs-dispatch UAF (bug-1298, task 5.8)

Browse source 
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).
This commit is contained in:
Rongsong Shen 2026-06-30 14:24:47 +00:00
parent 5be165312a
commit 6e05dc1523
6 changed files with 137 additions and 58 deletions
Download patch file Download diff file Expand all files Collapse all files

20
src/controller/cluster.c
View file

@ -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;
}

71
src/controller/ha.c
View file

@ -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,

17
src/controller/testsuite/test_ha_vep_teardown.c
View file

@ -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);

19
src/include/netio.h
View file

@ -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*);

36
src/lib/netio.c
View file

@ -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;

32
src/lib/vtaskmgr.c
View file

@ -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;