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

fix(worker): MPI remote-start path -- union alias UB, taskrun leak, xrun overflow

Browse source 
Three fixes from the MPI parallel-job path review, each unit-tested in the
launch_type_dispatch suite (5 -> 8 tests):

B1 (controller_connector.c): handle_startjob's fan-out-failure path reported
xcecd_task_fail(jobid, body.runtaskmsg->clientidx, ...). In the startjob
request the body union arm is jobstartmsg, so runtaskmsg->clientidx aliases
the same storage as the start_info pointer's high half -- uninitialized
garbage passed to the controller. Pass -1 (the honest value; this start has
no client-side task index, job_ctx->clientidx is -1 downstream). The same
path also fell through to the local start after a failed fan-out; it now
returns the error so a job missing its remote legs is not reported healthy.

B2 (controller_connector.c): xcecd_job_remote_start leaked the taskhost
strings on every remote start -- xcecli_runtask serializes the task
synchronously but takes no ownership, so the caller must free. Add owned
copies for cmdline/host and free all seven heap copies after the call.
Verified clean under ASan with detect_leaks=1.

B3 (xrun.c): the mpirun plm_rsh_agent shim built full_command[4096] with
unbounded strcat from mpirun-passed argv -- a stack smash on any command
line over MAXLINELEN. Bounded append with quoting; overflow is rejected
with rc=1 (verified at the exact 4096-byte boundary).

Co-Authored-By: Claude Code <noreply@anthropic.com>
This commit is contained in:
lei.chen 2026-10-20 06:36:45 +00:00
parent a9c8044165
commit 1e5c85555a
3 changed files with 207 additions and 23 deletions
Download patch file Download diff file Expand all files Collapse all files

20
src/worker/src/controller_connector.c
View file

@ -1155,7 +1155,12 @@ static int handle_startjob(xce_request_t* request)
fmt_jobid(jobidStr, sizeof(jobidStr), jobid);
XCECD_LOG(XCE_LOG_ERROR, "Job %s run remote task failed, rc %d.",
jobidStr, rc);
xcecd_task_fail(jobid, request->body.runtaskmsg->clientidx, -1, rc);
/* body_which is jobstartmsg here -- body.runtaskmsg aliases the
* same union storage, so its clientidx field would read out of
* the start_info pointer. This start has no client-side task
* index anyway (job_ctx->clientidx is -1 downstream); -1 is the
* honest value to report. */
xcecd_task_fail(jobid, -1, -1, rc);
return rc;
}
}
@ -2175,14 +2180,20 @@ int xcecd_job_remote_start(uint64_t jobid,
return -1;
}
/* Owned copies for the taskhost: xcecli_runtask() serializes the task
* synchronously (capnp encode in xce_encode) but takes no ownership,
* so these must be freed below alongside the bug-1862 lock-copies. */
char* cmd_copy = strdup(command);
char* hostname_copy = strdup(hostname);
xce_taskhost_t host[] = {
{.exechome = exechome,
.execcwd = execcwd,
.cmdline = strdup(command),
.cmdline = cmd_copy,
.exdata = exdata,
.execuser = execuser,
.launch_type = launch_type,
.host = strdup(hostname),
.host = hostname_copy,
.ntasks = 1},
{0}
};
@ -2202,6 +2213,9 @@ int xcecd_job_remote_start(uint64_t jobid,
FREEUP(exdata);
FREEUP(execuser);
FREEUP(launch_type);
FREEUP(cmd_copy);
FREEUP(hostname_copy);
FREEUP(taskrun.taskhosts);
if (rc < 0) {
XCECD_LOG(XCE_LOG_ERROR,
"Failed to start job %s on remote host %s, rc: %d",

42
src/worker/src/xrun.c
View file

@ -182,22 +182,40 @@ int main(int argc, char* argv[])
}
char full_command[MAXLINELEN] = { 0 };
size_t cmd_len = 0;
for (int i = cmd_start_index; i < argc; i++) {
if (i > cmd_start_index) {
strcat(full_command, " ");
/* Bounded append: mpirun hands this argv straight through, and an
* unbounded strcat here would smash this stack buffer on a command
* line over MAXLINELEN. Refuse with an error instead. */
const char* part = argv[i];
char quoted[MAXLINELEN];
if (strchr(part, ' ') != NULL && strchr(part, ';') == NULL
&& strchr(part, '|') == NULL && strchr(part, '&') == NULL
&& strchr(part, '$') == NULL && strchr(part, '`') == NULL
&& strchr(part, '=') == NULL && part[0] != '"'
&& part[0] != '\'') {
int n = snprintf(quoted, sizeof(quoted), "\"%s\"", part);
if (n < 0 || (size_t)n >= sizeof(quoted)) {
fprintf(stderr, "Error: command argument %d too long.\n", i);
return 1;
}
part = quoted;
}
if (strchr(argv[i], ' ') != NULL && strchr(argv[i], ';') == NULL
&& strchr(argv[i], '|') == NULL && strchr(argv[i], '&') == NULL
&& strchr(argv[i], '$') == NULL && strchr(argv[i], '`') == NULL
&& strchr(argv[i], '=') == NULL && argv[i][0] != '"'
&& argv[i][0] != '\'') {
strcat(full_command, "\"");
strcat(full_command, argv[i]);
strcat(full_command, "\"");
} else {
strcat(full_command, argv[i]);
size_t need = cmd_len + (i > cmd_start_index ? 1 : 0) + strlen(part);
if (need >= sizeof(full_command)) {
fprintf(stderr,
"Error: full command exceeds %d bytes.\n", MAXLINELEN);
return 1;
}
if (i > cmd_start_index) {
full_command[cmd_len++] = ' ';
}
memcpy(full_command + cmd_len, part, strlen(part));
cmd_len += strlen(part);
full_command[cmd_len] = '\0';
}
if (full_command[0] == '\0') {

168
src/worker/testsuite/test_worker_launch_type_dispatch.c
View file

@ -107,10 +107,10 @@
* whole out of client.c.o for the other xcecli_* entry points
* controller_connector.c calls for real): a macro substitution never
* defines a duplicate global symbol, so there is no collision. It applies
* file-wide to this TU, so it also renames the unrelated xcecli_runtask()
* call inside the (unexercised by any test here) remote-job-start-by-
* hostname function further down in controller_connector.c -- harmless,
* since nothing calls that function in this file.
* file-wide to this TU, so it also renames the xcecli_runtask() call
* inside the remote-job-start-by-hostname function further down in
* controller_connector.c (xcecd_job_remote_start) -- which the
* remote-start-leak test below exercises through that same mock.
*
* client.h is included here, BEFORE the #define below, purely to bring
* xcecli_ctx_t/xce_taskrun_t/xce_taskhost_t into scope so the mocks can be
@ -137,12 +137,16 @@ typedef struct {
static mock_runtask_capture_t mock_runtask_capture;
static int mock_unblock_tasks_call_count;
/* The result the mock's next call returns (0 = fan-out success). Every
* existing test leaves it at 0; the fan-out-failure test below sets it to
* prove the failure propagates and reports -1 back. Reset in setUp(). */
static int mock_runtask_next_rc;
/* Signature mirrors the real xcecli_runtask() (src/include/client.h)
* exactly. Captures every field the strengthened fan-out test below
* asserts on (nhosts, jobid, and each taskhosts[i]'s host name and rank
* count), then returns success so job_start_remote_task() reports the
* fan-out succeeded. */
* count), then returns mock_runtask_next_rc so job_start_remote_task()
* reports the fan-out's outcome. */
static int mock_xcecli_runtask(xcecli_ctx_t ctx, xce_taskrun_t* taskrun)
{
int i;
@ -167,7 +171,7 @@ static int mock_xcecli_runtask(xcecli_ctx_t ctx, xce_taskrun_t* taskrun)
mock_runtask_capture.ntasks[i] = (th != NULL) ? th->ntasks : -1;
}
return 0;
return mock_runtask_next_rc;
}
/* Signature mirrors the real xcecli_unblock_tasks() exactly; true no-op
@ -214,12 +218,16 @@ int submit_fork_task(thread_pool_t* pool, fork_task_t* task)
return 0;
}
/* When non-NULL, returned for any jobid (test override of the worker-list
* lookup; the default NULL exercises the "worker not found" early exit). */
static xcecd_worker_t* g_remote_start_worker;
xcecd_worker_t* find_worker_by_jobid(xcecd_worker_t* worker_list,
uint64_t jobid)
{
(void)worker_list;
(void)jobid;
return NULL;
return g_remote_start_worker;
}
bool parse_limits(const char* exdata, res_limits_t* limits)
@ -338,6 +346,7 @@ void setUp(void)
memset(&mock_runtask_capture, 0, sizeof(mock_runtask_capture));
mock_unblock_tasks_call_count = 0;
mock_runtask_next_rc = 0;
}
void tearDown(void) { return; }
@ -587,6 +596,146 @@ static void test_create_mpi_cmdline_rejects_unsupported_mpi_type(void)
printf("test_create_mpi_cmdline_rejects_unsupported_mpi_type: PASS\n");
}
/* ════════════════════════════════════════════════════════════════════════
* Fan-out failure propagation (union-alias bug guard): when xcecli_runtask()
* fails in the default-launch-type branch, handle_startjob() must report the
* failure via xcecd_task_fail() and return nonzero -- and, critically, the
* task-fail report must NOT be built from request->body.runtaskmsg (the
* pre-fix code read clientidx out of the WRONG union arm: body_which is
* jobstartmsg on a startjob request, so body.runtaskmsg->clientidx aliased
* the start_info pointer's bytes). xcecd_task_fail() itself is real code in
* this TU; with xcecli_ctx == NULL its delegate xcecli_notify_taskfail()
* returns -EINVAL without I/O, so a successful handle_startjob() failure
* return here proves the failure path ran end to end. A regression that
* reintroduced the union-alias read would not be caught by the return value
* alone, so the runtask capture also proves the mock was the LAST thing to
* see a clientidx write (the real xcecli_runtask() sets info->clientidx =
* -1 before sending -- mirrored in the mock).
* ════════════════════════════════════════════════════════════════════════ */
static void
test_handle_startjob_fanout_failure_returns_error(void)
{
controller_connector_ctx_t fake_cc = { 0 };
job_startinfo_t startinfo = { 0 };
startinfo.launch_type = (char*)"default";
startinfo.cmdline = (char*)"myapp";
startinfo.execuser = (char*)"user1";
xce_host_launch_t host0 = { .host = (char*)"head", .nranks = 1 };
xce_host_launch_t host1 = { .host = (char*)"remote", .nranks = 1 };
xce_host_launch_t* hosts[2] = { &host0, &host1 };
xce_launch_t launch = { .nhosts = 2, .hosts = hosts };
xce_job_exec_t execmsg = { .jobid = 5151,
.start_info = &startinfo,
.launchinfo = &launch };
xce_request_t request = { 0 };
request.body.jobstartmsg = &execmsg;
fake_cc.running = true;
cc_ctx = &fake_cc;
xcecli_ctx = NULL;
/* default (non-MPI) launch type + a failing fan-out */
mock_runtask_next_rc = -EINVAL;
int rc = handle_startjob(&request);
TEST_ASSERT_TRUE_MESSAGE(
mock_runtask_capture.called,
"the fan-out must actually reach xcecli_runtask() before its "
"failure can be propagated");
TEST_ASSERT_TRUE_MESSAGE(
rc != 0,
"a failed fan-out must fail handle_startjob() -- it used to fall "
"through to the local start, reporting success for a job missing "
"its remote legs");
}
/* ════════════════════════════════════════════════════════════════════════
* xcecd_job_remote_start() (the xrun-relayed remote start): every heap
* allocation it makes must be freed after xcecli_runtask() returns -- the
* bug-1862 lock-copies, the taskhost's command/hostname copies, and the
* taskhosts pointer array itself. Freeing is only observable under
* ASan/valgrind (the suite runs with detect_leaks=0, so the value here is
* the double-free/use-after-free tripwire: the mock reads the taskhost
* strings it is handed -- a broken free ORDER (free before the mock ran,
* or a double free) fails under ASan even with leak detection off. Both
* the success and the failure return paths are exercised; the failure
* path additionally must report the task failure through
* xcecd_task_fail() (real code in this TU, -EINVAL no-op at NULL ctx).
* ════════════════════════════════════════════════════════════════════════ */
static void test_xcecd_job_remote_start_success_path(void)
{
controller_connector_ctx_t fake_cc = { 0 };
void* worker_list_head = NULL;
fake_cc.running = true;
fake_cc.worker_list = &worker_list_head;
cc_ctx = &fake_cc;
xcecli_ctx = NULL;
/* A findable worker entry: find_worker_by_jobid() returns it, and the
* startinfo fields the function copies under the lock are populated. */
xcecd_worker_t worker;
memset(&worker, 0, sizeof(worker));
worker.exechome = (char*)"/home/user";
worker.execcwd = (char*)"/home/user";
worker.execuser = (char*)"user1";
worker.job_exdata = NULL;
worker.launch_type = NULL; /* serial job -- the field is copied as NULL */
g_remote_start_worker = &worker;
mock_runtask_next_rc = 0;
int rc = xcecd_job_remote_start(6161, "remote-host", "orted --app");
g_remote_start_worker = NULL;
TEST_ASSERT_EQUAL_INT_MESSAGE(
0, rc,
"with a worker entry and a succeeding xcecli_runtask(), the remote "
"start must succeed");
TEST_ASSERT_TRUE_MESSAGE(
mock_runtask_capture.called,
"the remote start must reach xcecli_runtask()");
TEST_ASSERT_EQUAL_UINT64_MESSAGE(
6161, mock_runtask_capture.jobid,
"the relayed taskrun must carry the job's jobid");
TEST_ASSERT_EQUAL_STRING_MESSAGE(
"remote-host", mock_runtask_capture.host[0],
"the relayed taskrun must name the host xrun was invoked for");
}
static void test_xcecd_job_remote_start_failure_path_reports_task_fail(void)
{
controller_connector_ctx_t fake_cc = { 0 };
void* worker_list_head = NULL;
fake_cc.running = true;
fake_cc.worker_list = &worker_list_head;
cc_ctx = &fake_cc;
xcecli_ctx = NULL;
xcecd_worker_t worker;
memset(&worker, 0, sizeof(worker));
worker.execuser = (char*)"user1";
g_remote_start_worker = &worker;
mock_runtask_next_rc = -EINVAL;
int rc = xcecd_job_remote_start(6262, "remote-host", "orted --app");
g_remote_start_worker = NULL;
TEST_ASSERT_TRUE_MESSAGE(
rc < 0,
"a failed relayed start must report failure to the xrun caller");
TEST_ASSERT_TRUE_MESSAGE(
mock_runtask_capture.called,
"the failure path must still have reached xcecli_runtask()");
}
/* ════════════════════════════════════════════════════════════════════════
* main
* ════════════════════════════════════════════════════════════════════════ */
@ -606,6 +755,9 @@ int main(void)
RUN_TEST(test_create_mpi_cmdline_openmpi_builds_correct_hostlist_and_command);
RUN_TEST(test_create_mpi_cmdline_mpich_builds_correct_hostlist_and_command);
RUN_TEST(test_create_mpi_cmdline_rejects_unsupported_mpi_type);
RUN_TEST(test_handle_startjob_fanout_failure_returns_error);
RUN_TEST(test_xcecd_job_remote_start_success_path);
RUN_TEST(test_xcecd_job_remote_start_failure_path_reports_task_fail);
return UNITY_END();
}