fix(controller): standby-endpoint err handler freed ep->data under a live vtask (bug-1839)
Live 3-node HA soak crashed xce_controller: SIGSEGV in peer_ctrl_msg_handler_inline (ha.c:7035) dereferencing a NULL ep->data. gdb analysis of the actual core dump showed the endpoint itself fully alive (refcnt=191964, tearing_down=0) while ep->data had been synchronously freed and nulled by standby_ep_err_handler_primary, the endpoint's err_handler, with zero coordination with the vtask pool that was concurrently dispatching a peer-control message for the same endpoint (g_drive_async_peerctrl_dispatch_enabled defaults ON). The sibling handlers (vote_ep_err_handler, pre_vote_ep_err_handler) already avoid this exact hazard, with an explicit comment: "Do NOT free vctx here -- peer_ctrl_msg_handler may still reference ep->data." standby_ep_err_handler_primary was never updated to the same discipline when the vtask-pool dispatch machinery (bug-1197/bug-1298) was introduced. Fix: route standby_ep_err_handler_primary through the same already-tested vote_ep_teardown_async() deferred, per-endpoint-FIFO- serialized teardown that vote-endpoint demotion already uses, plus an idempotency guard (endpoint_is_tearing_down) since the old "ep->data == NULL" reentry check no longer applies once the free is deferred. Also fixes a second-order double-unref this surfaced in on_standby_connected's stale-duplicate eviction path, caught by ASAN. TDD: two new tests prove the teardown is deferred until an in-flight same-ep vtask completes, and that a repeated error callback on the same endpoint is a safe no-op. Full controller suite: 1219/1219, zero regressions. Also logs bug-1840 (a separate scheduler heap-corruption crash from the same soak run) as not-fixed pending an ASAN reproduction -- a non-instrumented core dump cannot localize glibc's malloc_consolidate abort back to the actual out-of-bounds write. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
4f94638a1d
commit
e1da9f1e9b
7 changed files with 299 additions and 38 deletions
|
|
@ -9462,14 +9462,14 @@ static void on_standby_connected(endpoint_t *ep)
|
|||
WARN("HA: peer[%d] reconnected — evicting stale duplicate outbound "
|
||||
"channel to keep exactly one live connection (Req 2.6)",
|
||||
sctx->peer_idx);
|
||||
if (stale_same_peer->iomgr)
|
||||
iomgr_unmonitor((iomgr_t *)stale_same_peer->iomgr, stale_same_peer);
|
||||
/* bug-1839: standby_ep_err_handler_primary now owns the FULL
|
||||
* teardown itself (mark tearing_down, iomgr_unmonitor if needed,
|
||||
* defer the owner-ref drop via vote_ep_teardown_async so a same-ep
|
||||
* peer-control vtask already queued/in-flight is not read after
|
||||
* free) — it used to only free ep->data/null it, leaving the caller
|
||||
* to iomgr_unmonitor + endpoint_unref manually. Do NOT also
|
||||
* unmonitor/unref here: doing so double-drops the owner reference. */
|
||||
(void)standby_ep_err_handler_primary(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
|
||||
|
|
@ -9542,7 +9542,18 @@ static void on_standby_connected(endpoint_t *ep)
|
|||
*/
|
||||
static int standby_ep_err_handler_primary(void *ep_raw)
|
||||
{
|
||||
endpoint_t *ep = (endpoint_t *)ep_raw;
|
||||
endpoint_t *ep = (endpoint_t *)ep_raw;
|
||||
|
||||
/* bug-1839: idempotency guard. Once this handler stops nulling ep->data
|
||||
* synchronously (below), a second handle_error() callback for the same
|
||||
* ep (e.g. a read error and a write error both observed before
|
||||
* iomgr_unmonitor takes effect) must not re-enter vote_ep_teardown_async
|
||||
* and double-drop the endpoint's owner reference. endpoint_mark_tearing_
|
||||
* down (inside vote_ep_teardown_async) sets this flag synchronously
|
||||
* before returning, so a repeated call deterministically observes it. */
|
||||
if (endpoint_is_tearing_down(ep))
|
||||
return 0;
|
||||
|
||||
standby_ep_ctx_t *sctx = (standby_ep_ctx_t *)ep->data;
|
||||
if (!sctx) return 0;
|
||||
|
||||
|
|
@ -9566,8 +9577,15 @@ static int standby_ep_err_handler_primary(void *ep_raw)
|
|||
if (c)
|
||||
repl_remove_standby(&c->persist.repl, ep);
|
||||
|
||||
free(sctx);
|
||||
ep->data = NULL;
|
||||
/* bug-1839: do not free sctx / null ep->data synchronously here — a
|
||||
* peer-control vtask for this same ep may already be queued or in
|
||||
* flight (peer_ctrl_msg_handler_inline reads ep->data), same hazard
|
||||
* vote_ep_err_handler's own comment documents for vote_ep_ctx_t just
|
||||
* above. Defer to the same safe, per-ep-FIFO-serialized teardown
|
||||
* vote_ep_teardown_async already uses for promoted vote-endpoint
|
||||
* demotion (raft_cb_become_follower) — endpoint_fini's own
|
||||
* FREEUP(ep->data) frees sctx once the last reference releases. */
|
||||
vote_ep_teardown_async(ha, ep);
|
||||
return 0;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1456,10 +1456,13 @@ static void test_reconnect_resolves_dual_channel_same_peer(void)
|
|||
"the stale same-peer channel must be evicted exactly once");
|
||||
|
||||
/* fresh is owned by standby_endpoints; tear it down the production way so
|
||||
* LSan sees the heap ep + ctx reclaimed (the err handler frees both). */
|
||||
* LSan sees the heap ep + ctx reclaimed. bug-1839: the err handler now
|
||||
* defers to vote_ep_teardown_async, which (ha.pool == NULL here, no live
|
||||
* iomgr in this unit fixture) takes the documented no-pool inline
|
||||
* fallback — endpoint_unref drops the sole owner ref straight to 0 and
|
||||
* fini's + frees fresh itself, so no separate endpoint_fini/free call
|
||||
* is needed (or safe) here anymore. */
|
||||
standby_ep_err_handler_primary_for_test(fresh);
|
||||
endpoint_fini(fresh);
|
||||
free(fresh);
|
||||
|
||||
fini_ha_primary_with_standby_eps(&c.ha);
|
||||
printf("test_reconnect_resolves_dual_channel_same_peer: PASS\n");
|
||||
|
|
@ -1515,9 +1518,9 @@ static void test_reconnect_resolves_dual_channel_promoted_vote_ep(void)
|
|||
atomic_load(&ha_standby_evicted_count_for_test),
|
||||
"the stale promoted-vote-ep channel must be evicted exactly once");
|
||||
|
||||
/* bug-1839: see test_reconnect_resolves_dual_channel_same_peer's comment
|
||||
* above — the err handler now owns fresh's full teardown itself. */
|
||||
standby_ep_err_handler_primary_for_test(fresh);
|
||||
endpoint_fini(fresh);
|
||||
free(fresh);
|
||||
free(dummy_ectx);
|
||||
|
||||
fini_ha_primary_with_standby_eps(&c.ha);
|
||||
|
|
@ -1551,12 +1554,10 @@ static void test_reconnect_preserves_other_peer_channel(void)
|
|||
atomic_load(&ha_standby_evicted_count_for_test),
|
||||
"no eviction when the peer_idx differs");
|
||||
|
||||
/* bug-1839: see test_reconnect_resolves_dual_channel_same_peer's comment
|
||||
* above — the err handler now owns each ep's full teardown itself. */
|
||||
standby_ep_err_handler_primary_for_test(peer1);
|
||||
endpoint_fini(peer1);
|
||||
free(peer1);
|
||||
standby_ep_err_handler_primary_for_test(peer2);
|
||||
endpoint_fini(peer2);
|
||||
free(peer2);
|
||||
|
||||
fini_ha_primary_with_standby_eps(&c.ha);
|
||||
printf("test_reconnect_preserves_other_peer_channel: PASS\n");
|
||||
|
|
|
|||
|
|
@ -40,6 +40,11 @@ extern int peer_ctrl_msg_handler_for_test(endpoint_t *ep, io_packet_t *pkt);
|
|||
extern io_packet_t *repl_catchup_done_pkt_for_test(void);
|
||||
extern void raft_cb_become_follower_for_test(ha_state_t *ha, uint64_t term);
|
||||
|
||||
/* bug-1839: standby-endpoint error-handler deferred-teardown seams. */
|
||||
extern endpoint_t *standby_ep_ctx_build_for_test(endpoint_t *ep, ha_state_t *ha,
|
||||
cluster_t *c, int peer_idx);
|
||||
extern int standby_ep_err_handler_primary_for_test(endpoint_t *ep);
|
||||
|
||||
/* ── Fixtures ────────────────────────────────────────────────────────────── */
|
||||
|
||||
static ha_config_t g_cfg = {
|
||||
|
|
@ -623,6 +628,167 @@ static void test_vep_self_teardown_does_not_deadlock(void)
|
|||
printf("test_vep_self_teardown_does_not_deadlock: PASS\n");
|
||||
}
|
||||
|
||||
/* ── Test (e): bug-1839 — standby_ep_err_handler_primary defers teardown ──
|
||||
*
|
||||
* standby_ep_err_handler_primary (the err_handler for an outbound
|
||||
* standby/heartbeat channel, ha_connect_prep_peer) used to free ep->data and
|
||||
* null it out SYNCHRONOUSLY, on whatever thread handle_error() runs on, with
|
||||
* zero coordination with the vtask pool — unlike vote_ep_err_handler /
|
||||
* pre_vote_ep_err_handler (both of which explicitly leave vctx alone with a
|
||||
* "peer_ctrl_msg_handler may still reference ep->data" comment). A live
|
||||
* 3-node HA crash (bug-1839) hit exactly this: a peer-control vtask already
|
||||
* queued for an ep read ep->data == NULL after this handler had raced ahead
|
||||
* of it. The fix routes it through the SAME deferred, per-ep-FIFO-serialized
|
||||
* teardown (vote_ep_teardown_async) that vote-ep demotion already uses —
|
||||
* this test proves the ordering guarantee holds for THIS call site too,
|
||||
* using a real standby_ep_ctx_t (not the generic vote_ep_ctx_t stand-in
|
||||
* test (a) above uses) and the real handler entry point.
|
||||
*/
|
||||
static void test_standby_err_handler_defers_teardown_until_inflight_completes(void)
|
||||
{
|
||||
reset_oracles();
|
||||
gate_init();
|
||||
|
||||
ha_state_t ha;
|
||||
TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 1));
|
||||
|
||||
vtaskmgr_t pool;
|
||||
TEST_ASSERT_EQUAL_INT(0, vtaskmgr_init(&pool, 2));
|
||||
TEST_ASSERT_EQUAL_INT(0, vtaskmgr_start(&pool));
|
||||
ha.pool = &pool;
|
||||
|
||||
iomgr_t iomgr;
|
||||
TEST_ASSERT_EQUAL_INT(0, iomgr_init(&iomgr));
|
||||
|
||||
endpoint_t *ep = endpoint_get();
|
||||
TEST_ASSERT_NOT_NULL(ep);
|
||||
ep->iomgr = &iomgr;
|
||||
ep->fd = -1;
|
||||
TEST_ASSERT_NOT_NULL(standby_ep_ctx_build_for_test(ep, &ha, NULL, 0));
|
||||
|
||||
ha_set_peerctrl_dispatch_for_test(1);
|
||||
ha_vep_teardown_probe = teardown_probe;
|
||||
|
||||
/* Park an in-flight peer-control vtask on this ep (busy=1 while gated) —
|
||||
* standing in for the real "peer_ctrl_dispatch_to_pool already enqueued
|
||||
* a vtask for this ep" race the live crash hit. */
|
||||
enqueue_io_vtask(&pool, ep, (vtask_func_t)inflight_vtask_body);
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(0, gate_wait_entered(2000),
|
||||
"in-flight vtask must run and park (busy=1)");
|
||||
|
||||
/* The connection now hits a socket/SSL error — standby_ep_err_handler_
|
||||
* primary fires on this (test) thread, exactly as handle_error() would
|
||||
* call it on the io_worker thread in production. */
|
||||
TEST_ASSERT_EQUAL_INT(0, standby_ep_err_handler_primary_for_test(ep));
|
||||
|
||||
/* While the in-flight vtask is still parked, the deferred teardown must
|
||||
* NOT have run yet — ep->data must still be alive for anything else
|
||||
* still queued behind it. */
|
||||
TEST_ASSERT_TRUE_MESSAGE(endpoint_is_tearing_down(ep),
|
||||
"ep must be marked tearing_down once the err handler defers teardown");
|
||||
struct timespec ts = { .tv_sec = 0, .tv_nsec = 80 * 1000000L };
|
||||
nanosleep(&ts, NULL);
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(0, atomic_load(&g_teardown_done),
|
||||
"bug-1839: teardown must NOT run while an in-flight same-ep vtask "
|
||||
"is parked — ep->data must not be freed out from under it");
|
||||
|
||||
/* Release the in-flight vtask; teardown becomes eligible and runs. */
|
||||
gate_open();
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(0, poll_until(&g_teardown_done, 1, 2000),
|
||||
"teardown must run once the in-flight vtask completes");
|
||||
|
||||
TEST_ASSERT_GREATER_THAN_INT_MESSAGE(atomic_load(&g_inflight_seq),
|
||||
atomic_load(&g_teardown_seq),
|
||||
"teardown_seq must be strictly after inflight_seq");
|
||||
|
||||
vtaskmgr_fini(&pool);
|
||||
iomgr_fini(&iomgr);
|
||||
ha_vep_teardown_probe = NULL;
|
||||
ha_set_peerctrl_dispatch_for_test(0);
|
||||
ha.pool = NULL;
|
||||
ha_fini(&ha);
|
||||
gate_destroy();
|
||||
printf("test_standby_err_handler_defers_teardown_until_inflight_completes: PASS\n");
|
||||
}
|
||||
|
||||
/* ── Test (f): bug-1839 — idempotent against a repeated error callback ──
|
||||
*
|
||||
* handle_error() can plausibly fire standby_ep_err_handler_primary more than
|
||||
* once for the same ep (e.g. a read error and a write error both observed
|
||||
* before iomgr_unmonitor takes effect). Once the handler stops nulling
|
||||
* ep->data synchronously, the old "if (!sctx) return 0" reentry guard no
|
||||
* longer protects against this — a second call would otherwise re-enter
|
||||
* vote_ep_teardown_async and double-drop the endpoint's owner reference.
|
||||
* endpoint_mark_tearing_down sets ep->tearing_down synchronously, inside the
|
||||
* FIRST call, before it returns — so by construction a second call made
|
||||
* strictly after the first returns can deterministically observe the flag
|
||||
* and bail, no gating/timing needed.
|
||||
*/
|
||||
static _Atomic int g_teardown_calls_1839 = 0;
|
||||
|
||||
static void teardown_probe_count_1839(endpoint_t *ep)
|
||||
{
|
||||
(void)ep;
|
||||
atomic_fetch_add(&g_teardown_calls_1839, 1);
|
||||
}
|
||||
|
||||
static void test_standby_err_handler_idempotent_against_double_invocation(void)
|
||||
{
|
||||
reset_oracles();
|
||||
atomic_store(&g_teardown_calls_1839, 0);
|
||||
|
||||
ha_state_t ha;
|
||||
TEST_ASSERT_EQUAL_INT(0, ha_init(&ha, &g_cfg, 1));
|
||||
|
||||
vtaskmgr_t pool;
|
||||
TEST_ASSERT_EQUAL_INT(0, vtaskmgr_init(&pool, 2));
|
||||
TEST_ASSERT_EQUAL_INT(0, vtaskmgr_start(&pool));
|
||||
ha.pool = &pool;
|
||||
|
||||
iomgr_t iomgr;
|
||||
TEST_ASSERT_EQUAL_INT(0, iomgr_init(&iomgr));
|
||||
|
||||
endpoint_t *ep = endpoint_get();
|
||||
TEST_ASSERT_NOT_NULL(ep);
|
||||
ep->iomgr = &iomgr;
|
||||
ep->fd = -1;
|
||||
TEST_ASSERT_NOT_NULL(standby_ep_ctx_build_for_test(ep, &ha, NULL, 0));
|
||||
|
||||
ha_set_peerctrl_dispatch_for_test(1);
|
||||
ha_vep_teardown_probe = teardown_probe_count_1839;
|
||||
|
||||
TEST_ASSERT_EQUAL_INT(0, standby_ep_err_handler_primary_for_test(ep));
|
||||
TEST_ASSERT_TRUE_MESSAGE(endpoint_is_tearing_down(ep),
|
||||
"first call must mark tearing_down");
|
||||
|
||||
/* Second call, same ep, strictly after the first returns: ep is still
|
||||
* alive (the teardown vtask has not necessarily run yet), but must be a
|
||||
* pure no-op — no second teardown vtask, no double endpoint_unref. */
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(0, standby_ep_err_handler_primary_for_test(ep),
|
||||
"bug-1839: a repeated error callback on an already-tearing-down ep "
|
||||
"must be a safe no-op, not a second teardown");
|
||||
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(0, poll_until(&g_teardown_calls_1839, 1, 2000),
|
||||
"the (single) deferred teardown must still run to completion");
|
||||
|
||||
/* Give any wrongly-enqueued second teardown vtask a chance to also run
|
||||
* before asserting the count never exceeds 1 (ASAN would abort on the
|
||||
* underlying double endpoint_unref well before this if the guard were
|
||||
* missing, but assert the count explicitly too). */
|
||||
struct timespec ts = { .tv_sec = 0, .tv_nsec = 80 * 1000000L };
|
||||
nanosleep(&ts, NULL);
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(1, atomic_load(&g_teardown_calls_1839),
|
||||
"exactly one teardown vtask must ever run for this ep");
|
||||
|
||||
vtaskmgr_fini(&pool);
|
||||
iomgr_fini(&iomgr);
|
||||
ha_vep_teardown_probe = NULL;
|
||||
ha_set_peerctrl_dispatch_for_test(0);
|
||||
ha.pool = NULL;
|
||||
ha_fini(&ha);
|
||||
printf("test_standby_err_handler_idempotent_against_double_invocation: PASS\n");
|
||||
}
|
||||
|
||||
/* ── Entry point ─────────────────────────────────────────────────────────── */
|
||||
|
||||
void test_ha_vep_teardown(void)
|
||||
|
|
@ -634,4 +800,6 @@ void test_ha_vep_teardown(void)
|
|||
RUN_TEST(test_vep_off_path_uses_plain_fini);
|
||||
RUN_TEST(test_vep_mark_drains_inflight_dispatch);
|
||||
RUN_TEST(test_vep_self_teardown_does_not_deadlock);
|
||||
RUN_TEST(test_standby_err_handler_defers_teardown_until_inflight_completes);
|
||||
RUN_TEST(test_standby_err_handler_idempotent_against_double_invocation);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue