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.
This commit is contained in:
parent
bc4e70b790
commit
76862bd9aa
5 changed files with 541 additions and 8 deletions
114
src/lib/jobj.c
114
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);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue