diff --git a/.kiro/steering/scheduler-reason-ownership.md b/.kiro/steering/scheduler-reason-ownership.md new file mode 100644 index 00000000..dcec7159 --- /dev/null +++ b/.kiro/steering/scheduler-reason-ownership.md @@ -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. diff --git a/scheduler-hardening-plan.md b/scheduler-hardening-plan.md index d33abdda..5bd805ad 100644 --- a/scheduler-hardening-plan.md +++ b/scheduler-hardening-plan.md @@ -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 个会话。 --- diff --git a/src/scheduler/testsuite/meson.build b/src/scheduler/testsuite/meson.build index e844effe..32db5229 100644 --- a/src/scheduler/testsuite/meson.build +++ b/src/scheduler/testsuite/meson.build @@ -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', diff --git a/src/scheduler/testsuite/sched_unit_runner.c b/src/scheduler/testsuite/sched_unit_runner.c index 369d277e..d99fd76a 100644 --- a/src/scheduler/testsuite/sched_unit_runner.c +++ b/src/scheduler/testsuite/sched_unit_runner.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), diff --git a/src/scheduler/testsuite/test_reason_ownership_audit.c b/src/scheduler/testsuite/test_reason_ownership_audit.c new file mode 100644 index 00000000..36d58019 --- /dev/null +++ b/src/scheduler/testsuite/test_reason_ownership_audit.c @@ -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 +#include +#include + +#include + +#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: ` =` 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); +}