From cd9ecb4159cb54167f69849d36811ecaf1971de3 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Mon, 21 Sep 2026 03:51:48 +0000 Subject: [PATCH] fix(controller): CtrlHelloRequest.key Data field crashed HA leader election (bug-1835) 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 --- .wolf/anatomy-index.json | 39 +++++--- .wolf/anatomy.md | 9 +- .wolf/buglog.json | 34 +++++++ .wolf/memory.md | 9 ++ src/controller/testsuite/test_e2e_capnp.c | 106 ++++++++++++++++++++++ src/include/typemap.h | 5 +- src/schema/controller.capnp | 16 +++- 7 files changed, 197 insertions(+), 21 deletions(-) diff --git a/.wolf/anatomy-index.json b/.wolf/anatomy-index.json index 61697ae6..36784389 100644 --- a/.wolf/anatomy-index.json +++ b/.wolf/anatomy-index.json @@ -1,13 +1,13 @@ { "version": 1, "meta": { - "lastScanned": "2026-09-20T13:14:29.254Z", - "fileCount": 665, + "lastScanned": "2026-09-21T03:39:53.909Z", + "fileCount": 666, "hits": 0, "misses": 0, - "renderedHash": "858015d3083ccb0ba678b885dd6c20d8b4487c0043a691ea447f0c97ee9f9308", - "storeUpdatedAt": "2026-09-20T13:14:29.263Z", - "rootHash": "e1e5eae9e6a128bf" + "renderedHash": "45c99420f9851c89e6b8a493a3c4c5f0a05400f4342a6cd486d0b86ccf3680a8", + "storeUpdatedAt": "2026-09-21T03:39:53.919Z", + "rootHash": "5dc33e10712cf857" }, "files": { ".agent-shell/transcripts/2026-08-11-11-39-59.md": { @@ -10430,20 +10430,20 @@ }, "src/schema/controller.capnp": { "description": "Declares OPCode", - "tokens": 16126, - "hash": "90b5b72153c11857", - "size": 61231, - "mtimeMs": 1789566368210.08, - "updatedAt": "2026-09-16T13:46:08.258Z", + "tokens": 16389, + "hash": "9689b05d99cd0023", + "size": 62218, + "mtimeMs": 1789961885889.9292, + "updatedAt": "2026-09-21T03:38:05.932Z", "source": "hook" }, "src/include/typemap.h": { "description": "if !defined(_TYPEMAP_H_)", - "tokens": 8664, - "hash": "e67c46e54df6e1f8", - "size": 32933, - "mtimeMs": 1789566375907.337, - "updatedAt": "2026-09-16T13:46:15.946Z", + "tokens": 8720, + "hash": "f038a62e2ac8f43a", + "size": 33146, + "mtimeMs": 1789961896088.1375, + "updatedAt": "2026-09-21T03:38:16.133Z", "source": "hook" }, "src/controller/ha.h": { @@ -12207,6 +12207,15 @@ "mtimeMs": 1789910069218.0444, "updatedAt": "2026-09-20T13:14:29.254Z", "source": "hook" + }, + "src/controller/testsuite/test_e2e_capnp.c": { + "description": "include ", + "tokens": 20187, + "hash": "63c314d2bcebbc60", + "size": 75708, + "mtimeMs": 1789961993857.0706, + "updatedAt": "2026-09-21T03:39:53.909Z", + "source": "hook" } } } \ No newline at end of file diff --git a/.wolf/anatomy.md b/.wolf/anatomy.md index b94561c0..ff4da164 100644 --- a/.wolf/anatomy.md +++ b/.wolf/anatomy.md @@ -1,7 +1,7 @@ # anatomy.md -> Auto-maintained by OpenWolf. Last scanned: 2026-09-20T13:14:29.254Z -> Files: 665 tracked | Anatomy hits: 0 | Misses: 0 +> Auto-maintained by OpenWolf. Last scanned: 2026-09-21T03:39:53.909Z +> Files: 666 tracked | Anatomy hits: 0 | Misses: 0 ## ./ @@ -893,6 +893,7 @@ - `clean_failover_preservation.sh` — clean_failover_preservation.sh — ha-term-safety task 5.2 live integration test. (~4972 tok) - `controller_test.c` — include (~2466 tok) - `test_ctrlstatus.c` — include (~12594 tok) +- `test_e2e_capnp.c` — include (~20187 tok) - `test_ha_heartbeat_committed.c` — include (~5775 tok) - `test_ha_liveness.c` — Declares directly (~25885 tok) - `test_ha_partition.c` (~5500 tok) @@ -916,7 +917,7 @@ - `request.h` — if !defined(_REQUEST_H_) (~3288 tok) - `resreq.h` — if !defined(_RESREQ_H_) (~3310 tok) -- `typemap.h` — if !defined(_TYPEMAP_H_) (~8664 tok) +- `typemap.h` — if !defined(_TYPEMAP_H_) (~8720 tok) - `utils.h` — if !defined(_UTILS_H_) (~3454 tok) - `xcelimit.h` — if !defined(_XCELIMIT_H_) (~428 tok) @@ -1040,7 +1041,7 @@ ## src/schema/ -- `controller.capnp` — Declares OPCode (~16126 tok) +- `controller.capnp` — Declares OPCode (~16389 tok) ## src/tools/ diff --git a/.wolf/buglog.json b/.wolf/buglog.json index aa0d2cdd..bf504d83 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -24742,6 +24742,40 @@ ], "occurrences": 1, "last_seen": "2026-09-20T10:15:00Z" + }, + { + "id": "bug-1835", + "timestamp": "2026-09-21T03:47:10Z", + "error_message": "Live 3-node HA cluster crash: xce_controller SIGSEGV in __memcpy_avx_unaligned_erms, called from capnp's copy_ptr/capn_setp, called from write_CtrlHelloRequest (lib/controller.capnp.c:22821) via encode_CtrlHelloRequest_ptr (lib/controller.capnp.c:22935), via xce_encode/xce_request, via ha_send_ctrlhello_to_all_standbys (controller/cluster.c:2432), via raft_cb_become_leader (controller/ha.c:2113), via raft_on_vote_reply -> vote_reply_dispatch -> peer_ctrl_msg_handler_inline -> peer_ctrl_vtask_entry, on the vcpu thread handling the winning vote reply. Core confirmed built from commit 1e43345b (xce v0.0.1-1e43345b, 2026-09-21T03:10:39Z); ctrl-1 had just won its Raft election (term 2, votes_granted=2) and crashed while broadcasting CtrlHello to its 2 standbys.", + "file": "src/schema/controller.capnp", + "root_cause": "CtrlHelloRequest.key was declared `Data` in controller.capnp -- the exact same latent bug class as bug-1504 (SnapshotData.internalState) and bug-1505 (WalCatchupEntry.payload), and explicitly named as a known-but-unfixed instance of it in bug-1505's own writeup ('Known/suspected other Data fields with the same latent gap: ... CtrlHelloRequest.key'). This project's capnp-c backend silently no-ops the encoding of any Data-typed field inside the generated encode_XXX() composite helper (see encode_CtrlHelloRequest's ' /* key key */' stub, lib/controller.capnp.c) without ever zero-initializing the caller's local `struct CtrlHelloRequest d;` first (encode_CtrlHelloRequest_ptr, lib/controller.capnp.c:22926-22938) -- so d.key.p is left as genuinely uninitialized stack garbage. write_CtrlHelloRequest (lib/controller.capnp.c:22821) then unconditionally calls capn_setp(p.p, 1, s->key.p) with that garbage capn_ptr, and capn_setp's copy_ptr dereferences the garbage segment/data pointer, SIGSEGV'ing in memcpy. ha_send_ctrlhello_to_all_standbys (controller/cluster.c:2419-2424) and ha.c:9495 both always set ctrl_hello_req_t.key = NULL (the field has never had a real producer anywhere in the codebase -- confirmed via project-wide grep, no call site ever sets it to non-NULL nor reads it back), so this is genuine undefined behavior whose manifestation depends entirely on whatever garbage bytes happen to occupy that vtask thread's stack slot at each call -- exactly the same 'lucky vs unlucky stack layout' nondeterminism documented for bug-1504/1505, which is why this had not crashed before despite the gap being known since 2026-08-07. Also confirmed the C struct itself compounded the risk: ctrl_hello_req_s.key (typemap.h) was typed `xce_data_t*` -- the local-Data-struct-shadowing wrapper type from bug-1461's own root cause -- rather than a real byte-blob type, even though it was never actually used as one.", + "fix": "FIXED (2026-09-21), same pattern as bug-1504/bug-1505: changed CtrlHelloRequest.key from `Data` to `List(UInt8)` in src/schema/controller.capnp (which this capnp-c backend DOES correctly auto-generate real, non-no-op encode/decode/free for -- proven precedent: FetchFileReply.content/bytesRead, SnapshotData.internalState, WalCatchupEntry.payload), with a new keyLen (@4, UInt32) companion field as its maplistcount. Updated typemap.h's ctrl_hello_req_s.key from `xce_data_t*` to `uint8_t*` and added a new `key_len` (uint32_t) field. No consumer-code changes were needed: project-wide grep confirmed .key is never set to non-NULL nor read anywhere in production (ha_send_ctrlhello_to_all_standbys and ha.c's hello builder both always pass NULL), so the fix is purely structural -- it eliminates the uninitialized-capn_ptr class of UB for this field rather than papering over one occurrence. Added regression test test_ctrl_hello_request_key_round_trip (controller/testsuite/test_e2e_capnp.c), covering both the actual production shape (key==NULL/key_len==0, the exact shape that used to SIGSEGV -- now proven to encode/decode cleanly via the real generic codec) and a real non-NULL key payload (proves content genuinely round-trips now, which the old no-op'd Data codec never did). A deterministic RED test reproducing the SIGSEGV itself was not attempted -- by its own nature as reads of uninitialized stack memory, this bug class is not ASAN-catchable and its manifestation is stack-layout-dependent (bug-1504's own writeup makes the identical observation); the fix instead structurally eliminates the uninitialized-read entirely rather than chasing a flaky repro. Verified: full controller_unit_test suite (5/5 meson test binaries, 0 failures, including the new round-trip test and the pre-existing test_ctrl_hello_req_t_layout/test_ha_ctrlhello.c suite) via a clean ASAN rebuild (`xce-build.sh controller asan`) that also regenerated controller.capnp.c/.h from the updated schema with no codegen errors.", + "tags": [ + "controller", + "ha", + "capnp-schema", + "codegen-limitation", + "sigsegv", + "uninitialized-memory", + "controller.capnp", + "ctrl-hello", + "raft", + "leader-election", + "systemic", + "user-reported", + "live-crash", + "related-to-bug-1504", + "related-to-bug-1505", + "related-to-bug-1461", + "fixed" + ], + "related_bugs": [ + "bug-1504", + "bug-1505", + "bug-1461" + ], + "occurrences": 1, + "last_seen": "2026-09-21T03:47:10Z" } ] } diff --git a/.wolf/memory.md b/.wolf/memory.md index 2dd5ef5c..cd6b15f1 100644 --- a/.wolf/memory.md +++ b/.wolf/memory.md @@ -5524,3 +5524,12 @@ | 04:53 | Edited src/lib/testsuite/test_limit.c | added 2 condition(s) | ~1989 | | 04:53 | Edited src/lib/testsuite/test_limit.c | expanded (+8 lines) | ~135 | | 06:14 | Created reports/sep13-sep19-2026.md | — | ~3659 | + +## Session: 2026-09-21 20:31 + +| Time | Action | File(s) | Outcome | ~Tokens | +|------|--------|---------|---------|--------| +| 20:38 | Edited src/schema/controller.capnp | modified mapname() | ~329 | +| 20:38 | Edited src/include/typemap.h | 7→10 lines | ~151 | +| 20:39 | Edited src/controller/testsuite/test_e2e_capnp.c | modified test_ctrl_hello_request_key_round_trip() | ~1131 | +| 20:39 | Edited src/controller/testsuite/test_e2e_capnp.c | 2→3 lines | ~46 | diff --git a/src/controller/testsuite/test_e2e_capnp.c b/src/controller/testsuite/test_e2e_capnp.c index 34a71e06..05d87ee2 100644 --- a/src/controller/testsuite/test_e2e_capnp.c +++ b/src/controller/testsuite/test_e2e_capnp.c @@ -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); diff --git a/src/include/typemap.h b/src/include/typemap.h index cc16b587..34bbd73e 100644 --- a/src/include/typemap.h +++ b/src/include/typemap.h @@ -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 { diff --git a/src/schema/controller.capnp b/src/schema/controller.capnp index e2ec0028..97530f52 100644 --- a/src/schema/controller.capnp +++ b/src/schema/controller.capnp @@ -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") {