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