test(scheduler): lock reason-ownership threading contract (Phase 1)
Phase 1 of scheduler-hardening-plan.md: mechanically locks the bug-1521
contract that 91c3fb7f established but which lived only in code comments:
the allocator thread is the ONLY writer of a schedobj's reason fields;
the picker thread never touches reasons.
test_reason_ownership_audit.c (in the sched_unit_test aggregate) enforces
this in three layers:
- a whole-tree scan of every src/scheduler source (fixed 51-file list) for
schedobj-shaped reason ownership lines (obj->reason / schedobj->reason /
primary->reason / cobj->reason / compound->reason combined with
MOVE_PTR( / free_PendReason_ptr( / calloc( / a bare = store), checked
against an OWNERSHIP_SITES allowlist whose fragments must still exist in
their files (so the list cannot rot silently);
- a dedicated scan of sched_job_picker()'s brace-matched function body
rejecting ANY reason ownership inside the picker;
- detector self-tests proving it fires on the historical violation shapes
and stays quiet on sanctioned reads, ctx->reason (the sched_ctx_t local
shuttle), construction =NULL, and comment lines (skipped by the scans --
the contract's own documentation names the write shape in prose at
scheduler.c:496 / fcfs.c:243).
Red proof: a temporary MOVE_PTR(job->schedobj->reason, ...) injected into
the picker loop trips BOTH the file scan and the picker-body scan; the
injection is reverted and the suite is green.
.kiro/steering/scheduler-reason-ownership.md formalizes the discipline
(who allocates, who frees, which thread, MOVE_PTR semantics, the
ctx->reason exemption) for review use.
Full scheduler suite: 102/102 OK.
Co-Authored-By: Claude Code <noreply@anthropic.com>
This commit is contained in:
lei.chen
parent
5269f8a5c4
commit
1fd6dc1c13
5 changed files with 799 additions and 0 deletions
105
.kiro/steering/scheduler-reason-ownership.md
Normal file
105
.kiro/steering/scheduler-reason-ownership.md
Normal file
|
|
@ -0,0 +1,105 @@
|
|||
# Reason Ownership & Threading Contract (scheduler)
|
||||
|
||||
This document formalizes the discipline established by commit 91c3fb7f
|
||||
(bug-1521). It is the review reference for any change that touches a pend
|
||||
reason (`xce_pendreason_t*`) anywhere in `src/scheduler/`.
|
||||
|
||||
## Why this contract exists
|
||||
|
||||
bug-1521 (fixed 2026-10) was heap corruption from a data race: the picker
|
||||
thread wrote `job->schedobj->reason` while the allocator thread executed
|
||||
`MOVE_PTR(obj->reason, ctx.reason)` on the same field. The two threads run
|
||||
concurrently over the same `xce_sched_job_t` — the picker selects
|
||||
candidate jobids and the allocator resolves each into a decision — so any
|
||||
field both threads can touch needs a single-writer owner. The reason chain
|
||||
is the one field with a non-obvious ownership shape, because reasons are
|
||||
built in several places (alloc chain, plugins, election gate) before being
|
||||
attached.
|
||||
|
||||
## The contract
|
||||
|
||||
1. **The allocator thread is the ONLY writer of a schedobj's reason
|
||||
fields.** "Reason fields" means `obj->reason` (the
|
||||
`xce_sched_obj_t` top-level field) and the sub-struct fields
|
||||
`obj->u.primary->reason`, `obj->u.compound->reason`, and the alter
|
||||
loop's `cobj->reason`.
|
||||
|
||||
All sanctioned write sites execute on the allocator thread:
|
||||
- `scheduler.c` `sched_make_primary_alloc_()` /
|
||||
`sched_make_compound_alloc_()` — `free_PendReason_ptr` +
|
||||
`MOVE_PTR(obj->reason, ctx.reason)` (reached only via
|
||||
`sched_make_allocation()` ← `sched_allocator()`,
|
||||
`sched_election.c`).
|
||||
- `plugins/limit.c` `limit_job_election()` — the
|
||||
`MOVE_PTR(obj->reason, ctx.reason)` carry-shape (reached via
|
||||
`plugin_job_election()` from `sched_allocator()`; today
|
||||
dead-by-construction because `match_job_global_limit_()` never
|
||||
attaches a reason, kept deliberately — see its own comment).
|
||||
|
||||
2. **The picker thread never touches reasons.** `sched_job_picker()`
|
||||
(sched_election.c) resolves the picker plugin, pulls jobids through the
|
||||
`wbuf` rendezvous, runs the session protocol, and pushes jobids into
|
||||
the `jbuf` cbuf. Its only sanctioned references to a job are reads of
|
||||
scheduling-relevant state (`job->jobid`, queue membership through the
|
||||
picker plugin's own data structures). Plugins reachable from the picker
|
||||
(`plugins/fcfs.c`'s `fcfs_elect_job()`, `builtin_fcfs.c`) must not
|
||||
write `schedobj->reason` — fcfs.c carries the ban in a comment at the
|
||||
historical violation site.
|
||||
|
||||
Queue-state pend reasons (queue drained / outside its dispatch window)
|
||||
are attached in `sched_allocator()` (sched_election.c, allocator
|
||||
thread), NOT in the picker — see the PEND-reason P1 comment there.
|
||||
|
||||
3. **Ownership verbs on a reason field**: `MOVE_PTR(dst, src)` transfers
|
||||
ownership INTO the field (the previous value must be released first —
|
||||
the `free_PendReason_ptr(&(obj->reason))` guard before each MOVE_PTR
|
||||
handles bug-1884's stale-reason shape); `free_PendReason_ptr(&field)`
|
||||
releases; `make_pendreason()` (lib/preason.c) is the sole allocation
|
||||
factory (enforced separately by
|
||||
`test_picker_pendreason_factory_audit.c`). A bare `field = ptr` store
|
||||
is an ownership transfer too and needs the same justification as a
|
||||
MOVE_PTR.
|
||||
|
||||
4. **The `ctx->reason` shuttle is not a schedobj field.**
|
||||
`sched_ctx_t.reason` (schedobj.h) is a function-local accumulator the
|
||||
alloc chain builds into (alloc_runner.c, resource.c, scheduler.c) and
|
||||
`sched_ctx_fini()` releases. It is thread-confined by the alloc chain's
|
||||
single-thread shape and never shared with the picker. Assignments to
|
||||
it are not contract violations.
|
||||
|
||||
5. **Construction and destruction are exempt** (no concurrent partner
|
||||
exists): constructors explicitly NULL the field (core_events.c,
|
||||
schedobj.c), and the destruction path frees it (core.c's
|
||||
`xce_sched_core_free_schedobj()` / kill chain).
|
||||
|
||||
## How the contract is enforced
|
||||
|
||||
`testsuite/test_reason_ownership_audit.c` (scheduler unit aggregate)
|
||||
mechanically enforces this:
|
||||
|
||||
- a whole-tree scan of every `src/scheduler` source file (fixed list,
|
||||
extend it when files are added) for schedobj-shaped reason ownership
|
||||
lines (`obj->reason` / `schedobj->reason` / `primary->reason` /
|
||||
`cobj->reason` / `compound->reason` combined with `MOVE_PTR(` /
|
||||
`free_PendReason_ptr(` / `calloc(` / a bare `= store`);
|
||||
- an allowlist (`OWNERSHIP_SITES` in that test) of the sanctioned
|
||||
fragments above, each of which must still exist in its file (so the
|
||||
list cannot rot silently);
|
||||
- a dedicated scan of `sched_job_picker()`'s function body rejecting ANY
|
||||
reason ownership inside the picker;
|
||||
- comment lines are skipped (the contract's own documentation names the
|
||||
write shape in prose).
|
||||
|
||||
A new reason write site fails the test with instructions. To legitimately
|
||||
add one: put it on the allocator thread's call path, then add the exact
|
||||
line fragment to `OWNERSHIP_SITES` with a comment stating the thread
|
||||
context — the review of that allowlist entry IS the enforcement point.
|
||||
|
||||
## Related
|
||||
|
||||
- bug-1521 (the race), bug-1884 (stale-reason release before re-attach),
|
||||
PEND-reason P1 (queue-state reasons attached allocator-side).
|
||||
- `.kiro/steering/resource-accounting.md` — the analogous single-writer
|
||||
discipline for resource accounting pools.
|
||||
- `scheduler-hardening-plan.md` Phase 1 — where this test and document
|
||||
were introduced.
|
||||
|
|
@ -109,6 +109,13 @@ scheduler 与 controller 断连后重连，走 in-place rejoin：`xcecli_join(Ho
|
|||
|
||||
**验收**：结构测试对人为注入的 picker 线程写入变红；`/xce-build scheduler` 全绿。
|
||||
|
||||
> **执行结果（2026-10-30）**：✅ 已落地。`testsuite/test_reason_ownership_audit.c`
|
||||
> 三层锁死（封闭清单扫描 + allowlist 活性校验 + `sched_job_picker` 函数体专属扫描），
|
||||
> `ctx->reason` 本地 shuttle、构造期 `= NULL`、析构路径、注释行均已正确豁免；
|
||||
> 注入反证双重红（文件扫描 + picker 扫描同时抓住注入的
|
||||
> `MOVE_PTR(job->schedobj->reason, injected)`）。steering 文档
|
||||
> `.kiro/steering/scheduler-reason-ownership.md` 已建。全量 102/102 绿。
|
||||
|
||||
**工作量**：1 个会话。
|
||||
|
||||
---
|
||||
|
|
|
|||
|
|
@ -82,6 +82,7 @@ sched_unit_test_srcs = ['sched_unit_runner.c']
|
|||
foreach f : ['test_sched_election_null_schedobj.c',
|
||||
'test_picker_iface.c',
|
||||
'test_picker_pendreason_factory_audit.c',
|
||||
'test_reason_ownership_audit.c',
|
||||
'test_picker_core_loop_boundary.c',
|
||||
'test_no_bare_printf.c',
|
||||
'test_core_isolation.c',
|
||||
|
|
|
|||
|
|
@ -64,6 +64,7 @@ void tearDown(void)
|
|||
extern void test_sched_election_null_schedobj(void);
|
||||
extern void test_sched_picker_iface(void);
|
||||
extern void test_sched_picker_pendreason_factory_audit(void);
|
||||
extern void test_sched_reason_ownership_audit(void);
|
||||
extern void test_sched_picker_core_loop_boundary(void);
|
||||
extern void test_sched_no_bare_printf(void);
|
||||
extern void test_sched_core_isolation(void);
|
||||
|
|
@ -99,6 +100,7 @@ static const suite_entry_t SUITES[] = {
|
|||
SUITE(test_sched_election_null_schedobj),
|
||||
SUITE(test_sched_picker_iface),
|
||||
SUITE(test_sched_picker_pendreason_factory_audit),
|
||||
SUITE(test_sched_reason_ownership_audit),
|
||||
SUITE(test_sched_picker_core_loop_boundary),
|
||||
SUITE(test_sched_no_bare_printf),
|
||||
SUITE(test_sched_core_isolation),
|
||||
|
|
|
|||
684
src/scheduler/testsuite/test_reason_ownership_audit.c
Normal file
684
src/scheduler/testsuite/test_reason_ownership_audit.c
Normal file
|
|
@ -0,0 +1,684 @@
|
|||
/*
|
||||
* test_reason_ownership_audit.c - Phase 1 of the scheduler rejoin-hardening
|
||||
* plan (scheduler-hardening-plan.md): mechanically locks the bug-1521
|
||||
* reason-ownership threading contract that commit 91c3fb7f established but
|
||||
* which until now lived only in code comments and commit messages.
|
||||
*
|
||||
* THE CONTRACT (91c3fb7f, bug-1521): the picker thread (sched_job_picker(),
|
||||
* sched_election.c) and the allocator thread (sched_allocator(), same file)
|
||||
* run concurrently over the same xce_sched_job_t. The allocator thread is
|
||||
* the ONLY writer of a schedobj's reason chain
|
||||
* (obj->reason / obj->u.primary->reason / obj->u.compound->reason /
|
||||
* job->schedobj->reason): the MOVE_PTR(obj->reason, ctx.reason) sites in
|
||||
* scheduler.c's sched_make_primary_alloc_()/sched_make_compound_alloc_()
|
||||
* and plugins/limit.c's limit_job_election() all execute on the allocator
|
||||
* thread, while the picker only pushes jobids into the capacity-1 jbuf
|
||||
* rendezvous. bug-1521's live heap corruption was the picker thread
|
||||
* writing job->schedobj->reason racing the allocator thread's MOVE_PTR of
|
||||
* the same field.
|
||||
*
|
||||
* WHAT THIS TEST LOCKS, in three layers (same genre as
|
||||
* test_picker_pendreason_factory_audit.c's fixed-file-list same-line scan,
|
||||
* which this file's scanner is modeled on):
|
||||
*
|
||||
* 1. OWNERSHIP-SITE CLOSURE: every line in the scanned scheduler tree that
|
||||
* performs a reason OWNERSHIP operation on a schedobj field -- a
|
||||
* MOVE_PTR into a `->reason`, a free_PendReason_ptr() of a `->reason`,
|
||||
* or a calloc() constructing an xce_pendreason_t -- must be a line that
|
||||
* ALSO names the site's thread ownership per the OWNERSHIP_SITES
|
||||
* allowlist below. A new write site added anywhere else fails the scan
|
||||
* with instructions: move it into the allocator thread's call path or,
|
||||
* if genuinely new, update the allowlist WITH a thread-ownership note.
|
||||
* (Same-line discipline, not whole-file co-occurrence, keeps this
|
||||
* precise: scheduler.c/decision.c/etc. contain "calloc(" and
|
||||
* "pendreason" elsewhere, never together on the one line that matters --
|
||||
* the same finding test_picker_pendreason_factory_audit.c documented.)
|
||||
*
|
||||
* 2. PICKER-THREAD BAN (the bug-1521 shape): sched_job_picker()'s function
|
||||
* body (extracted from sched_election.c by brace matching) must contain
|
||||
* NO reason-ownership operation at all -- no `->reason` reference
|
||||
* combined with MOVE_PTR/free_PendReason_ptr/calloc, and no assignment
|
||||
* into any `->reason` field. The picker's sanctioned surface is:
|
||||
* resolve the picker, pull a jobid from wbuf, run the session protocol,
|
||||
* push jobids into jbuf. Everything reason-shaped belongs to the
|
||||
* allocator thread.
|
||||
*
|
||||
* 3. DETECTOR SELF-TESTS: the scan predicates are proven to fire on the
|
||||
* historical violation shapes (a picker-thread
|
||||
* MOVE_PTR(job->schedobj->reason, ...), a direct xce_pendreason_t
|
||||
* calloc, a bare `obj->reason = ...` assignment) and to stay quiet on
|
||||
* sanctioned lines (a READ like sched_election.c:373's
|
||||
* `= job->schedobj->reason` with no ownership op, a comment line, an
|
||||
* unrelated struct's calloc) -- so a typo cannot make the real scans
|
||||
* vacuously pass forever.
|
||||
*
|
||||
* KNOWN LIMITATIONS (inherited deliberately from the factory-audit model):
|
||||
* a fixed file list that must be extended when files are added under
|
||||
* src/scheduler (the readability test catches a stale path); a same-line
|
||||
* textual detector, which misses a multi-line ownership expression but
|
||||
* matches this codebase's clang-format style where a MOVE_PTR/free call
|
||||
* and its `->reason` argument share a line; and comments are scanned too
|
||||
* (a comment merely MENTIONING "MOVE_PTR" plus "->reason" on one line
|
||||
* would false-positive -- today's comments are wrapped so this does not
|
||||
* happen, and the failure message makes the cause obvious).
|
||||
*/
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
|
||||
#include <unity/unity.h>
|
||||
|
||||
#ifndef XCE_SCHEDULER_ROOT_PATH
|
||||
#error \
|
||||
"XCE_SCHEDULER_ROOT_PATH must be defined by meson.build to the absolute path of src/scheduler"
|
||||
#endif
|
||||
#ifndef XCE_SCHED_ELECTION_C_PATH
|
||||
#error \
|
||||
"XCE_SCHED_ELECTION_C_PATH must be defined by meson.build to sched_election.c's absolute path"
|
||||
#endif
|
||||
|
||||
/* Every non-testsuite .c file under src/scheduler (src/ and src/plugins/)
|
||||
* that the ownership scan covers -- the same fixed-list discipline as
|
||||
* test_picker_pendreason_factory_audit.c's SCANNED_RELATIVE_PATHS (that
|
||||
* list's own header comment records why: no directory-walk dependency for
|
||||
* a rarely-changing tree; add files here as they're created). This list is
|
||||
* the FULL scheduler_srcs set minus main.c, derived from src/scheduler/
|
||||
* meson.build's scheduler_srcs at the time of writing -- main.c is a
|
||||
* startup/round-loop file that never touches schedobj reasons, but is
|
||||
* listed anyway so a future edit there is still covered by the scan (and
|
||||
* the readability test proves the path stays valid). */
|
||||
static const char* const SCANNED_RELATIVE_PATHS[] = {
|
||||
"src/cli_compat.c",
|
||||
"src/handler.c",
|
||||
"src/scheduler.c",
|
||||
"src/sched_election.c",
|
||||
"src/core.c",
|
||||
"src/core_events.c",
|
||||
"src/core_session.c",
|
||||
"src/core_clock.c",
|
||||
"src/driver.c",
|
||||
"src/snapshot.c",
|
||||
"src/snapshot_wire.c",
|
||||
"src/snapshot_load.c",
|
||||
"src/decision.c",
|
||||
"src/recording.c",
|
||||
"src/recording_events.c",
|
||||
"src/recording_session.c",
|
||||
"src/sim.c",
|
||||
"src/sim_loop.c",
|
||||
"src/sim_picker.c",
|
||||
"src/sim_limit.c",
|
||||
"src/sim_alloc.c",
|
||||
"src/sim_verify.c",
|
||||
"src/sim_record.c",
|
||||
"src/repl.c",
|
||||
"src/repl_fmt.c",
|
||||
"src/repl_rollup.c",
|
||||
"src/repl_break.c",
|
||||
"src/repl_info.c",
|
||||
"src/repl_params.c",
|
||||
"src/repl_trace_render.c",
|
||||
"src/repl_trace_render_outcome.c",
|
||||
"src/repl_trace_store.c",
|
||||
"src/repl_trace.c",
|
||||
"src/repl_trace_emit.c",
|
||||
"src/schedobj.c",
|
||||
"src/plugin.c",
|
||||
"src/resource.c",
|
||||
"src/alloc_cache.c",
|
||||
"src/host_iter.c",
|
||||
"src/alloc_runner.c",
|
||||
"src/sched_trace.c",
|
||||
"src/sched_trace_scan.c",
|
||||
"src/sched_trace_capture.c",
|
||||
"src/sched_trace_outcome.c",
|
||||
"src/plugins/fcfs.c",
|
||||
"src/plugins/builtin_fcfs.c",
|
||||
"src/plugins/seqjob_allocator.c",
|
||||
"src/plugins/parjob_allocator.c",
|
||||
"src/plugins/limit.c",
|
||||
"src/plugins/limit_config.c",
|
||||
};
|
||||
#define NUM_SCANNED \
|
||||
(sizeof(SCANNED_RELATIVE_PATHS) / sizeof(SCANNED_RELATIVE_PATHS[0]))
|
||||
|
||||
/* --------------------------------------------------------------------------
|
||||
* Scanner primitives (shared shape with test_picker_pendreason_factory_
|
||||
* audit.c: whole-file read, per-line predicate, hard-fail on unreadable)
|
||||
* -------------------------------------------------------------------------- */
|
||||
|
||||
static char* read_whole_file(const char* path)
|
||||
{
|
||||
FILE* f = fopen(path, "rb");
|
||||
if (f == NULL) {
|
||||
return NULL;
|
||||
}
|
||||
|
||||
if (fseek(f, 0, SEEK_END) != 0) {
|
||||
fclose(f);
|
||||
return NULL;
|
||||
}
|
||||
|
||||
long len = ftell(f);
|
||||
if (len < 0) {
|
||||
fclose(f);
|
||||
return NULL;
|
||||
}
|
||||
rewind(f);
|
||||
|
||||
char* buf = (char*)malloc((size_t)len + 1);
|
||||
if (buf == NULL) {
|
||||
fclose(f);
|
||||
return NULL;
|
||||
}
|
||||
|
||||
size_t nread = fread(buf, 1, (size_t)len, f);
|
||||
fclose(f);
|
||||
buf[nread] = '\0';
|
||||
|
||||
return buf;
|
||||
}
|
||||
|
||||
/* Extract the body of the C function whose definition starts with `sig`
|
||||
* (a prefix of the definition line, e.g. "static void* sched_job_picker").
|
||||
* Returns a malloc'd copy of the text from the function's opening '{' to
|
||||
* its matching closing '}' (inclusive), or NULL if the signature or a
|
||||
* balanced brace pair cannot be found. Brace matching ignores braces
|
||||
* inside double-quoted strings and char literals and after '//' to the end
|
||||
* of a line -- enough for this codebase's formatting; comment-block
|
||||
* braces ({/* ... *}/) are handled by also skipping /*...*\/ spans. */
|
||||
static char* extract_function_body(const char* src, const char* sig)
|
||||
{
|
||||
const char* defn = strstr(src, sig);
|
||||
if (defn == NULL) {
|
||||
return NULL;
|
||||
}
|
||||
|
||||
const char* brace = strchr(defn, '{');
|
||||
if (brace == NULL) {
|
||||
return NULL;
|
||||
}
|
||||
|
||||
int depth = 0;
|
||||
const char* p = brace;
|
||||
while (*p != '\0') {
|
||||
if (*p == '"') { /* skip string literal */
|
||||
p++;
|
||||
while ((*p != '"') && (*p != '\0')) {
|
||||
if ((*p == '\\') && (*(p + 1) != '\0')) {
|
||||
p++;
|
||||
}
|
||||
p++;
|
||||
}
|
||||
} else if ((*p == '\'') && (*(p + 1) != '\0')) { /* char literal */
|
||||
p += 2;
|
||||
while ((*p != '\'') && (*p != '\0')) {
|
||||
p++;
|
||||
}
|
||||
} else if ((*p == '/') && (*(p + 1) == '/')) { /* line comment */
|
||||
while ((*p != '\n') && (*p != '\0')) {
|
||||
p++;
|
||||
}
|
||||
} else if ((*p == '/') && (*(p + 1) == '*')) { /* block comment */
|
||||
p += 2;
|
||||
while ((*p != '\0')
|
||||
&& !((*p == '*') && (*(p + 1) == '/'))) {
|
||||
p++;
|
||||
}
|
||||
if (*p != '\0') {
|
||||
p++;
|
||||
}
|
||||
} else if (*p == '{') {
|
||||
depth++;
|
||||
} else if (*p == '}') {
|
||||
depth--;
|
||||
if (depth == 0) {
|
||||
size_t len = (size_t)(p - brace) + 1;
|
||||
char* body = (char*)malloc(len + 1);
|
||||
if (body == NULL) {
|
||||
return NULL;
|
||||
}
|
||||
memcpy(body, brace, len);
|
||||
body[len] = '\0';
|
||||
return body;
|
||||
}
|
||||
}
|
||||
if (*p == '\0') {
|
||||
break;
|
||||
}
|
||||
p++;
|
||||
}
|
||||
|
||||
return NULL;
|
||||
}
|
||||
|
||||
/* --------------------------------------------------------------------------
|
||||
* The detector
|
||||
* -------------------------------------------------------------------------- */
|
||||
|
||||
/* line_has_reason_ownership - a single source line performs a reason
|
||||
* OWNERSHIP operation on a SCHEDOBJ reason field if it contains a
|
||||
* schedobj-shaped reason lvalue together with one of the ownership verbs,
|
||||
* in any order on the line:
|
||||
* MOVE_PTR( -- ownership transfer INTO the field (the bug-1521
|
||||
* racing write shape)
|
||||
* free_PendReason_ptr( -- ownership release OF the field
|
||||
* calloc( -- construction of a new xce_pendreason_t being aimed
|
||||
* at the field (factory bypass; the factory-audit
|
||||
* test separately catches calloc+pendreason_t lines)
|
||||
* A bare `obj->reason = ...` pointer-store is ALSO ownership (the third
|
||||
* historical shape) and is caught by assignment detection below.
|
||||
*
|
||||
* SCHEDOBJ-SHAPED means the lvalue's object is a schedobj (or its
|
||||
* primary/compound/alter sub-structs): `obj->reason`,
|
||||
* `job->schedobj->reason`, `primary->reason`, `cobj->reason`,
|
||||
* `compound->reason`. The scanner approximates this by REQUIRING one of
|
||||
* those exact bases on the line -- `ctx->reason`/`ctx.reason` (the
|
||||
* sched_ctx_t LOCAL shuttle field that sched_ctx_init() owns and
|
||||
* sched_ctx_fini() releases; the alloc chain builds into it, then the
|
||||
* sanctioned MOVE_PTR hands it to the schedobj) is a deliberate
|
||||
* NON-target: its assignments (alloc_runner.c:48, scheduler.c:186,
|
||||
* resource.c:570/587) are ctx construction, thread-confined by the alloc
|
||||
* chain's own single-thread shape, and never shared with the picker.
|
||||
* The sanctioned MOVE_PTR/free lines themselves carry `obj->reason`, so
|
||||
* they match the schedobj-shape requirement. If a future site moves a
|
||||
* reason with a new base name, the scan going red HERE is the contract
|
||||
* working: the base must be added to the detector or the site justified.
|
||||
*
|
||||
* Deliberately NOT an ownership op: a plain READ of ->reason (the
|
||||
* allocator thread reads job->schedobj->reason at sched_election.c:373 to
|
||||
* wrap it in a pend decision -- that is the sanctioned consumer shape).
|
||||
* Line-level discipline (both substrings on ONE line) keeps comments that
|
||||
* merely discuss the contract from firing as long as they wrap, the same
|
||||
* trade the factory audit documented and accepted. */
|
||||
static int line_has_reason_ownership(const char* line)
|
||||
{
|
||||
/* schedobj-shaped reason lvalues only (see block comment): */
|
||||
const char* bases[] = {"obj->reason",
|
||||
"schedobj->reason",
|
||||
"primary->reason",
|
||||
"cobj->reason",
|
||||
"compound->reason"};
|
||||
const int n_bases = (int)(sizeof(bases) / sizeof(bases[0]));
|
||||
const char* r = NULL;
|
||||
int i;
|
||||
|
||||
for (i = 0; i < n_bases; i++) {
|
||||
r = strstr(line, bases[i]);
|
||||
if (r != NULL) {
|
||||
break;
|
||||
}
|
||||
}
|
||||
if (r == NULL) {
|
||||
return 0;
|
||||
}
|
||||
|
||||
if ((strstr(line, "MOVE_PTR(") != NULL)
|
||||
|| (strstr(line, "free_PendReason_ptr(") != NULL)
|
||||
|| (strstr(line, "calloc(") != NULL)) {
|
||||
return 1;
|
||||
}
|
||||
|
||||
/* bare pointer-store detection: `<base> =` where the '=' is a real
|
||||
* assignment (not ==, !=, <=, >=, +=, -=). */
|
||||
{
|
||||
const char* p = r + strlen(bases[i]);
|
||||
while ((*p == ' ') || (*p == '\t')) {
|
||||
p++;
|
||||
}
|
||||
if (*p == '=' && (*(p + 1) != '=')) {
|
||||
return 1;
|
||||
}
|
||||
}
|
||||
|
||||
return 0;
|
||||
}
|
||||
|
||||
/* A line that MENTIONS an ownership verb but has no schedobj reason
|
||||
* lvalue is fine (e.g. MOVE_PTR(job->allocs, ...) at sched_election.c:388
|
||||
* -- allocs, not reason). The helper below is only for the self-test's
|
||||
* readability. */
|
||||
static int line_mentions_verb_but_not_reason(const char* line)
|
||||
{
|
||||
return ((strstr(line, "MOVE_PTR(") != NULL)
|
||||
|| (strstr(line, "free_PendReason_ptr(") != NULL))
|
||||
&& (strstr(line, "obj->reason") == NULL)
|
||||
&& (strstr(line, "schedobj->reason") == NULL);
|
||||
}
|
||||
|
||||
/* The allowlist: lines performing reason ownership that the contract
|
||||
* SANCTIONS, matched by (file-relative-path, normalized-content-fragment).
|
||||
* Each entry carries its thread ownership in the comment -- that note is
|
||||
* the point of the list: a new site must either join an existing thread
|
||||
* context or justify a new one HERE, in review, before the scan goes
|
||||
* green again.
|
||||
*
|
||||
* Today's complete inventory (verified 2026-10-30 by scanning the whole
|
||||
* scheduler tree for ->reason / .reason):
|
||||
* scheduler.c:338-339 free+MOVE_PTR -- ALLOCATOR thread
|
||||
* (sched_make_primary_alloc_ <- sched_make_primary_allocation <-
|
||||
* sched_make_allocation <- sched_allocator, sched_election.c)
|
||||
* scheduler.c:429-430 free+MOVE_PTR -- ALLOCATOR thread
|
||||
* (sched_make_compound_alloc_, same call chain)
|
||||
* plugins/limit.c:296 MOVE_PTR -- ALLOCATOR thread
|
||||
* (limit_job_election <- plugin_job_election <- sched_allocator /
|
||||
* sim_loop.c's reconstruction loop, both allocator-thread shapes;
|
||||
* dead-by-construction today -- match_job_global_limit_() never
|
||||
* attaches a reason -- kept as the hook's historical carry-shape)
|
||||
* core.c:236/242/252/263 free_PendReason_ptr -- DESTRUCTION path
|
||||
* (xce_sched_core_free_schedobj / kill chain; no concurrent writer
|
||||
* can race it because the object is being torn down)
|
||||
* core_events.c:140 / schedobj.c:57 `= NULL` -- CONSTRUCTION
|
||||
* (calloc-initialized then explicitly NULLed; no reader yet exists)
|
||||
* sched_election.c:373 READ only -- allocator thread, sanctioned
|
||||
* consumer (fetched to build the pend decision)
|
||||
* plugins/fcfs.c:242-248 comment only -- the picker-thread ban note
|
||||
*/
|
||||
typedef struct {
|
||||
const char* path; /* relative to XCE_SCHEDULER_ROOT_PATH */
|
||||
const char* fragment; /* a substring that must appear on the line */
|
||||
} sanctioned_site_t;
|
||||
|
||||
static const sanctioned_site_t OWNERSHIP_SITES[] = {
|
||||
/* scheduler.c -- both alloc-chain sites, ALLOCATOR thread */
|
||||
{"src/scheduler.c", "MOVE_PTR(obj->reason, ctx.reason)"},
|
||||
{"src/scheduler.c", "free_PendReason_ptr(&(obj->reason))"},
|
||||
/* plugins/limit.c -- election hook carry-shape, ALLOCATOR thread */
|
||||
{"src/plugins/limit.c", "MOVE_PTR(obj->reason, ctx.reason)"},
|
||||
/* core.c -- destruction path */
|
||||
{"src/core.c", "free_PendReason_ptr(&(obj->u.primary->reason))"},
|
||||
{"src/core.c", "free_PendReason_ptr(&(obj->u.compound->reason))"},
|
||||
{"src/core.c", "free_PendReason_ptr(&(cobj->reason))"},
|
||||
{"src/core.c", "free_PendReason_ptr(&(obj->reason))"},
|
||||
/* construction: calloc'd then explicitly NULLed; no reader exists
|
||||
* yet, so no concurrent-writer hazard (core_events.c:140,
|
||||
* schedobj.c:57) */
|
||||
{"src/core_events.c", "primary->reason = NULL"},
|
||||
{"src/schedobj.c", "primary->reason = NULL"},
|
||||
};
|
||||
#define NUM_SITES (sizeof(OWNERSHIP_SITES) / sizeof(OWNERSHIP_SITES[0]))
|
||||
|
||||
/* Is this (file, line) sanctioned? A line is sanctioned if its file is in
|
||||
* the allowlist AND the line contains one of that file's fragments. */
|
||||
static int line_is_sanctioned(const char* rel_path, const char* line)
|
||||
{
|
||||
size_t i;
|
||||
|
||||
for (i = 0; i < NUM_SITES; i++) {
|
||||
if (strcmp(OWNERSHIP_SITES[i].path, rel_path) == 0) {
|
||||
/* Allowing any sanctioned fragment on any line of the file is
|
||||
* deliberately file-grained, not line-grained: line numbers
|
||||
* drift with every edit and would rot the allowlist into a
|
||||
* maintenance tax. The fragments are specific enough (the
|
||||
* exact lvalue expression) that a DIFFERENT reason write in
|
||||
* the same file -- the thing the scan exists to catch -- does
|
||||
* not match any of them. */
|
||||
if (strstr(line, OWNERSHIP_SITES[i].fragment) != NULL) {
|
||||
return 1;
|
||||
}
|
||||
}
|
||||
}
|
||||
return 0;
|
||||
}
|
||||
|
||||
/* is_comment_line - a line whose first non-whitespace character opens or
|
||||
* continues a comment (`*`, `/*`, `//`). Comment lines are SKIPPED by the
|
||||
* scans: this codebase's contract documentation itself lives in comments
|
||||
* that name the write shape ("cannot race MOVE_PTR(obj->reason, ...)" at
|
||||
* scheduler.c:496, fcfs.c's picker-ban note, sched_election.c's
|
||||
* bug-1521 paragraph), and clang-format keeps every comment line's
|
||||
* leading marker as the first non-blank character, so a leading-character
|
||||
* test is exact for this tree. (The scanner does not attempt full
|
||||
* block-comment state tracking -- trailing inline comments after real
|
||||
* code are NOT skipped, which only risks a false POSITIVE on a code line
|
||||
* whose trailing comment repeats the shape, not a false negative, and no
|
||||
* such line exists today.) */
|
||||
static int is_comment_line(const char* line)
|
||||
{
|
||||
while ((*line == ' ') || (*line == '\t')) {
|
||||
line++;
|
||||
}
|
||||
return (*line == '*' || (line[0] == '/' && line[1] == '*')
|
||||
|| (line[0] == '/' && line[1] == '/'));
|
||||
}
|
||||
|
||||
/* scan_file -- returns 1 if an unsanctioned reason-ownership line was
|
||||
* found (copied to out), 0 if clean, -1 if unreadable (hard failure). */
|
||||
static int scan_file(const char* root, const char* rel, char* out,
|
||||
size_t out_sz)
|
||||
{
|
||||
char path[4096];
|
||||
char* text;
|
||||
char* saveptr = NULL;
|
||||
char* line;
|
||||
int found = 0;
|
||||
|
||||
snprintf(path, sizeof(path), "%s/%s", root, rel);
|
||||
text = read_whole_file(path);
|
||||
if (text == NULL) {
|
||||
return -1;
|
||||
}
|
||||
|
||||
line = strtok_r(text, "\n", &saveptr);
|
||||
while (line != NULL) {
|
||||
if (!is_comment_line(line) && line_has_reason_ownership(line)
|
||||
&& !line_is_sanctioned(rel, line)) {
|
||||
found = 1;
|
||||
if ((out != NULL) && (out_sz > 0)) {
|
||||
snprintf(out, out_sz, "%s", line);
|
||||
}
|
||||
break;
|
||||
}
|
||||
line = strtok_r(NULL, "\n", &saveptr);
|
||||
}
|
||||
|
||||
free(text);
|
||||
return found;
|
||||
}
|
||||
|
||||
/* --------------------------------------------------------------------------
|
||||
* Self-tests: the detector must fire on the violation shapes and stay
|
||||
* quiet on the sanctioned shapes.
|
||||
* -------------------------------------------------------------------------- */
|
||||
|
||||
static void test_detector_fires_on_violation_shapes(void)
|
||||
{
|
||||
/* bug-1521's exact shape: a picker-thread write racing the allocator */
|
||||
TEST_ASSERT_TRUE(line_has_reason_ownership(
|
||||
" MOVE_PTR(job->schedobj->reason, ctx.reason);"));
|
||||
/* ownership release */
|
||||
TEST_ASSERT_TRUE(line_has_reason_ownership(
|
||||
" free_PendReason_ptr(&(obj->u.primary->reason));"));
|
||||
/* factory bypass aimed at the field */
|
||||
TEST_ASSERT_TRUE(line_has_reason_ownership(
|
||||
" obj->reason = (xce_pendreason_t*)calloc(1, sizeof(*r0));"));
|
||||
/* bare pointer-store */
|
||||
TEST_ASSERT_TRUE(line_has_reason_ownership(
|
||||
" obj->reason = other;"));
|
||||
/* calloc-first line that also carries a schedobj reason lvalue */
|
||||
TEST_ASSERT_TRUE(line_has_reason_ownership(
|
||||
" xce_pendreason_t* r1 = calloc(1, sizeof(xce_pendreason_t));"
|
||||
" obj->reason = r1;"));
|
||||
/* the compound/alter sub-struct bases are also targets */
|
||||
TEST_ASSERT_TRUE(line_has_reason_ownership(
|
||||
" free_PendReason_ptr(&(cobj->reason));"));
|
||||
}
|
||||
|
||||
static void test_detector_quiet_on_sanctioned_shapes(void)
|
||||
{
|
||||
/* the sanctioned allocator-thread READ (sched_election.c:373) --
|
||||
* pointer READ with no ownership verb and no bare store */
|
||||
TEST_ASSERT_FALSE(line_has_reason_ownership(
|
||||
" xce_pendreason_t* reason = job->schedobj->reason;"));
|
||||
/* ctx->reason assignments: the sched_ctx_t LOCAL shuttle field, not a
|
||||
* schedobj field (see line_has_reason_ownership's block comment) */
|
||||
TEST_ASSERT_FALSE(line_has_reason_ownership(
|
||||
" ctx->reason = make_pendreason(0);"));
|
||||
TEST_ASSERT_FALSE(line_has_reason_ownership(
|
||||
" if (ctx->reason != NULL) {"));
|
||||
/* a MOVE_PTR of a DIFFERENT field (allocs) -- no schedobj reason
|
||||
* lvalue on the line */
|
||||
TEST_ASSERT_TRUE(line_mentions_verb_but_not_reason(
|
||||
" MOVE_PTR(job->allocs, job->schedobj->alloc);"));
|
||||
TEST_ASSERT_FALSE(line_has_reason_ownership(
|
||||
" MOVE_PTR(job->allocs, job->schedobj->alloc);"));
|
||||
/* comments/discussion lines: comment lines are skipped by the SCANS
|
||||
* (is_comment_line), and lines whose substrings never share a line
|
||||
* are quiet for the detector itself */
|
||||
TEST_ASSERT_FALSE(line_has_reason_ownership(
|
||||
" * writing job->schedobj->reason races this thread's"));
|
||||
/* a comment line that DOES carry both substrings on one line (the
|
||||
* exact scheduler.c:496 / fcfs.c:243 shape) fires the raw detector --
|
||||
* which is precisely why the scans skip comment lines */
|
||||
TEST_ASSERT_TRUE(line_has_reason_ownership(
|
||||
" * MOVE_PTR(obj->reason, ...) (bug-1521 heap corruption)."));
|
||||
TEST_ASSERT_TRUE(is_comment_line(
|
||||
" * MOVE_PTR(obj->reason, ...) (bug-1521 heap corruption)."));
|
||||
TEST_ASSERT_TRUE(is_comment_line(" /* trailing open"));
|
||||
TEST_ASSERT_TRUE(is_comment_line(" // line comment"));
|
||||
TEST_ASSERT_FALSE(is_comment_line(
|
||||
" MOVE_PTR(job->allocs, job->schedobj->alloc);"));
|
||||
/* pend decision construction on a fresh reason (no schedobj reason
|
||||
* lvalue; decision->u.reason is the decision's own field, written by
|
||||
* make_pend_decision's single factory in decision.c) */
|
||||
TEST_ASSERT_FALSE(line_has_reason_ownership(
|
||||
" decision->u.reason = reason;"));
|
||||
}
|
||||
|
||||
static void test_sanctioned_table_matches_live_sites(void)
|
||||
{
|
||||
/* Every allowlisted fragment must actually be found in its file --
|
||||
* this catches the allowlist itself rotting (a renamed lvalue, a
|
||||
* moved site) and, symmetrically, guarantees each inventory entry
|
||||
* still exists in the shape the table claims. */
|
||||
size_t i;
|
||||
|
||||
for (i = 0; i < NUM_SITES; i++) {
|
||||
char path[4096];
|
||||
char* text;
|
||||
|
||||
snprintf(path, sizeof(path), "%s/%s", XCE_SCHEDULER_ROOT_PATH,
|
||||
OWNERSHIP_SITES[i].path);
|
||||
text = read_whole_file(path);
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(text, path);
|
||||
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(
|
||||
strstr(text, OWNERSHIP_SITES[i].fragment),
|
||||
OWNERSHIP_SITES[i].fragment);
|
||||
free(text);
|
||||
}
|
||||
}
|
||||
|
||||
/* --------------------------------------------------------------------------
|
||||
* The real scans
|
||||
* -------------------------------------------------------------------------- */
|
||||
|
||||
static void test_all_scanned_paths_are_readable(void)
|
||||
{
|
||||
size_t i;
|
||||
|
||||
for (i = 0; i < NUM_SCANNED; i++) {
|
||||
char path[4096];
|
||||
char* text;
|
||||
|
||||
snprintf(path, sizeof(path), "%s/%s", XCE_SCHEDULER_ROOT_PATH,
|
||||
SCANNED_RELATIVE_PATHS[i]);
|
||||
text = read_whole_file(path);
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(text, path);
|
||||
free(text);
|
||||
}
|
||||
}
|
||||
|
||||
static void test_no_unsanctioned_reason_ownership_in_scheduler_tree(void)
|
||||
{
|
||||
size_t i;
|
||||
|
||||
for (i = 0; i < NUM_SCANNED; i++) {
|
||||
char path[4096];
|
||||
char violating_line[512];
|
||||
char msg[4096 + 512 + 256];
|
||||
int rc;
|
||||
|
||||
snprintf(path, sizeof(path), "%s/%s", XCE_SCHEDULER_ROOT_PATH,
|
||||
SCANNED_RELATIVE_PATHS[i]);
|
||||
|
||||
rc = scan_file(XCE_SCHEDULER_ROOT_PATH, SCANNED_RELATIVE_PATHS[i],
|
||||
violating_line, sizeof(violating_line));
|
||||
|
||||
TEST_ASSERT_NOT_EQUAL_MESSAGE(-1, rc, path);
|
||||
|
||||
if (rc == 1) {
|
||||
snprintf(msg, sizeof(msg),
|
||||
"%s contains an unsanctioned reason-ownership write "
|
||||
"(bug-1521 contract): %s. Reason ownership belongs to "
|
||||
"the ALLOCATOR thread only (scheduler.c's "
|
||||
"sched_make_{primary,compound}_alloc_ and "
|
||||
"plugins/limit.c's limit_job_election carry today's "
|
||||
"sanctioned sites; core.c frees on destruction; "
|
||||
"constructors NULL the field). Either move this write "
|
||||
"into the allocator thread's call path, or -- if it "
|
||||
"is genuinely a new sanctioned site -- add it to "
|
||||
"OWNERSHIP_SITES in this test WITH a thread-ownership "
|
||||
"note. Offending line: %s",
|
||||
path, violating_line, violating_line);
|
||||
TEST_FAIL_MESSAGE(msg);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/* The bug-1521 shape itself: the picker's function body must contain no
|
||||
* reason-ownership operation. Extracted by brace matching from
|
||||
* sched_election.c (XCE_SCHED_ELECTION_C_PATH), so a future edit that
|
||||
* adds one fails here regardless of which line it landed on. Comment
|
||||
* lines inside the body are skipped for the same reason the file scan
|
||||
* skips them (sched_job_picker's body carries the bug-1521 discussion in
|
||||
* comments? No -- the picker body itself is clean prose-free code today;
|
||||
* the skip is future-proofing consistent with the file scan). */
|
||||
static void test_picker_body_has_no_reason_ownership(void)
|
||||
{
|
||||
char* src = read_whole_file(XCE_SCHED_ELECTION_C_PATH);
|
||||
char* body = NULL;
|
||||
char* saveptr = NULL;
|
||||
char* line;
|
||||
int violation = 0;
|
||||
char violating_line[512];
|
||||
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(src,
|
||||
"could not read sched_election.c for scan");
|
||||
|
||||
body = extract_function_body(src, "void* sched_job_picker(");
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(
|
||||
body, "sched_job_picker() definition not found in sched_election.c");
|
||||
|
||||
line = strtok_r(body, "\n", &saveptr);
|
||||
while (line != NULL) {
|
||||
if (!is_comment_line(line) && line_has_reason_ownership(line)) {
|
||||
violation = 1;
|
||||
snprintf(violating_line, sizeof(violating_line), "%s", line);
|
||||
break;
|
||||
}
|
||||
line = strtok_r(NULL, "\n", &saveptr);
|
||||
}
|
||||
|
||||
free(body);
|
||||
free(src);
|
||||
|
||||
TEST_ASSERT_FALSE_MESSAGE(
|
||||
violation, violating_line[0] != '\0' ? violating_line
|
||||
: "(line unavailable)");
|
||||
}
|
||||
|
||||
/* --------------------------------------------------------------------------
|
||||
* Suite entry for the sched_unit_test aggregate
|
||||
* -------------------------------------------------------------------------- */
|
||||
|
||||
void test_sched_reason_ownership_audit(void)
|
||||
{
|
||||
UnitySetTestFile(__FILE__);
|
||||
RUN_TEST(test_detector_fires_on_violation_shapes);
|
||||
RUN_TEST(test_detector_quiet_on_sanctioned_shapes);
|
||||
RUN_TEST(test_sanctioned_table_matches_live_sites);
|
||||
RUN_TEST(test_all_scanned_paths_are_readable);
|
||||
RUN_TEST(test_no_unsanctioned_reason_ownership_in_scheduler_tree);
|
||||
RUN_TEST(test_picker_body_has_no_reason_ownership);
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue