fix(controller): stop SIGSEGV in WalCatchupEntry capnp encode (bug-1505)
controller_unit_test crashed deterministically in write_WalCatchupEntry (test_wal_catchup_append_term_fields_round_trip) -- the same bug class as bug-1504 (SnapshotData.internalState): this codebase's capnpc-c backend silently no-ops Data-field encoding in generated encode_XXX() composite helpers without zero-initializing the caller's local struct first, leaving an uninitialized capn_ptr written onto the wire. Confirmed zero production exposure before fixing: xce_wal_batch_encode_ raw()/wal_entry_to_wire() (request.c) never call the generic encode_WalCatchupEntry() at all -- they memset() their own struct and call write_WalCatchupEntry() directly with every field hand-set, so this was 100% test-only exposure (grepped every caller of the generic encode/decode pair; only the crashing test uses it). Fix mirrors bug-1504: WalCatchupEntry.payload (@2) and batchPayloads (@10) switched from Data to List(UInt8) in controller.capnp (new batchPayloadsLen @11 companion; payload reuses the existing wirePayloadLen @9), typemap.h's wal_catchup_entry_t field types updated, and request.c's extract_data_bytes() widened to take a raw capn_ptr (it already only used .p internally) so its call sites work regardless of which Data-shaped wrapper the generated accessor returns now. batchPayloads' single-top-level-blob shape confirmed this does not reintroduce bug-847's per-element cross-segment far-pointer loss -- that issue is inherent to per-element data regardless of Data vs List(UInt8) spelling. Strengthened test_wal_catchup_append_term_fields_round_trip with real payload/batchPayloads content assertions -- previously never exercised, since the test's src fields were left NULL. Verified: controller suite 3/3 (was 2/3, crashing), scheduler suite unaffected at 37/37 (unrelated schema area). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JPPXeKSUXUqcqvzNXY1rzJ
This commit is contained in:
parent
829fbe41dc
commit
1fd467d072
SSH key fingerprint:
SHA256:8Vf2pZ3h6rJa7EhGK+WaAQ9Mta/apZvteW6g9U/WKKk
7 changed files with 121 additions and 22 deletions
|
|
@ -195,11 +195,21 @@ static void test_request_vote_last_log_tuple_round_trip(void)
|
|||
/* ------------------------------------------------------------------ */
|
||||
/* Test 5: WalCatchupEntry (replication envelope) round-trips the */
|
||||
/* AppendEntries term fields (prev_log_index, prev_log_term, term) */
|
||||
/* (ha-term-safety task 1.1, Req 1.3/1.4). */
|
||||
/* (ha-term-safety task 1.1, Req 1.3/1.4), AND (bug-1505 fix) genuinely */
|
||||
/* round-trips payload/batchPayloads content through the GENERIC codec */
|
||||
/* now that both are List(UInt8) instead of Data -- this is the exact */
|
||||
/* path that used to SIGSEGV (write_WalCatchupEntry writing an */
|
||||
/* uninitialized capn_ptr for a no-op'd Data field) even though this */
|
||||
/* test never previously touched payload content at all (src.payload */
|
||||
/* stayed NULL); see controller.capnp's WalCatchupEntry.payload comment */
|
||||
/* for the full root cause. */
|
||||
/* ------------------------------------------------------------------ */
|
||||
|
||||
static void test_wal_catchup_append_term_fields_round_trip(void)
|
||||
{
|
||||
static const uint8_t entry_payload[] = { 0x11, 0x22, 0x33, 0x44, 0x55 };
|
||||
static const uint8_t batch_payload[] = { 0xAA, 0xBB, 0xCC, 0xDD };
|
||||
|
||||
wal_catchup_entry_t src;
|
||||
memset(&src, 0, sizeof(src));
|
||||
src.lsn = 42ULL;
|
||||
|
|
@ -207,6 +217,12 @@ static void test_wal_catchup_append_term_fields_round_trip(void)
|
|||
src.term = 7ULL; /* NEW: this entry's term stamp */
|
||||
src.prev_log_index = 41ULL; /* NEW: lsn - 1 */
|
||||
src.prev_log_term = 6ULL; /* NEW: term at prev_log_index */
|
||||
/* bug-1505 fix: real content through the generic codec (List(UInt8),
|
||||
* not Data) for both payload and batchPayloads. */
|
||||
src.payload = (uint8_t *)entry_payload;
|
||||
src.wire_payload_len = (uint32_t)sizeof(entry_payload);
|
||||
src.batch_payloads = (uint8_t *)batch_payload;
|
||||
src.batch_payloads_wire_len = (uint32_t)sizeof(batch_payload);
|
||||
|
||||
struct capn c;
|
||||
capn_init_malloc(&c);
|
||||
|
|
@ -238,6 +254,20 @@ static void test_wal_catchup_append_term_fields_round_trip(void)
|
|||
TEST_ASSERT_EQUAL_UINT64(41ULL, dst.prev_log_index); /* survives the wire */
|
||||
TEST_ASSERT_EQUAL_UINT64(6ULL, dst.prev_log_term); /* survives the wire */
|
||||
|
||||
/* bug-1505 fix: payload/batchPayloads now genuinely round-trip through
|
||||
* the generic codec (List(UInt8), not Data) -- this is the content the
|
||||
* old no-op'd Data-field codec never actually carried. */
|
||||
TEST_ASSERT_EQUAL_UINT32((uint32_t)sizeof(entry_payload),
|
||||
dst.wire_payload_len);
|
||||
TEST_ASSERT_NOT_NULL(dst.payload);
|
||||
TEST_ASSERT_EQUAL_MEMORY(entry_payload, dst.payload, sizeof(entry_payload));
|
||||
|
||||
TEST_ASSERT_EQUAL_UINT32((uint32_t)sizeof(batch_payload),
|
||||
dst.batch_payloads_wire_len);
|
||||
TEST_ASSERT_NOT_NULL(dst.batch_payloads);
|
||||
TEST_ASSERT_EQUAL_MEMORY(batch_payload, dst.batch_payloads,
|
||||
sizeof(batch_payload));
|
||||
|
||||
capn_free(&d);
|
||||
free(buf);
|
||||
printf("test_wal_catchup_append_term_fields_round_trip: PASS\n");
|
||||
|
|
|
|||
|
|
@ -385,8 +385,14 @@ struct snapshot_transfer_s {
|
|||
struct wal_catchup_entry {
|
||||
uint64_t lsn;
|
||||
uint8_t entry_type;
|
||||
xce_data_t *payload; /* legacy decoded field — broken for
|
||||
* raw bytes; kept NULL, do not use */
|
||||
/* bug-1505 fix: List(UInt8) now, not Data (see controller.capnp's
|
||||
* WalCatchupEntry.payload comment for the full root cause) — a real
|
||||
* uint8_t* + wire_payload_len pair via the generic codec, same shape
|
||||
* bytes_read/content already establish elsewhere. Still legacy/unused
|
||||
* by production (wal_entry_to_wire() in request.c deliberately never
|
||||
* sets it — see that comment); this genuinely round-trips now instead
|
||||
* of being a documented-broken placeholder, but nothing reads it. */
|
||||
uint8_t *payload;
|
||||
int n_entries; /* batch: number of sub-entries */
|
||||
struct wal_catchup_entry **entries; /* batch: array of sub-entry pointers */
|
||||
/* Raw WAL entry payload bytes — same pattern as repl_config_push_req_t.
|
||||
|
|
@ -421,15 +427,23 @@ struct wal_catchup_entry {
|
|||
* send; used on decode to slice this entry's bytes out of the TOP-LEVEL
|
||||
* container's batch_payloads blob (below). */
|
||||
uint32_t wire_payload_len;
|
||||
/* bug-847: generic decoded placeholder for wire field batchPayloads @10 —
|
||||
* same broken-for-raw-bytes limitation as `payload` above; kept NULL, do
|
||||
* not use. Only meaningful on the TOP-LEVEL container (never on a nested
|
||||
* sub-entry). The real bytes are hand-extracted by xce_decode() into
|
||||
* raw_batch_payloads/raw_batch_payloads_len below, then sliced per
|
||||
/* bug-1505 fix: List(UInt8) now, not Data (controller.capnp's
|
||||
* WalCatchupEntry.batchPayloads comment has the full root cause —
|
||||
* same class as `payload` above and SnapshotData.internalState,
|
||||
* bug-1504). Generic decoded field for wire field batchPayloads @10;
|
||||
* genuinely round-trips now via the generic codec, but still unused by
|
||||
* production — the real bytes are hand-extracted by xce_decode() into
|
||||
* raw_batch_payloads/raw_batch_payloads_len below (a separate,
|
||||
* already-correct code path this fix does not touch), then sliced per
|
||||
* sub-entry using each entry's wire_payload_len into that entry's OWN
|
||||
* raw_payload/raw_payload_len (so every consumer keeps reading
|
||||
* raw_payload exactly as before — this is purely a wire-transport fix). */
|
||||
xce_data_t *batch_payloads;
|
||||
* raw_payload exactly as before). Only meaningful on the TOP-LEVEL
|
||||
* container (never on a nested sub-entry). batch_payloads_wire_len is
|
||||
* its own maplistcount companion (wire field batchPayloadsLen @11) —
|
||||
* distinct from raw_batch_payloads_len below, which belongs to the
|
||||
* separate hand-rolled extraction path. */
|
||||
uint8_t *batch_payloads;
|
||||
uint32_t batch_payloads_wire_len;
|
||||
uint8_t *raw_batch_payloads;
|
||||
size_t raw_batch_payloads_len;
|
||||
};
|
||||
|
|
|
|||
|
|
@ -56,16 +56,25 @@ int xce_encode(xce_request_t* req, io_packet_t* pkt)
|
|||
return 1;
|
||||
}
|
||||
|
||||
/* extract_data_bytes: copy a capnp Data (List(UInt8)) field into a fresh
|
||||
/* extract_data_bytes: copy a capnp Data/List(UInt8) field into a fresh
|
||||
* malloc'd buffer. Returns NULL (*out_len = 0) for absent/empty fields or
|
||||
* allocation failure. Used by xce_decode() to pre-extract raw payload
|
||||
* bytes before capn_free() destroys the segment. */
|
||||
static uint8_t *extract_data_bytes(capn_data dp, size_t *out_len)
|
||||
* bytes before capn_free() destroys the segment.
|
||||
*
|
||||
* Takes a raw capn_ptr rather than capn_data/capn_list8 (bug-1505 fix):
|
||||
* both wrapper types are a single `.p` field with identical layout — this
|
||||
* function already only ever touched `.p` — so accepting the shared
|
||||
* underlying type lets every call site pass either wrapper's `.p` directly,
|
||||
* regardless of whether the source schema field is spelled `Data` (still
|
||||
* SnapshotTransfer.payload) or `List(UInt8)` (WalCatchupEntry.payload/
|
||||
* batchPayloads, converted from Data by bug-1505's schema fix — see
|
||||
* controller.capnp's WalCatchupEntry comment for why). */
|
||||
static uint8_t *extract_data_bytes(capn_ptr p, size_t *out_len)
|
||||
{
|
||||
*out_len = 0;
|
||||
|
||||
capn_list8 l;
|
||||
l.p = dp.p;
|
||||
l.p = p;
|
||||
if (l.p.type == CAPN_NULL)
|
||||
return NULL;
|
||||
|
||||
|
|
@ -199,7 +208,7 @@ int xce_decode(io_packet_t* pkt, xce_request_t* req)
|
|||
|
||||
/* Top-level payload (single-entry mode) */
|
||||
batch->raw_payload =
|
||||
extract_data_bytes(WalCatchupEntry_get_payload(b_ptr),
|
||||
extract_data_bytes(WalCatchupEntry_get_payload(b_ptr).p,
|
||||
&batch->raw_payload_len);
|
||||
|
||||
/* bug-847: the top-level batch_payloads blob — every sub-entry's
|
||||
|
|
@ -208,7 +217,7 @@ int xce_decode(io_packet_t* pkt, xce_request_t* req)
|
|||
* exhibit the per-element cross-segment pointer loss that scattered
|
||||
* per-sub-entry Data blobs did. */
|
||||
batch->raw_batch_payloads =
|
||||
extract_data_bytes(WalCatchupEntry_get_batchPayloads(b_ptr),
|
||||
extract_data_bytes(WalCatchupEntry_get_batchPayloads(b_ptr).p,
|
||||
&batch->raw_batch_payloads_len);
|
||||
|
||||
/* ha-term-safety task 3.2: the top-level container's AppendEntries
|
||||
|
|
@ -249,7 +258,7 @@ int xce_decode(io_packet_t* pkt, xce_request_t* req)
|
|||
}
|
||||
} else {
|
||||
batch->entries[i]->raw_payload =
|
||||
extract_data_bytes(we.payload,
|
||||
extract_data_bytes(we.payload.p,
|
||||
&batch->entries[i]->raw_payload_len);
|
||||
}
|
||||
}
|
||||
|
|
@ -263,7 +272,7 @@ int xce_decode(io_packet_t* pkt, xce_request_t* req)
|
|||
snapshot_transfer_t *chunk = req->body.snapshottransfermsg;
|
||||
SnapshotTransfer_ptr s_ptr = rq.body.snapshottransfermsg;
|
||||
chunk->raw_payload =
|
||||
extract_data_bytes(SnapshotTransfer_get_payload(s_ptr),
|
||||
extract_data_bytes(SnapshotTransfer_get_payload(s_ptr).p,
|
||||
&chunk->raw_payload_len);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -954,7 +954,29 @@ struct SnapshotTransfer $C.mapname("snapshot_transfer_t") {
|
|||
struct WalCatchupEntry $C.mapname("wal_catchup_entry_t") {
|
||||
lsn @0 :UInt64 $C.mapname("lsn");
|
||||
entryType @1 :UInt8 $C.mapname("entry_type");
|
||||
payload @2 :Data $C.mapname("payload");
|
||||
# payload is List(UInt8), NOT Data (bug-1505 fix, same class as bug-1504 on
|
||||
# SnapshotData.internalState -- see that struct's own comment for the full
|
||||
# root cause: this codebase's capnpc-c backend silently no-ops every Data
|
||||
# field's encoding in generated encode_XXX() helpers without zero-
|
||||
# initializing the caller's local struct first, leaving a genuinely
|
||||
# uninitialized capn_ptr written onto the wire -- a real, reproducible
|
||||
# SIGSEGV in write_WalCatchupEntry, hit by
|
||||
# test_wal_catchup_append_term_fields_round_trip even though that test
|
||||
# never touches payload content at all (src.payload stays NULL -- the
|
||||
# crash is from the OUTPUT struct's uninitialized field, independent of
|
||||
# the input). List(UInt8) gets a real generated encode/decode from this
|
||||
# same backend (proven precedent: FetchFileReply.content/bytesRead), so
|
||||
# no hand-written accessor is needed. Reuses wirePayloadLen (@9, below)
|
||||
# as its maplistcount companion -- already exactly "this sub-entry's
|
||||
# payload byte count" for the bug-847 slicing use below, same value.
|
||||
# NOTE: production (xce_wal_batch_encode_raw/wal_entry_to_wire, request.c)
|
||||
# never goes through this generic encode_WalCatchupEntry() path at all --
|
||||
# it memset()s its own struct and calls write_WalCatchupEntry() directly
|
||||
# with every field hand-set (payload deliberately left at its post-memset
|
||||
# zero value, per bug-847's own comment below) -- so this fix has zero
|
||||
# production behavior impact, it only stops the generic codec (used by
|
||||
# this one test) from crashing.
|
||||
payload @2 :List(UInt8) $C.mapname("payload") $C.maplistcount("wire_payload_len");
|
||||
entries @3 :List(WalCatchupEntry) $C.mapname("entries") $C.maplistcount("n_entries");
|
||||
# Raft AppendEntries term fields (ha-term-safety task 1.1, Req 1.3/1.4).
|
||||
# This envelope is the replication-carrying message (WAL catch-up); the four
|
||||
|
|
@ -996,8 +1018,25 @@ struct WalCatchupEntry $C.mapname("wal_catchup_entry_t") {
|
|||
# n_entries (0/1/N); sub-entries no longer carry their own @2 payload on
|
||||
# this path (kept for any other/legacy sender; decode still handles it
|
||||
# defensively as a fallback when batchPayloads is absent).
|
||||
#
|
||||
# NOTE (bug-1505 fix, same as `payload` above): this note's own "cross-
|
||||
# segment loss" concern is about scattered PER-ELEMENT Data (or
|
||||
# List(UInt8) -- identical wire representation either way in this
|
||||
# codegen; the Data vs List(UInt8) schema keyword only changes whether
|
||||
# THIS backend's generic encode_XXX() composite helper populates the
|
||||
# field automatically, not the underlying wire type). batchPayloads
|
||||
# itself is the single TOP-LEVEL blob this fix already routes around that
|
||||
# exact problem with -- so switching ITS OWN keyword to List(UInt8) does
|
||||
# not reintroduce bug-847's issue; it only stops the generic codec's
|
||||
# SnapshotData.internalState-class no-op/uninitialized-capn_ptr crash
|
||||
# (bug-1504) for this field too. batchPayloadsLen (@11, below) is its
|
||||
# maplistcount companion, distinct from raw_batch_payloads_len
|
||||
# (typemap.h) -- that field is populated by request.c's OWN hand-rolled
|
||||
# xce_decode() extraction (a completely separate, already-correct code
|
||||
# path this fix does not touch), not by this generic decode.
|
||||
wirePayloadLen @9 :UInt32 $C.mapname("wire_payload_len");
|
||||
batchPayloads @10 :Data $C.mapname("batch_payloads");
|
||||
batchPayloads @10 :List(UInt8) $C.mapname("batch_payloads") $C.maplistcount("batch_payloads_wire_len");
|
||||
batchPayloadsLen @11 :UInt32 $C.mapname("batch_payloads_wire_len");
|
||||
}
|
||||
|
||||
struct CtrlHelloRequest $C.mapname("ctrl_hello_req_t") {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue