lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(controller): CtrlHelloRequest.key Data field crashed HA leader election (bug-1835)

Browse source 
CtrlHelloRequest.key was declared `Data` in controller.capnp. This
project's capnp-c backend silently no-ops encoding of Data-typed fields
without zero-initializing the caller's local struct, leaving an
uninitialized capn_ptr that write_CtrlHelloRequest unconditionally wrote
to the wire -- SIGSEGV in capn_setp/copy_ptr. Hit live: ctrl-1 won its
Raft election and crashed calling ha_send_ctrlhello_to_all_standbys().

Same bug class as bug-1504/bug-1505, both already fixed by switching the
field from Data to List(UInt8); bug-1505's own writeup had flagged
CtrlHelloRequest.key as a known-but-unfixed instance. Applied the
identical fix here: Data -> List(UInt8) + keyLen companion in the schema,
uint8_t*/key_len in typemap.h. The field is unused in production (always
NULL), so no consumer code changed. Added a round-trip regression test
covering both the NULL-key production shape and real key content.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Rongsong Shen 2026-09-21 03:51:48 +00:00
parent 1e43345b2e
commit cd9ecb4159
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
7 changed files with 197 additions and 21 deletions
Download patch file Download diff file Expand all files Collapse all files

106
src/controller/testsuite/test_e2e_capnp.c
View file

@ -273,6 +273,111 @@ static void test_wal_catchup_append_term_fields_round_trip(void)
printf("test_wal_catchup_append_term_fields_round_trip: PASS\n");
}
/* ------------------------------------------------------------------ */
/* CtrlHelloRequest.key round-trip through the generic codec, now that */
/* it's List(UInt8) instead of Data -- this is the exact field/path that */
/* SIGSEGV'd a live 3-node HA cluster (write_CtrlHelloRequest writing an */
/* uninitialized capn_ptr for a no-op'd Data field, hit the moment a */
/* newly-elected primary called ha_send_ctrlhello_to_all_standbys()). */
/* bug-1505's own writeup had already flagged CtrlHelloRequest.key as a */
/* known-but-unfixed instance of the same bug class as bug-1504/bug-1505 */
/* above; see controller.capnp's CtrlHelloRequest.key comment for the */
/* full root cause. Covers both the actual production shape (key==NULL, */
/* the only value any current caller ever sets -- must not crash) and a */
/* real non-NULL key, to prove the codec genuinely carries content now */
/* rather than silently dropping it. */
/* ------------------------------------------------------------------ */
static void test_ctrl_hello_request_key_round_trip(void)
{
/* Case 1: key == NULL (matches ha_send_ctrlhello_to_all_standbys and
* ha.c's hello builder -- the ONLY shape any production caller uses
* today, and exactly what used to SIGSEGV via uninitialized stack). */
ctrl_hello_req_t src;
memset(&src, 0, sizeof(src));
src.name = "ctrl-1";
src.key = NULL;
src.key_len = 0;
src.applied_lsn = 100ULL;
src.config_tag = 55ULL;
struct capn c;
capn_init_malloc(&c);
struct capn_segment *cs = capn_root(&c).seg;
CtrlHelloRequest_ptr p;
encode_CtrlHelloRequest_ptr(cs, &p, &src);
capn_setp(capn_root(&c), 0, p.p);
int64_t sz = capn_size(&c);
TEST_ASSERT_TRUE(sz > 0);
uint8_t *buf = (uint8_t *)malloc((size_t)sz);
TEST_ASSERT_NOT_NULL(buf);
capn_write_mem(&c, buf, (size_t)sz, 0);
capn_free(&c);
struct capn d;
TEST_ASSERT_EQUAL_INT(0, capn_init_mem(&d, buf, (size_t)sz, 0));
CtrlHelloRequest_ptr dp;
dp.p = capn_getp(capn_root(&d), 0, 1);
struct CtrlHelloRequest wire;
read_CtrlHelloRequest(&wire, dp);
ctrl_hello_req_t dst;
memset(&dst, 0, sizeof(dst));
decode_CtrlHelloRequest(&dst, &wire);
TEST_ASSERT_EQUAL_UINT64(100ULL, dst.applied_lsn);
TEST_ASSERT_EQUAL_UINT64(55ULL, dst.config_tag);
TEST_ASSERT_EQUAL_UINT32(0, dst.key_len);
TEST_ASSERT_NULL(dst.key);
free_CtrlHelloRequest(&dst);
capn_free(&d);
free(buf);
/* Case 2: non-NULL key -- proves the wire genuinely carries content
* now, which the old no-op'd Data codec never did. */
static const uint8_t key_bytes[] = {
0xDE, 0xAD, 0xBE, 0xEF, 0x01, 0x02, 0x03, 0x04
};
memset(&src, 0, sizeof(src));
src.name = "ctrl-2";
src.key = (uint8_t *)key_bytes;
src.key_len = (uint32_t)sizeof(key_bytes);
src.applied_lsn = 200ULL;
src.config_tag = 99ULL;
capn_init_malloc(&c);
cs = capn_root(&c).seg;
encode_CtrlHelloRequest_ptr(cs, &p, &src);
capn_setp(capn_root(&c), 0, p.p);
sz = capn_size(&c);
TEST_ASSERT_TRUE(sz > 0);
buf = (uint8_t *)malloc((size_t)sz);
TEST_ASSERT_NOT_NULL(buf);
capn_write_mem(&c, buf, (size_t)sz, 0);
capn_free(&c);
TEST_ASSERT_EQUAL_INT(0, capn_init_mem(&d, buf, (size_t)sz, 0));
dp.p = capn_getp(capn_root(&d), 0, 1);
read_CtrlHelloRequest(&wire, dp);
memset(&dst, 0, sizeof(dst));
decode_CtrlHelloRequest(&dst, &wire);
TEST_ASSERT_EQUAL_UINT64(200ULL, dst.applied_lsn);
TEST_ASSERT_EQUAL_UINT64(99ULL, dst.config_tag);
TEST_ASSERT_EQUAL_UINT32((uint32_t)sizeof(key_bytes), dst.key_len);
TEST_ASSERT_NOT_NULL(dst.key);
TEST_ASSERT_EQUAL_MEMORY(key_bytes, dst.key, sizeof(key_bytes));
free_CtrlHelloRequest(&dst);
capn_free(&d);
free(buf);
printf("test_ctrl_hello_request_key_round_trip: PASS\n");
}
/* ------------------------------------------------------------------ */
/* Test 6: Data union's new jobArray variant (job-array-single-wal-record */
/* task 1.2, Req 6.1/6.2) round-trips the compact (base jobid, array size, */
@ -1736,6 +1841,7 @@ void test_e2e_capnp(void)
RUN_TEST(test_repl_envelope_capnp_fields);
RUN_TEST(test_request_vote_last_log_tuple_round_trip);
RUN_TEST(test_wal_catchup_append_term_fields_round_trip);
RUN_TEST(test_ctrl_hello_request_key_round_trip);
RUN_TEST(test_data_job_array_variant_round_trip);
RUN_TEST(test_replay_header_snapshot_entry_round_trip);
RUN_TEST(test_session_summary_round_trip);

5
src/include/typemap.h
View file

@ -623,9 +623,12 @@ struct heartbeat_msg_s {
/* CtrlHello state sync (Req 6.5, 6.6, 6.7) */
struct ctrl_hello_req_s {
char *name; /* standby's logical name */
xce_data_t *key; /* SHA-256(cluster_name + shared_secret) auth key */
uint8_t *key; /* SHA-256(cluster_name + shared_secret) auth key,
* malloc'd, or NULL; wire type is List(UInt8),
* not Data (see controller.capnp's own comment) */
uint64_t applied_lsn; /* standby's last applied WAL LSN */
uint64_t config_tag; /* standby's current config tag */
uint32_t key_len; /* length of key */
};
struct ctrl_hello_reply_s {

16
src/schema/controller.capnp
View file

@ -1142,9 +1142,23 @@ struct WalCatchupEntry $C.mapname("wal_catchup_entry_t") {
struct CtrlHelloRequest $C.mapname("ctrl_hello_req_t") {
name @0 :Text $C.mapname("name");
key @1 :Data $C.mapname("key");
# key is List(UInt8), NOT Data (fixes a live production SIGSEGV, same bug
# class as bug-1504/bug-1505 -- see SnapshotData.internalState/
# WalCatchupEntry.payload above for the full root cause: this codebase's
# capnpc-c backend silently no-ops every Data-typed field's encoding in
# the generated encode_XXX() helper without zero-initializing the
# caller's local struct first, leaving a genuinely uninitialized capn_ptr
# written onto the wire by write_XXX(). bug-1505's own writeup already
# flagged "CtrlHelloRequest.key" as a known-but-unfixed instance of this
# gap; it went on to actually crash a live 3-node HA cluster (SIGSEGV in
# write_CtrlHelloRequest via encode_CtrlHelloRequest_ptr) the moment a
# newly-elected primary called ha_send_ctrlhello_to_all_standbys() --
# confirming the risk was real, not theoretical. keyLen (@4, below) is
# its maplistcount companion.
key @1 :List(UInt8) $C.mapname("key") $C.maplistcount("key_len");
appliedLsn @2 :UInt64 $C.mapname("applied_lsn");
configTag @3 :UInt64 $C.mapname("config_tag");
keyLen @4 :UInt32 $C.mapname("key_len");
}
struct CtrlHelloReply $C.mapname("ctrl_hello_reply_t") {