lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions
myxce/src/lib/testsuite/test_jobj_hardening.c
Rongsong Shen 76862bd9aa 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.
2026-07-14 08:29:45 +00:00

424 lines
18 KiB
C
Raw Blame History

/*
* 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 <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <unity/unity.h>
#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;
}
Reference in a new issue View git blame Copy permalink