diff --git a/src/controller/cluster.c b/src/controller/cluster.c index 076cab2..cb24ec8 100644 --- a/src/controller/cluster.c +++ b/src/controller/cluster.c @@ -1439,6 +1439,16 @@ int cluster_start(cluster_t* c) int cluster_run(cluster_t* c) { + /* c->running is the controller's liveness flag: 1 while the run loop is + * active, 0 once a shutdown request breaks the loop below. check_controller() + * (the streaming gate for joblist/workerlist replies in xce_reply_get) treats + * !running as "shutting down" and zeroes the reply chunk. The legacy + * single-node bring-up set this in cluster_start(), but cluster_start is + * orphaned (not on the HA myapp_run path), so it was never set on HA nodes — + * leaving joblist/workerlist permanently empty on the primary. Own the whole + * flag lifecycle here, symmetric with the `c->running = 0` on shutdown. */ + c->running = 1; + for (;;) { int stop; diff --git a/src/lib/request.c b/src/lib/request.c index 75a687f..888ca02 100644 --- a/src/lib/request.c +++ b/src/lib/request.c @@ -240,6 +240,14 @@ int xce_decode(io_packet_t* pkt, xce_request_t* req) void init_reply_by_req(xce_reply_t* reply, xce_request_t* req) { + /* Zero the whole reply first. Callers declare `xce_reply_t reply;` on the + * stack and only populate the fields they need (status / data_which / data), + * leaving the common trailer fields (redirect_host, redirect_port) untouched. + * The generated encode_RequestReply reads redirect_host unconditionally + * (gated only on != NULL), so a stale stack value there is dereferenced via + * strlen() — a heap-use-after-free when the garbage aliases a freed capnp + * segment. Clearing here makes every reply path safe by construction. */ + memset(reply, 0, sizeof(*reply)); reply->seq = req->seq; reply->opcode = req->opcode; } diff --git a/src/lib/testsuite/test_request.c b/src/lib/testsuite/test_request.c index caf5eef..46d8070 100644 --- a/src/lib/testsuite/test_request.c +++ b/src/lib/testsuite/test_request.c @@ -65,6 +65,34 @@ static void test_hostctrl_req_reject_negative_action(void) TEST_ASSERT_EQUAL_INT(-EINVAL, rc); } +/* init_reply_by_req must fully zero the reply so the generated + * encode_RequestReply never reads a stale `redirect_host` pointer (and the + * other common trailer fields) off an uninitialized stack reply. Regression + * guard for the heap-use-after-free in encode_RequestReply / strlen seen on a + * NOP/state-report reply whose redirect_host was stack garbage. */ +static void test_init_reply_by_req_zeroes_reply(void) +{ + /* Poison the whole reply with non-zero bytes — mirrors an uninitialized + * stack reply whose redirect_host happens to be a (dangling) non-NULL. */ + xce_reply_t reply; + xce_request_t req = { .seq = 42, .opcode = OPCode_nop }; + memset(&reply, 0xFF, sizeof(reply)); + + init_reply_by_req(&reply, &req); + + /* seq/opcode carried from the request */ + TEST_ASSERT_EQUAL_UINT64(42, reply.seq); + TEST_ASSERT_EQUAL_INT(OPCode_nop, reply.opcode); + + /* Trailer/common fields the encoder always reads must be cleared so a + * handler that only sets data_which/data cannot leak garbage onto the wire + * (or dereference a dangling redirect_host). */ + TEST_ASSERT_NULL(reply.redirect_host); + TEST_ASSERT_EQUAL_UINT(0, reply.redirect_port); + TEST_ASSERT_EQUAL_INT(0, reply.status); + TEST_ASSERT_EQUAL_INT(0, reply.data_which); +} + void test_request(void) { printf("\n>> host control request validation tests\n\n"); @@ -76,4 +104,5 @@ void test_request(void) RUN_TEST(test_hostctrl_req_reject_tag); RUN_TEST(test_hostctrl_req_reject_untag); RUN_TEST(test_hostctrl_req_reject_negative_action); + RUN_TEST(test_init_reply_by_req_zeroes_reply); }