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

fix(libxce): bug-1077 ctrlstatus crash from shared reply-union type confusion

Browse source 
ctx->reply.u is a single untagged union shared across opcodes (debugmsg /
get(dataset_t*) / jobid / clusterconf / ctrlstatus alias the same 8 bytes).
Commit 61e1391 added speculative free-guards
`if (ctx->reply.u.ctrlstatus) free_CtrlStatus_ptr(...)` (and the clusterconf
twin). A non-NULL slot does NOT imply a CtrlStatus*: a failover-aborted
reslist/joblist leaves ctx->reply.u.get == &getreply (a dead STACK pointer)
because xce_get_with_condition/xce_resval only cleared the slot on the success
path, not the xce_get_req<0 / cbuf_get<0 early returns. The next ctrlstatus
reply (worker thread) freed that foreign pointer as a CtrlStatus -> ASan
SEGV / stack-use-after-return in free_CtrlStatus (controller.capnp.c:11638).

Fix (root cause, both layers):
- Revert the unsafe free-guards in xce_handle_reply: MOVE_PTR overwrite only,
  never free/deref an untagged union slot by type.
- Clear ctx->reply.u.get = NULL on ALL exit paths of xce_get_with_condition
  and xce_resval via a single `goto done` cleanup (forget-proof; the bug was
  one forgotten failover path). Also drops the now-unused controller.capnp.h.
- Defense-in-depth: handle_get_reply drops a reply when the slot is NULL.

Regression: expose xce_handle_reply and add
test_handle_reply_ctrlstatus_does_not_free_foreign_slot, which poisons the
shared slot with a foreign heap value and proves the ctrlstatus reply handler
does not free it (RED reproduced the exact free_CtrlStatus_ptr double-free;
GREEN after fix). xcelib_test 244/0, controller_unit_test 166/0, all binaries
build clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MSxwb2Xt5F84ATgiAZnH8V
This commit is contained in:
Rongsong Shen 2026-06-24 09:44:43 +00:00
parent 61e1391cd0
commit 8da8fd03dd
7 changed files with 943 additions and 767 deletions
Download patch file Download diff file Expand all files Collapse all files

59
src/lib/testsuite/test_ctrlstatus.c
View file

@ -24,6 +24,7 @@
#include "config.h" /* XCE_MAXHOSTNAME_LEN */
#include "request.h"
#include "xce/types.h"
#include "xcectx.h" /* xce_ctx_t, xce_handle_reply (bug-1077 regression) */
/* generated Cap'n Proto codec for CtrlStatus / CtrlPeer */
#include "controller.capnp.h"
@ -331,10 +332,68 @@ static void test_ctrlstatus_primary_addr_unknown_or_missing(void)
printf("test_ctrlstatus_primary_addr_unknown_or_missing: PASS\n");
}
/* ── reply-union type-confusion regression (bug-1077) ───────────────────────
*
* ctx->reply.u is a single untagged union shared by every opcode (debugmsg /
* get / jobid / clusterconf / ctrlstatus all alias the same 8 bytes). A
* failover-aborted reslist/joblist leaves ctx->reply.u.get pointing at the
* caller's (now-dead) stack `getreply`. When the next ctrlstatus reply lands on
* a worker thread, handle_reply must NOT treat that foreign, non-NULL slot as a
* CtrlStatus* and free it — doing so dereferenced a dead stack address inside
* free_CtrlStatus and crashed (ASan stack-use-after-return at
* controller.capnp.c:11638).
*
* This drives xce_handle_reply directly with a poisoned union slot and proves
* the foreign occupant is left untouched (no free, no deref). Pre-fix, the
* ctrlstatus branch frees the sentinel and the explicit free() below is a
* double-free that ASan aborts on. */
static void test_handle_reply_ctrlstatus_does_not_free_foreign_slot(void)
{
xce_ctx_t ctx;
memset(&ctx, 0, sizeof(ctx));
/* A prior get/reslist leaked a non-CtrlStatus pointer into the shared slot.
* Sized as a zeroed xce_ctrlstatus_t so that, even on the buggy path,
* free_CtrlStatus(sentinel) only walks all-NULL fields (no wild deref) and
* the regression surfaces as a clean double-free rather than a hard crash. */
xce_ctrlstatus_t* sentinel = (xce_ctrlstatus_t*)calloc(1, sizeof(*sentinel));
TEST_ASSERT_NOT_NULL(sentinel);
ctx.reply.u.get = (dataset_t*)sentinel; /* same 8 bytes as u.ctrlstatus */
/* A genuine ctrlstatus reply arrives for this ctx. */
xce_ctrlstatus_t* cs = (xce_ctrlstatus_t*)calloc(1, sizeof(*cs));
TEST_ASSERT_NOT_NULL(cs);
cs->clusterName = strdup("eda1");
xce_reply_t reply;
memset(&reply, 0, sizeof(reply));
reply.opcode = OPCode_ctrlstatus;
reply.status = 0;
reply.data_which = RequestReply_data_ctrlstatus;
reply.data.ctrlstatus = cs;
int rc = xce_handle_reply(&ctx, &reply);
/* The genuine reply is now owned by the ctx (MOVE_PTR nulls the source) … */
TEST_ASSERT_EQUAL_INT(1, rc);
TEST_ASSERT_EQUAL_PTR(cs, ctx.reply.u.ctrlstatus);
TEST_ASSERT_NULL(reply.data.ctrlstatus);
/* … and the foreign sentinel was NOT freed by the reply handler. Freeing it
* here is the sole owner's free; on the buggy code it is the second free. */
free(sentinel);
free_CtrlStatus(cs);
free(cs);
printf("test_handle_reply_ctrlstatus_does_not_free_foreign_slot: PASS\n");
}
/* ── Suite entry point ──────────────────────────────────────────────────── */
void test_ctrlstatus(void)
{
RUN_TEST(test_handle_reply_ctrlstatus_does_not_free_foreign_slot);
RUN_TEST(test_ctrlstatus_roundtrip_all_fields);
RUN_TEST(test_ctrlstatus_roundtrip_unknown_primary);
RUN_TEST(test_xcecli_ctrlstatus_null_ctx_typed_failure);

19
src/lib/xceapi.c
View file

@ -106,16 +106,16 @@ int xce_get_with_condition(xce_ctx_t* ctx, xce_get_t* get, void*** data)
}
memset(&getreply, 0x0, sizeof(getreply));
ctx->reply.u.get = &getreply;
rc = xce_get_req(ctx->ep, get);
if (rc < 0) {
return rc;
goto done;
}
rc = cbuf_get(&(ctx->wbuf), &status);
if (rc < 0) {
return rc;
goto done;
}
if (status >= 0) {
@ -125,6 +125,12 @@ int xce_get_with_condition(xce_ctx_t* ctx, xce_get_t* get, void*** data)
rc = status;
}
done:
/* Always clear the destination — including on the xce_get_req/cbuf_get
* failover error paths — so the single, untagged reply union never retains
* the address of this now-returning stack `getreply`. A later ctrlstatus
* reply would otherwise free that dead-stack pointer as a CtrlStatus*
* (bug-1077, ASan stack-use-after-return). */
ctx->reply.u.get = NULL;
return rc;
@ -189,12 +195,12 @@ int xce_resval(xce_ctx_t* ctx, char* host, char* res, xce_resval_t*** vals)
ctx->reply.u.get = &getreply;
rc = xce_get_req(ctx->ep, &get);
if (rc < 0) {
return rc;
goto done;
}
rc = cbuf_get(&(ctx->wbuf), &status);
if (rc < 0) {
return rc;
goto done;
}
if (status >= 0) {
@ -204,6 +210,9 @@ int xce_resval(xce_ctx_t* ctx, char* host, char* res, xce_resval_t*** vals)
rc = status;
}
done:
/* Clear on every path (incl. the failover error returns) so the shared
* reply union never keeps this stack `getreply` address — bug-1077. */
ctx->reply.u.get = NULL;
return rc;

37
src/lib/xcectx.c
View file

@ -7,7 +7,6 @@
#include "netio.h"
#include "typemap.h"
#include "utils.h"
#include "controller.capnp.h" /* free_CtrlStatus_ptr, free_ClusterConf_ptr */
#include "xcectx.h"
static int handle_get_reply(xce_ctx_t* ctx, xce_reply_t* reply)
@ -15,6 +14,14 @@ static int handle_get_reply(xce_ctx_t* ctx, xce_reply_t* reply)
xce_data_t* getdata = reply->data.getdata;
dataset_t* dataset = ctx->reply.u.get;
/* No get is in flight (slot cleared by the consumer on every exit path):
* drop a late/duplicate reply rather than dereference a NULL or stale
* destination — bug-1077, the shared reply union must never be used as a
* stale typed pointer. */
if (dataset == NULL) {
return -1;
}
return extract_data(dataset, getdata);
}
@ -47,7 +54,10 @@ void xce_ctx_capture_redirect_hint(xce_ctx_t* ctx, const char* host,
ctx->redirect_hint_port = port;
}
static int handle_reply(xce_ctx_t* ctx, xce_reply_t* reply)
/* Exposed (non-static) so the libxce test suite can drive the reply path
* directly without a live endpoint (see test_handle_reply_* regression for the
* shared-reply-union type-confusion crash, bug-1077). */
int xce_handle_reply(xce_ctx_t* ctx, xce_reply_t* reply)
{
int rc = 0;
@ -72,14 +82,13 @@ static int handle_reply(xce_ctx_t* ctx, xce_reply_t* reply)
case OPCode_clusterconf:
rc = -1;
if (reply->data_which == RequestReply_data_clusterconf) {
/* Free a prior unconsumed reply before storing the new one.
* The consumer xce_clusterconf() MOVE_PTRs out, so in the normal
* single-request-per-ctx path this is a no-op; on a re-stolen slot
* (reply decode on a stale fd after client rotation) it prevents
* a permanent heap leak of the prior occupant. */
if (ctx->reply.u.clusterconf) {
free_ClusterConf_ptr(&ctx->reply.u.clusterconf);
}
/* MOVE_PTR overwrites the slot WITHOUT dereferencing the old value.
* Do NOT "free the prior occupant": ctx->reply.u is a single
* untagged union shared by every opcode, so a non-NULL slot does NOT
* imply a ClusterConf* — it may hold a dataset_t* (a dead stack
* `getreply` left by a failover-aborted get) or a jobid. Freeing it
* as a ClusterConf walked a foreign pointer and crashed (bug-1077).
* The consumer owns slot lifecycle and clears it on every path. */
MOVE_PTR(ctx->reply.u.clusterconf, reply->data.clusterconf);
rc = 1;
}
@ -87,9 +96,9 @@ static int handle_reply(xce_ctx_t* ctx, xce_reply_t* reply)
case OPCode_ctrlstatus:
rc = -1;
if (reply->data_which == RequestReply_data_ctrlstatus) {
if (ctx->reply.u.ctrlstatus) {
free_CtrlStatus_ptr(&ctx->reply.u.ctrlstatus);
}
/* See the clusterconf note above: never free the shared union slot
* here as a CtrlStatus* — a foreign occupant would be type-confused
* and dereferenced (bug-1077, ASan stack-use-after-return). */
MOVE_PTR(ctx->reply.u.ctrlstatus, reply->data.ctrlstatus);
rc = 1;
}
@ -128,7 +137,7 @@ static int vtask_entry(vtask_t* task)
int need_wake = 1;
int status = request.body.replymsg->status;
need_wake = handle_reply(ctx, request.body.replymsg);
need_wake = xce_handle_reply(ctx, request.body.replymsg);
if (need_wake) {
rc = xce_wakeup(ctx, (int)status);
}