wdgzyjx/myxce
1
0
Fork 0
forked from lei.chen/myxce
Code Pull requests Activity

fix(controller): persist_job_hook_fun OOB read on RUSAGE_UPDATE hook (bug-1502)

Browse source 
The RUSAGE_UPDATE job hook fires with (cluster_t*, xce_job_rusages_t*) as
its varargs — the apiserver and scheduler subscribers parse exactly that
contract — but persist_job_hook_fun's default case treated the second arg
as a job_t* and read j->job.state at offset 56. A xce_job_rusages_t is 56
bytes and shares only jobid@0 with xce_job_t (which is why the jobid read
coincidentally worked), so the state read landed one byte past the struct:
ASAN heap-buffer-overflow (READ of size 4) in thread T7 whenever a job runs
and the worker pushes rusage updates. This was the recurring controller
crash (term 3/4/5) blocking job-PENDING verification.

Add a dedicated HOOK_DETAIL_JOB_RUSAGE_UPDATE case that reads jobid from
the xce_job_rusages_t* and bakes the job's current state from a jobmgr
key_find on the vararg cluster_t* (race-free: the sole caller
update_job_rusages() holds that job's write lock while firing the
AFTER_EVENT hook; re-locking the job inside the hook would deadlock).

Regression test: test_job_hook_rusage_writes_wal_entry now passes a real
56-byte xce_job_rusages_t instead of a full job_t (which masked the
overflow) — ASAN stack-buffer-overflow under the old code, PASS under the
fix. Full controller unit suite: 458 tests, 0 failures.

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
lei.chen 2026-08-04 09:39:57 +00:00
parent ff2d784ca9
commit 2fa9bedcd3
4 changed files with 61 additions and 2 deletions
Download patch file Download diff file Expand all files Collapse all files

29
src/controller/persist.c
View file

@ -6573,8 +6573,35 @@ int persist_job_hook_fun(int type, int situation, int detail,
js.jobid = j->job.jobid; js.state = (uint32_t)j->job.state;
break;
}
case HOOK_DETAIL_JOB_RUSAGE_UPDATE: {
/* args: cluster_t*, xce_job_rusages_t* — NOT job_t* (bug-1502).
* The RUSAGE_UPDATE hook carries the rusage struct, which has no job
* state (apiserver_job_rusage_update_fun / sched_job_rusage_update_fun
* parse this exact contract). The old default-case path treated the
* 2nd arg as a job_t* and read j->job.state at offset 56 — one byte
* past a 56-byte xce_job_rusages_t → ASAN heap-buffer-overflow (the
* only field the two structs share is jobid@0, so that read
* coincidentally worked). Bake the job's CURRENT state from the
* jobmgr: the sole caller update_job_rusages() fires this AFTER_EVENT
* while holding the job's write lock, so the pointer returned here IS
* that same write-locked object and reading its state is race-free.
* Never re-lock the job inside a hook — the caller already holds it,
* and a same-thread re-lock would deadlock. */
cluster_t* c = va_arg(ap, cluster_t*);
xce_job_rusages_t* r = va_arg(ap, xce_job_rusages_t*);
js.jobid = r->jobid;
if (c != NULL && c->jobmgr != NULL) {
job_t* jj = NULL;
lock(c->jobmgr, RTTI_LOCK_READ);
key_find(c->jobmgr, (void*)r->jobid, (void**)&jj);
unlock(c->jobmgr);
if (jj != NULL)
js.state = (uint32_t)jj->job.state;
}
break;
}
default: {
/* STOP, RESUME, UPDATE, CKPT, PREASON, RUSAGE — args: cluster_t*, job_t* */
/* STOP, RESUME, UPDATE, CKPT, PREASON — args: cluster_t*, job_t* */
(void)va_arg(ap, cluster_t*);
job_t* j = va_arg(ap, job_t*);
js.jobid = j->job.jobid; js.state = (uint32_t)j->job.state;

13
src/controller/testsuite/test_persist_hooks.c
View file

@ -14,6 +14,7 @@
#include "persist.h"
#include "xce/cluster_def.h"
#include "xlist.h"
#include "controller.capnp.h" /* xce_job_rusages_t (bug-1502 regression test) */
/* ------------------------------------------------------------------ */
/* Test helpers */
@ -276,9 +277,19 @@ static void test_job_hook_rusage_writes_wal_entry(void)
uint64_t lsn_before = p->current_lsn;
/* bug-1502 regression: production fires RUSAGE_UPDATE with a real
* xce_job_rusages_t* (56 bytes, NO job-state field) as the second hook
* arg — NOT a job_t*. The old persist_job_hook_fun default case read
* j->job.state at offset 56, one byte past this struct → ASAN
* heap-buffer-overflow. Passing the real minimal struct here (instead of
* the old &g_stub_job, a full job_t that masked the overflow) reproduces
* the production shape exactly. */
xce_job_rusages_t rusages = {0};
rusages.jobid = 0x1234567890ABCDEFULL;
int rc = call_job_hook(HOOK_AFTER_EVENT, HOOK_AFTER_EVENT,
HOOK_DETAIL_JOB_RUSAGE_UPDATE, p,
(cluster_t*)NULL, &g_stub_job);
(cluster_t*)NULL, &rusages);
TEST_ASSERT_EQUAL_INT(0, rc);
TEST_ASSERT_GREATER_THAN(lsn_before, p->current_lsn);