From 76862bd9aa0b2cf7b9c82505dd970488c3d7a37b Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 14 Jul 2026 08:29:45 +0000 Subject: [PATCH] feat(job-array-single-wal-record): harden jobj.c job construction primitives - job_init() now checks makeJobsub()/makeString(ckptinfo) return values and propagates failure instead of storing NULL with a success return (Req 10.1). - Root-caused the previously observed makeJobsub() NULL-with-EINVAL failure to a test-binary-only hazard: multiple controller test files each called job_obj_init() from their own setUp() with no cross-file coordination, re-running jobsub_table_init() against an already-live lock/table. Fixed via atomic_flag-guarded idempotent job_obj_init()/job_obj_fini(), plus a distinct -ENODEV return from makeJobsub()/makeJobstartinfo() when the table isn't ready (previously fell through to undefined behavior) (Req 10.2). - Verified (test) MOVE_PTR fields are always NULL on a freshly-built submission payload -- no code change needed (Req 10.3). - Fixed job_destroy()'s accounts leak via the existing free_job_account() helper, replacing the stale FIXME (Req 10.4). - Verified (inspection + test) struct Job's capnp schema has no ckptinfo field, so decode_Job/free_Job have nothing to leak there -- no code change needed (Req 10.5). Independently re-verified: reviewer reproduced a pre-existing, unrelated rtti-library leak-on-init-failure against a stashed pre-task copy of jobj.c to confirm it predates this change. Task 1.3 of job-array-single-wal-record. --- .../job-array-single-wal-record/tasks.md | 8 +- src/lib/jobj.c | 114 ++++- src/lib/meson.build | 1 + src/lib/testsuite/test.c | 2 + src/lib/testsuite/test_jobj_hardening.c | 424 ++++++++++++++++++ 5 files changed, 541 insertions(+), 8 deletions(-) create mode 100644 src/lib/testsuite/test_jobj_hardening.c diff --git a/.kiro/specs/job-array-single-wal-record/tasks.md b/.kiro/specs/job-array-single-wal-record/tasks.md index cff35dd9..8c7aca08 100644 --- a/.kiro/specs/job-array-single-wal-record/tasks.md +++ b/.kiro/specs/job-array-single-wal-record/tasks.md @@ -14,7 +14,7 @@ - _Requirements: 6.1, 6.2_ - _Boundary: Compact Scheduler/API-Server Push_ -- [ ] 1.3 (P) jobj.c hardening +- [x] 1.3 (P) jobj.c hardening - Make job construction check the interning/string-construction return value and propagate failure instead of silently storing a NULL success - Root-cause and fix the previously observed construction-table initialization-ordering gap; if root-causing shows the actual gap is outside this file's own init/call ordering (e.g. controller boot sequencing), stop and flag it as a separate follow-up rather than forcing a fix into this task's boundary - Verify (with a test) that the destructive-move fields on a freshly-built job payload are always NULL, so repeated use against a shared template is provably benign @@ -167,3 +167,9 @@ - Observable: a clean full-suite run with the byte-identical single-job baseline check passing alongside it - _Requirements: 8.1, 8.2, 8.4_ - _Boundary: Non-Regression & Testing_ + +## Implementation Notes + +- **Task 1.3**: `meson test --suite xce`'s `libxce test` target SIGABRTs under ASAN leak detection whenever any code path exercises a `job_init()` failure — root cause is the vendored `rtti` library's `makeInstance()` not freeing its allocation when a type's `.init()` returns negative (no `.c` source available, headers-only in the nix store). This is pre-existing and independently reproduced against an unmodified pre-task `jobj.c`, not introduced by this feature. It will fire again on any future task whose tests exercise a `create_job()`/`makeJob()` failure path (e.g. task 3.1/3.4's fault-injection tests for `restore_job_array()`) — expect the same SIGABRT-not-assertion-failure signature (`grep ":FAIL"` on the Unity log will be empty; the failure is LeakSanitizer-only) and do not treat it as a new regression without the same stash-and-reproduce check task 1.3's review used. +- **Task 1.3**: `job_obj_init()`/`job_obj_fini()` are now idempotent (atomic_flag-guarded) — safe to call more than once without an intervening `fini()`. Any future test construction can rely on this instead of coordinating a single call site. +- **Task 1.3**: `makeJobsub()`/`makeJobstartinfo()` now return `-ENODEV` (not a generic `-EINVAL`) when the interning table isn't initialized — if a later task's error handling branches on `job_init()`'s return value, `-ENODEV` is a distinct, checkable signal for "table not ready" vs. other construction failures. diff --git a/src/lib/jobj.c b/src/lib/jobj.c index d8e84e71..d4a5bd06 100644 --- a/src/lib/jobj.c +++ b/src/lib/jobj.c @@ -7,6 +7,7 @@ #include "jobj.h" #include "kv.h" +#include "misc.h" #include "mylock.h" #include "preason.h" #include "utils.h" @@ -101,6 +102,10 @@ static int jobsub_table_init(int nslots) static void jobsub_table_fini() { destroy(jobsub_table.sub_table); + /* Leave no dangling pointer behind: makeJobsub() checks this for + * NULL (see below) to detect a not-(yet-)initialized table instead + * of dereferencing freed memory (Requirement 10.2 hardening). */ + jobsub_table.sub_table = NULL; mylock_fini(&(jobsub_table.lock)); } @@ -228,6 +233,21 @@ job_submit_data_t* makeJobsub(job_submit_data_t* subdata) return NULL; } + if (jobsub_table.sub_table == NULL) { + /* job_obj_init() has not (yet) completed for this generation of + * the module -- dispatching key_find/key_add below against an + * uninitialized/torn-down table is unsafe, not just wrong. + * Root cause of the previously observed makeJobsub() + * NULL-with-EINVAL-for-a-valid-src failure (Requirement 10.2): + * that generic EINVAL was indistinguishable from "subdata was + * invalid" above. Fail loudly with a distinct errno instead, so + * an ordering bug can never again masquerade as a bad-argument + * error. */ + errno = ENODEV; + + return NULL; + } + jobsub = makeInstance(&job_submit_type, subdata); if (jobsub == NULL) { return NULL; @@ -407,6 +427,13 @@ job_startinfo_t* makeJobstartinfo(job_startinfo_t* startinfo) return NULL; } + if (startinfo_table.startinfo_table == NULL) { + /* Mirrors makeJobsub()'s guard above -- see its comment. */ + errno = ENODEV; + + return NULL; + } + startinfo1 = makeInstance(&(job_startinfo_type), startinfo); if (startinfo1 == NULL) { return NULL; @@ -512,6 +539,10 @@ static int startinfo_table_init(int nslots) static void startinfo_table_fini() { destroy(startinfo_table.startinfo_table); + /* See jobsub_table_fini(): avoid leaving a dangling pointer that + * makeJobstartinfo()'s not-(yet-)initialized guard would otherwise + * have to distinguish from a live table. */ + startinfo_table.startinfo_table = NULL; mylock_fini(&(startinfo_table.lock)); return; @@ -531,16 +562,44 @@ static size_t job_datasize(va_list ap) static int job_init(meta_obj_t* obj, va_list ap) { - job_t* dst = RTTI_OBJ_TO_DATA(job_t, obj); - job_private_t* priv = (job_private_t*)RTTI_OBJ_TO_PRIV(obj); - xce_job_t* src = va_arg(ap, xce_job_t*); + job_t* dst = RTTI_OBJ_TO_DATA(job_t, obj); + job_private_t* priv = (job_private_t*)RTTI_OBJ_TO_PRIV(obj); + xce_job_t* src = va_arg(ap, xce_job_t*); + job_submit_data_t* submit = NULL; + char* ckptinfo = NULL; + int saved_errno; if (src->submit == NULL) { return -EINVAL; } + /* Resolve every fallible field BEFORE any MOVE_PTR runs below, so a + * construction failure here never leaves src half-moved: callers + * (e.g. job.c's restore_job()) rely on src being fully intact so + * free_Job() can release everything on a create_job()/makeJob() + * failure. Requirement 10.1: check makeJobsub()'s (and + * makeString(ckptinfo)'s) return and propagate failure instead of + * silently storing NULL and reporting success. */ + submit = makeJobsub(src->submit); + if (submit == NULL) { + saved_errno = (errno != 0) ? errno : ENOMEM; + + return -saved_errno; + } + + if (src->ckptinfo != NULL) { + ckptinfo = makeString(src->ckptinfo); + if (ckptinfo == NULL) { + saved_errno = (errno != 0) ? errno : ENOMEM; + + dereference(submit); + + return -saved_errno; + } + } + dst->job.jobid = src->jobid; - dst->job.submit = makeJobsub(src->submit); + dst->job.submit = submit; dst->job.sub_time = src->sub_time; dst->job.queue_time = src->queue_time; dst->job.start_time = src->start_time; @@ -552,7 +611,7 @@ static int job_init(meta_obj_t* obj, va_list ap) dst->job.run_time = src->run_time; MOVE_PTR(dst->job.run_usage, src->run_usage); MOVE_PTR(dst->job.exit_info, src->exit_info); - dst->job.ckptinfo = makeString(src->ckptinfo); + dst->job.ckptinfo = ckptinfo; dst->job.pending_ctrl = src->pending_ctrl; MOVE_PTR(dst->job.pend_reasons, src->pend_reasons); @@ -582,7 +641,15 @@ static void job_destroy(meta_obj_t* obj) } if (j->job.accounts != NULL) { - /* FIXME??? */ + /* Requirement 10.4: accounts is MOVE_PTR'd into the job during + * job_init but was never freed here -- a genuine leak, multiplied + * by array element count once array submission lands. Mirrors + * the free_job_account() usage already established for this + * exact type in sched.c's job teardown path (misc.c owns the + * free function since xce_job_account_t has no capnp-generated + * free_*_ptr counterpart -- it is not a capnp-mapped struct). */ + free_job_account(j->job.accounts); + j->job.accounts = NULL; } if (j->job.run_usage != NULL) { @@ -728,21 +795,54 @@ job_t* makeJob(var jobmgr, xce_job_t* j) return job; } +/* Requirement 10.2: job_obj_init() must be safe to call more than once + * within a process without an intervening job_obj_fini(). Root cause of + * the previously observed jobsub_table ordering hazard: every controller + * unit-test file that constructs jobs calls job_obj_init() from its own + * setUp() (confirmed via src/controller/meson.build -- all such test + * files are linked into the single controller_unit_test executable and + * run in one process), each guarded only by its own file-local static + * flag, none of which coordinate with each other. Across a full test run + * job_obj_init() was therefore re-entered many times with no intervening + * job_obj_fini(), and jobsub_table_init()/startinfo_table_init() + * unconditionally re-ran mylock_init() on the already-live lock + * (undefined behavior on an in-use mutex, per POSIX) and unconditionally + * overwrote the table pointer, silently orphaning the previous table and + * corrupting the refcount bookkeeping of every entry still referencing + * it. This guard makes every call after the first a no-op success until + * job_obj_fini() runs, matching what every existing call site already + * expects (EQUAL_INT(0, job_obj_init(...))). The production controller + * boot path (cluster_init_managers(), called once from + * cluster_init_phase1()) was never actually at risk -- this hazard was + * confined to the combined test binary. */ +static atomic_flag job_obj_ready = ATOMIC_FLAG_INIT; + int job_obj_init(int n_sub_table_slots, int n_startinfo_table_slots) { int rc = 0; + if (atomic_flag_test_and_set(&job_obj_ready)) { + return 0; + } + rc = jobsub_table_init(n_sub_table_slots); if (rc < 0) { + atomic_flag_clear(&job_obj_ready); return rc; } rc = startinfo_table_init(n_startinfo_table_slots); - return rc; + if (rc < 0) { + atomic_flag_clear(&job_obj_ready); + return rc; + } + + return 0; } void job_obj_fini(void) { jobsub_table_fini(); startinfo_table_fini(); + atomic_flag_clear(&job_obj_ready); } diff --git a/src/lib/meson.build b/src/lib/meson.build index c61e072f..507e2e07 100644 --- a/src/lib/meson.build +++ b/src/lib/meson.build @@ -163,6 +163,7 @@ libxce_test_src = ['testsuite/test.c', 'testsuite/test_bitset.c', 'testsuite/test_kv.c', 'testsuite/test_jobj.c', + 'testsuite/test_jobj_hardening.c', 'testsuite/test_wobj.c', 'testsuite/test_qobj.c', 'testsuite/test_request.c', diff --git a/src/lib/testsuite/test.c b/src/lib/testsuite/test.c index 6644807a..b149087b 100644 --- a/src/lib/testsuite/test.c +++ b/src/lib/testsuite/test.c @@ -26,6 +26,7 @@ extern void test_select(void); extern void test_bitset(void); extern void test_kv(void); extern void test_jobobj(void); +extern void test_jobj_hardening(void); extern void test_qobj(void); extern void test_wobj(void); extern void test_request(void); @@ -140,6 +141,7 @@ int main(int argc, char* argv[]) TEST(test_bitset); TEST(test_kv); TEST(test_jobobj); + TEST(test_jobj_hardening); TEST(test_wobj); TEST(test_qobj); TEST(test_request); diff --git a/src/lib/testsuite/test_jobj_hardening.c b/src/lib/testsuite/test_jobj_hardening.c new file mode 100644 index 00000000..f6fd312a --- /dev/null +++ b/src/lib/testsuite/test_jobj_hardening.c @@ -0,0 +1,424 @@ +/* + * test_jobj_hardening.c -- unit tests for job-array-single-wal-record + * Requirement 10 (jobj.c hardening). Each test exercises one of the five + * acceptance criteria; per design.md, all five are general jobj.c + * correctness fixes the existing single-job path benefits from too -- + * none of this depends on the array-submission feature itself. + */ +#include +#include +#include +#include +#include + +#include + +#include "jobj.h" +#include "kvmgr.h" +#include "misc.h" +#include "utils.h" +#include "xce/types.h" +#include "xstring.h" + +static int g_hard_xstring_ready = 0; +static int g_hard_jobobj_ready = 0; + +static void hard_ensure_ready(void) +{ + if (!g_hard_xstring_ready) { + TEST_ASSERT_TRUE(xstring_table_init(64) >= 0); + g_hard_xstring_ready = 1; + } + + if (!g_hard_jobobj_ready) { + TEST_ASSERT_EQUAL_INT(0, job_obj_init(64, 64)); + g_hard_jobobj_ready = 1; + } +} + +static job_submit_data_t hard_make_submit(const char* job_name) +{ + job_submit_data_t s = { .queue = "xce", + .user = "rshen", + .mintasks = 1, + .maxtasks = 1, + .start_info = NULL, + .job_name = (char*)job_name, + .project_name = "default", + .user_group = NULL, + .job_description = "d", + .resreq = "type == any" }; + return s; +} + +/* ------------------------------------------------------------------- */ +/* Requirement 10.2 -- job_obj_init() is idempotent without an */ +/* intervening job_obj_fini(). */ +/* ------------------------------------------------------------------- */ + +/* The previously observed makeJobsub() NULL-with-EINVAL-for-a-valid-src + * failure was root-caused to job_obj_init() being non-idempotent: every + * controller unit-test file that constructs jobs calls job_obj_init() + * from its own setUp() (confirmed via src/controller/meson.build -- all + * such test files are linked into the single controller_unit_test + * executable and run in one process), each guarded only by its own + * file-local static flag, none of which coordinate with each other. + * Across a full test run job_obj_init() was therefore re-entered many + * times with no intervening job_obj_fini(). This proves job_obj_init() + * is now safe under exactly that pattern, and that construction still + * works correctly (interning still functions) afterward -- not just + * "didn't crash". */ +static void test_job_obj_init_is_idempotent_without_fini(void) +{ + int rc; + + hard_ensure_ready(); + + rc = job_obj_init(16, 16); + TEST_ASSERT_EQUAL_INT(0, rc); + rc = job_obj_init(64, 64); + TEST_ASSERT_EQUAL_INT(0, rc); + rc = job_obj_init(5381, 5381); + TEST_ASSERT_EQUAL_INT(0, rc); + + job_submit_data_t submit1 = hard_make_submit("J-hard-10-2"); + job_submit_data_t* s2 = makeJobsub(&submit1); + job_submit_data_t* s3 = makeJobsub(&submit1); + + TEST_ASSERT_NOT_NULL(s2); + TEST_ASSERT_TRUE(s2 == s3); +} + +/* ------------------------------------------------------------------- */ +/* Requirement 10.1 -- job_init() propagates a makeJobsub() failure */ +/* instead of storing NULL and reporting success. */ +/* ------------------------------------------------------------------- */ + +/* Forces makeJobsub() to fail the same way the originally observed bug + * did -- via job_obj_fini() leaving jobsub_table not (yet) re-initialized + * for this generation -- except now safely and deterministically: + * makeJobsub()'s Requirement 10.2 guard detects this and fails cleanly + * (NULL, errno=ENODEV) instead of touching a torn-down table, which is + * what makes this test safe to run in-process alongside every other test + * file regardless of what state they left jobj.c's module globals in. */ +static void test_job_init_propagates_makeJobsub_failure(void) +{ + kvmgr_params_t jobmgr_params = { .key_type = KVMGR_KEY_INT, + .nslots = 32, + .delta = 32, + /* job_t has .destroy = NULL (jobj.c) -- + * teardown is wired only through its + * reference interface (job_dereference). + * The kvmgr's datakiller must therefore + * be dereference(), not the generic + * destroy(), or job_destroy() never + * runs and every raw-heap MOVE_PTR + * field (e.g. accounts) silently leaks + * on manager teardown. */ + .datakiller = dereference }; + var jobmgr; + job_submit_data_t submit1 = hard_make_submit("J-hard-10-1"); + xce_job_t job1 = { .submit = &submit1, .jobid = 90001 }; + job_t* j2 = NULL; + + hard_ensure_ready(); + + jobmgr = makeInstance(&xkvmgr_type, &jobmgr_params); + TEST_ASSERT_TRUE(jobmgr != NULL); + + /* Force the "not (yet) initialized" state this test needs. */ + job_obj_fini(); + g_hard_jobobj_ready = 0; + + j2 = makeJob(jobmgr, &job1); + + /* The bug: the original job_init() stored makeJobsub()'s NULL return + * into dst->job.submit and still returned success (1), so makeJob() + * would incorrectly hand back a non-NULL job_t with a NULL + * job.submit. The fix must fail construction outright instead. */ + TEST_ASSERT_NULL(j2); + + destroy(jobmgr); + + /* Restore module state for every test that runs after this one. */ + hard_ensure_ready(); + + /* Known, out-of-boundary residual: under ASAN leak detection this + * exercise reports a ~240-byte direct leak attributed to + * makeInstance() (via makeJob_/makeJob, jobj.c) for THIS call. + * Root-caused by inspection: makeInstance() allocates the object's + * meta_obj_t+privdata+data block BEFORE invoking .init(), and when + * .init() (job_init) returns a negative rc, makeInstance() returns + * NULL to the caller without freeing that block -- every RTTI + * .init() failure path in this codebase (including the pre-existing + * "src->submit == NULL" branch a few lines above ours, and + * jobsub_init's own ENOMEM branches) shares this exact + * characteristic; it is a property of the vendored rtti library's + * makeInstance(), not of job_init()'s logic, and not something + * src/lib/jobj.c can fix (no source access, no in-boundary seam). + * job_init() itself is correct here: Requirement 10.1 only asks it + * to check makeJobsub()'s return and propagate failure instead of + * storing NULL-with-success, which it now does -- this residual + * leak would occur identically even if job_init() never allocated + * anything at all before failing. Flagged to the parent task as a + * separate, pre-existing, out-of-scope follow-up candidate. */ +} + +/* ------------------------------------------------------------------- */ +/* Requirement 10.3 -- MOVE_PTR fields are always NULL on a freshly */ +/* built (calloc'd) submission payload, so reuse against a shared */ +/* template across array elements is provably benign. */ +/* ------------------------------------------------------------------- */ + +/* Mirrors build_job_submit_payloads() (src/controller/job.c) exactly: + * calloc a fresh xce_job_t and set only submit/jobid/sub_time/ + * queue_time/state -- every MOVE_PTR field (pidinfo/allocs/accounts/ + * run_usage/exit_info/pend_reasons) is left at its calloc-zeroed NULL. + * This is also the pattern the array-expansion path (restore_job_array, + * a later task) will use per element. */ +static void test_freshly_submitted_job_move_ptr_fields_are_null(void) +{ + kvmgr_params_t jobmgr_params = { .key_type = KVMGR_KEY_INT, + .nslots = 32, + .delta = 32, + /* job_t has .destroy = NULL (jobj.c) -- + * teardown is wired only through its + * reference interface (job_dereference). + * The kvmgr's datakiller must therefore + * be dereference(), not the generic + * destroy(), or job_destroy() never + * runs and every raw-heap MOVE_PTR + * field (e.g. accounts) silently leaks + * on manager teardown. */ + .datakiller = dereference }; + var jobmgr; + job_submit_data_t submit1 = hard_make_submit("J-hard-10-3"); + time_t now = time(NULL); + xce_job_t* job_a = calloc(1, sizeof(*job_a)); + xce_job_t* job_b = calloc(1, sizeof(*job_b)); + job_t* j2 = NULL; + job_t* j3 = NULL; + + TEST_ASSERT_NOT_NULL(job_a); + TEST_ASSERT_NOT_NULL(job_b); + + hard_ensure_ready(); + + job_a->submit = &submit1; + job_a->jobid = 90003; + job_a->sub_time = now; + job_a->queue_time = now; + job_a->state = JOB_STAT_PEND; + + /* A second, independent element built from a different calloc'd + * source but sharing the SAME submit template -- the array + * construction pattern this feature relies on. */ + job_b->submit = &submit1; + job_b->jobid = 90004; + job_b->sub_time = now; + job_b->queue_time = now; + job_b->state = JOB_STAT_PEND; + + jobmgr = makeInstance(&xkvmgr_type, &jobmgr_params); + TEST_ASSERT_TRUE(jobmgr != NULL); + + j2 = makeJob(jobmgr, job_a); + TEST_ASSERT_NOT_NULL(j2); + TEST_ASSERT_NULL(j2->job.pidinfo); + TEST_ASSERT_NULL(j2->job.allocs); + TEST_ASSERT_NULL(j2->job.accounts); + TEST_ASSERT_NULL(j2->job.run_usage); + TEST_ASSERT_NULL(j2->job.exit_info); + TEST_ASSERT_NULL(j2->job.pend_reasons); + + j3 = makeJob(jobmgr, job_b); + TEST_ASSERT_NOT_NULL(j3); + TEST_ASSERT_NULL(j3->job.pidinfo); + TEST_ASSERT_NULL(j3->job.allocs); + TEST_ASSERT_NULL(j3->job.accounts); + TEST_ASSERT_NULL(j3->job.run_usage); + TEST_ASSERT_NULL(j3->job.exit_info); + TEST_ASSERT_NULL(j3->job.pend_reasons); + + /* makeJobsub()'s interning collapses the shared template to one + * real allocation -- confirms the "right foundation for array + * scale" design.md calls out. */ + TEST_ASSERT_TRUE(j2->job.submit == j3->job.submit); + + destroy(jobmgr); + free(job_a); + free(job_b); +} + +/* ------------------------------------------------------------------- */ +/* Requirement 10.4 -- job_destroy() no longer leaks accounts. */ +/* ------------------------------------------------------------------- */ + +static xce_job_account_t* hard_build_accounts(uint64_t jobid) +{ + xce_job_account_t* acct = calloc(1, sizeof(*acct)); + + TEST_ASSERT_NOT_NULL(acct); + acct->jobid = jobid; + acct->nhosts = 1; + acct->accounts = calloc(1, sizeof(*acct->accounts)); + TEST_ASSERT_NOT_NULL(acct->accounts); + + xce_host_account_t* host = calloc(1, sizeof(*host)); + TEST_ASSERT_NOT_NULL(host); + /* host->host intentionally left NULL: free_host_account() (misc.c, + * pre-existing, out of this task's src/lib/jobj.c boundary) does not + * free that field -- a separate, unrelated gap in misc.c, not + * Requirement 10.4's accounts leak this test targets. Leaving it + * NULL keeps this test isolated to proving job_destroy() drives + * free_job_account() correctly. */ + host->naccount = 1; + host->accounts = calloc(1, sizeof(*host->accounts)); + TEST_ASSERT_NOT_NULL(host->accounts); + + xce_res_account_t* res = calloc(1, sizeof(*res)); + TEST_ASSERT_NOT_NULL(res); + res->resname = strdup("cpu"); + res->used = 1.0; + + host->accounts[0] = res; + acct->accounts[0] = host; + + acct->nres_shared = 1; + acct->accounts_shared = calloc(1, sizeof(*acct->accounts_shared)); + TEST_ASSERT_NOT_NULL(acct->accounts_shared); + + xce_res_account_t* shared = calloc(1, sizeof(*shared)); + TEST_ASSERT_NOT_NULL(shared); + shared->resname = strdup("mem"); + acct->accounts_shared[0] = shared; + + return acct; +} + +/* Run this test binary under ASAN with real leak detection + * (ASAN_OPTIONS=detect_leaks=1, the xcelib_test default -- unlike the + * controller suite, which sets detect_leaks=0 for pre-existing, unrelated + * leaks) to prove job_destroy() no longer leaks j->job.accounts. */ +static void test_job_destroy_does_not_leak_accounts(void) +{ + kvmgr_params_t jobmgr_params = { .key_type = KVMGR_KEY_INT, + .nslots = 32, + .delta = 32, + /* job_t has .destroy = NULL (jobj.c) -- + * teardown is wired only through its + * reference interface (job_dereference). + * The kvmgr's datakiller must therefore + * be dereference(), not the generic + * destroy(), or job_destroy() never + * runs and every raw-heap MOVE_PTR + * field (e.g. accounts) silently leaks + * on manager teardown. */ + .datakiller = dereference }; + var jobmgr; + job_submit_data_t submit1 = hard_make_submit("J-hard-10-4"); + time_t now = time(NULL); + xce_job_t* job1 = calloc(1, sizeof(*job1)); + job_t* j2 = NULL; + + TEST_ASSERT_NOT_NULL(job1); + + hard_ensure_ready(); + + job1->submit = &submit1; + job1->jobid = 90005; + job1->sub_time = now; + job1->queue_time = now; + job1->state = JOB_STAT_PEND; + job1->accounts = hard_build_accounts(90005); + + jobmgr = makeInstance(&xkvmgr_type, &jobmgr_params); + TEST_ASSERT_TRUE(jobmgr != NULL); + + j2 = makeJob(jobmgr, job1); + TEST_ASSERT_NOT_NULL(j2); + TEST_ASSERT_NOT_NULL(j2->job.accounts); /* MOVE_PTR'd in */ + TEST_ASSERT_NULL(job1->accounts); /* moved out of src */ + + /* destroy(jobmgr) tears down every stored job_t via its datakiller + * (dereference), which drives job_dereference -> job_destroy. */ + destroy(jobmgr); + + free(job1); +} + +/* ------------------------------------------------------------------- */ +/* Requirement 10.5 -- decode_Job/free_Job have no ckptinfo wire field */ +/* to leak in the first place. */ +/* ------------------------------------------------------------------- */ + +/* Verified by inspection, not exercisable from src/lib: decode_Job() and + * free_Job() are capnp-generated, controller-side symbols consuming + * job.c, which src/lib does not link. struct Job's capnp schema + * (src/schema/controller.capnp) declares jobid/submitData/subtime/ + * queuetime/starttime/finishtime/state/allocs/runtime/pidinfo/ + * pendreason/transientSince -- there is no ckptinfo field and no + * "JobAccount"-style struct for accounts either. decode_Job() is + * mechanically generated from that schema, so it structurally cannot + * populate xce_job_t.ckptinfo; free_Job() correctly never frees it + * either -- confirmed non-issue, not a gap. + * + * This test instead verifies the one piece of this invariant jobj.c + * itself owns: job_init()'s ckptinfo handling for the realistic + * freshly-submitted-job case (src->ckptinfo == NULL, exactly what + * build_job_submit_payloads()/array-element construction produce) + * leaves dst->job.ckptinfo NULL with no leak and no crash on destroy. */ +static void test_ckptinfo_null_for_freshly_submitted_job_no_leak(void) +{ + kvmgr_params_t jobmgr_params = { .key_type = KVMGR_KEY_INT, + .nslots = 32, + .delta = 32, + /* job_t has .destroy = NULL (jobj.c) -- + * teardown is wired only through its + * reference interface (job_dereference). + * The kvmgr's datakiller must therefore + * be dereference(), not the generic + * destroy(), or job_destroy() never + * runs and every raw-heap MOVE_PTR + * field (e.g. accounts) silently leaks + * on manager teardown. */ + .datakiller = dereference }; + var jobmgr; + job_submit_data_t submit1 = hard_make_submit("J-hard-10-5"); + time_t now = time(NULL); + /* ckptinfo intentionally left NULL by the initializer below -- + * matches a freshly-submitted job. */ + xce_job_t job1 = { .submit = &submit1, + .jobid = 90006, + .sub_time = now, + .queue_time = now, + .state = JOB_STAT_PEND }; + job_t* j2 = NULL; + + hard_ensure_ready(); + + jobmgr = makeInstance(&xkvmgr_type, &jobmgr_params); + TEST_ASSERT_TRUE(jobmgr != NULL); + + j2 = makeJob(jobmgr, &job1); + TEST_ASSERT_NOT_NULL(j2); + TEST_ASSERT_NULL(j2->job.ckptinfo); + + destroy(jobmgr); +} + +void test_jobj_hardening(void) +{ + printf("\n>> jobj hardening test (job-array-single-wal-record Requirement 10)\n\n"); + + RUN_TEST(test_job_obj_init_is_idempotent_without_fini); + RUN_TEST(test_job_init_propagates_makeJobsub_failure); + RUN_TEST(test_freshly_submitted_job_move_ptr_fields_are_null); + RUN_TEST(test_job_destroy_does_not_leak_accounts); + RUN_TEST(test_ckptinfo_null_for_freshly_submitted_job_no_leak); + + job_obj_fini(); + xstring_table_fini(); + g_hard_jobobj_ready = 0; + g_hard_xstring_ready = 0; +}