diff --git a/.wolf/buglog.json b/.wolf/buglog.json index c1351304..c08cbd05 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -14378,18 +14378,18 @@ }, { "id": "bug-1824", - "status": "open", - "status_updated": "2026-09-24", - "status_note": "repl_info.c bounds check uses zarray count instead of caps", + "status": "fixed", + "status_updated": "2026-09-30", + "status_note": "", "component": "scheduler", - "severity": "crash", + "severity": "wrong-result", "timestamp": "2026-09-17T02:45:00Z", "last_seen": "2026-09-19T09:00:00Z", "occurrences": 1, "error_message": "out-of-scope latent bug discovered while implementing scheduler-sim-diagnostics task 3.1 (new…\n\nNOT A CRASH -- pre-existing, out-of-scope latent bug discovered while implementing scheduler-sim-diagnostics task 3.1 (new repl_info_limit() live-usage display). repl_info_resource_one() (src/scheduler/src/repl_info.c ~line 996) bounds-checks a resource's zarray index via `(r->idx < 0) || (r->idx >= sim->core.sharedInst.count)` before calling zarray_get() -- but zarray_t.count tracks the number of POPULATED slots (incremented only when a previously-NULL slot becomes non-NULL via zarray_put_ex/zarray_store), not the array's allocated capacity (.caps, grown by zarray_ensure_items). A sparsely-populated zarray (e.g. only a high index ever zarray_put() -- realistic if resources are registered out of index order, or an index is skipped) can have count=1 while a LOWER, unpopulated index is itself < count and would incorrectly pass this check and be read as though populated, or a HIGHER populated index can exceed .count and be incorrectly rejected as absent even though zarray_get()'s own .caps-based check would have safely returned it (or correctly returned NULL for a genuinely-unpopulated-but-in-capacity slot, since zarray_ensure_items() NULL-initializes newly-grown slots). Net effect: repl_info_resource_one() can report a resource as having no value ('value=(none)') when a real, populated value exists at that index but the array is sparse, OR (less likely) skip a real bounds-safety net zarray_get_ex() would otherwise provide.", - "file": "src/scheduler/src/repl_info.c", + "file": "src/scheduler/src/repl_info.c (repl_info_resource_one), src/lib/zarray.c (zarray_get_ex)", "root_cause": "repl_info_resource_one()'s pre-check compares r->idx against sim->core.sharedInst.count (populated-slot count) instead of relying on zarray_get_ex()'s own bounds check, which is against .caps (allocated capacity) -- these two counters diverge for any sparsely-populated zarray. Confirmed by direct reading of src/lib/zarray.c: zarray_get_ex() (~line 341-362) checks only `idx >= array->caps` before dereferencing array->data[idx]; zarray_put_ex()/zarray_store() are the only writers of .count, incrementing it exclusively when filling a previously-NULL slot -- so .count is a 'high-water mark of contiguous-from-zero fill,' not a true capacity bound. Separately, and more seriously: neither the .count-based check NOR zarray_get_ex()'s own .caps check safely rejects a NEGATIVE idx -- `idx >= caps` is false for any negative idx when caps >= 0, so a negative idx falls through to `array->data[idx]`, a genuine out-of-bounds read. A negative idx is reachable in this codebase: add_vres() (src/lib/xcelimit.c ~407-435) unconditionally sets a newly-allocated xce_resource_t's ->idx to -1 before key_add() runs, only overwriting it with the real slot index on success -- a caller that reaches a resource-value read before/without that overwrite succeeding would pass a negative idx through. repl_info_resource_one()'s existing `r->idx < 0` half of its check does happen to guard this specific negative case correctly (by luck of ordering, not by understanding zarray's real contract) -- the count-vs-caps confusion is the part that is a genuine, distinct latent bug.", - "fix": "NOT FIXED -- discovered and characterized during scheduler-sim-diagnostics task 3.1 (limit-usage display), out of that task's scope (task 3.1 only touches its OWN new code path, repl_print_limit_usage(), which was written from scratch to avoid this exact mistake: it omits the imprecise .count pre-check entirely, relies on zarray_get_ex()'s own .caps bound for the upper end, and adds an explicit idx<0 guard of its own -- verified correct and reviewed independently, see this session's git history for scheduler-sim-diagnostics task 3.1). repl_info_resource_one() itself was deliberately left untouched to keep task 3.1 in its declared boundary. Recommended fix when picked up: replace the `r->idx >= sim->core.sharedInst.count` half of the pre-check with reliance on zarray_get()'s own .caps-based bound (mirroring task 3.1's new repl_print_limit_usage() pattern), keeping the `r->idx < 0` guard. Likely benign in practice today only because resource registration order in this codebase's current production paths tends to be dense/contiguous from index 0, so .count and .caps rarely diverge for sim->core.sharedInst specifically -- but this is an incidental property of current callers, not a guarantee, and worth a regression test with a deliberately sparse fixture (zarray_put() at a high index with no lower indices ever populated) before relying on it.", + "fix": "src/lib/zarray.c: zarray_get_ex() returns NULL for idx < 0 as well as idx >= caps, so every zarray_get() caller is safe from a negative index. src/scheduler/src/repl_info.c: repl_info_resource_one() no longer pre-bounds the slot by sharedInst.count or sharedInUse.count (populated-slot counts); it relies on zarray_get()'s own capacity check, which returns NULL for an unpopulated or out-of-range slot. A search for other `idx < / >= .count` bounds in scheduler, controller, lib, worker and harness found no other site.\n\nVerification:\n- Test: test_info_resource_shows_value_when_slot_is_above_count (src/scheduler/testsuite/test_repl_resource_table.c): RED before the fix (`bug-1824: a populated shared slot at an index >= sharedInst.count must still display its value`), GREEN after. test_zarray_get_negative_index_returns_null (src/lib/testsuite/test_zarray.c): RED before (ASAN heap-buffer-overflow in zarray_get_ex), GREEN after\n- Suites: xcelib_test 700/700, scheduler 90/90, controller 1258/1258, worker 13/13, asan, 0 sanitizer reports\n- Live: n/a -- a bounds check in a library getter and a REPL display path, both covered by the unit tests above", "tags": [ "scheduler", "repl", @@ -14400,7 +14400,9 @@ "discovered-during-scheduler-sim-diagnostics" ], "related_bugs": [], - "fix_commits": [], + "fix_commits": [ + "16b7aa4b" + ], "record": ".wolf/bugs/bug-1824.md" }, { diff --git a/.wolf/bugs/bug-1824.md b/.wolf/bugs/bug-1824.md index 0b98649b..972363c6 100644 --- a/.wolf/bugs/bug-1824.md +++ b/.wolf/bugs/bug-1824.md @@ -1,16 +1,17 @@ --- id: "bug-1824" -status: "open" -status_updated: "2026-09-24" -status_note: "repl_info.c bounds check uses zarray count instead of caps" +status: "fixed" +status_updated: "2026-09-30" +status_note: "" component: "scheduler" -severity: "crash" +severity: "wrong-result" timestamp: "2026-09-17T02:45:00Z" last_seen: "2026-09-19T09:00:00Z" occurrences: 1 -files: ["src/scheduler/src/repl_info.c"] +files: ["src/scheduler/src/repl_info.c (repl_info_resource_one)", "src/lib/zarray.c (zarray_get_ex)"] tags: ["scheduler", "repl", "zarray", "bounds-check", "out-of-bounds-read", "latent", "discovered-during-scheduler-sim-diagnostics"] related: [] +fix_commits: ["16b7aa4b"] legacy: true --- @@ -20,19 +21,30 @@ legacy: true NOT A CRASH -- pre-existing, out-of-scope latent bug discovered while implementing scheduler-sim-diagnostics task 3.1 (new repl_info_limit() live-usage display). repl_info_resource_one() (src/scheduler/src/repl_info.c ~line 996) bounds-checks a resource's zarray index via `(r->idx < 0) || (r->idx >= sim->core.sharedInst.count)` before calling zarray_get() -- but zarray_t.count tracks the number of POPULATED slots (incremented only when a previously-NULL slot becomes non-NULL via zarray_put_ex/zarray_store), not the array's allocated capacity (.caps, grown by zarray_ensure_items). A sparsely-populated zarray (e.g. only a high index ever zarray_put() -- realistic if resources are registered out of index order, or an index is skipped) can have count=1 while a LOWER, unpopulated index is itself < count and would incorrectly pass this check and be read as though populated, or a HIGHER populated index can exceed .count and be incorrectly rejected as absent even though zarray_get()'s own .caps-based check would have safely returned it (or correctly returned NULL for a genuinely-unpopulated-but-in-capacity slot, since zarray_ensure_items() NULL-initializes newly-grown slots). Net effect: repl_info_resource_one() can report a resource as having no value ('value=(none)') when a real, populated value exists at that index but the array is sparse, OR (less likely) skip a real bounds-safety net zarray_get_ex() would otherwise provide. ## Environment -_Not recorded (migrated from buglog.json)._ +- OS: no /etc/os-release; Docker container; NixOS kernel; kernel Linux 6.18.33 x86_64 +- libc: glibc 2.42-47 (interpreter /nix/store/wb6rhpznjfczwlwx23zmdrrw74bayxw4-glibc-2.42-47/lib/ld-linux-x86-64.so.2) +- Toolchain: gcc (GCC) 15.2.0, meson 1.9.1 +- Build: asan; meson runs the suites with ASAN_OPTIONS=detect_leaks=0 +- Code: found by code reading during scheduler-sim-diagnostics task 3.1 (Sep 2026); reproduced and fixed at fd61721b, 2026-09-29 +- Deployment: n/a -- xce_scheduler simulator REPL (`info resource`) and libxce ## Reproduction -_Not recorded (migrated from buglog.json)._ +1. `bash src/tools/xce-build.sh scheduler` -- test_info_resource_shows_value_when_slot_is_above_count (src/scheduler/testsuite/test_repl_resource_table.c) registers a SHARED resource, sets its value, then lowers sim.core.sharedInst.count below the resource's populated slot and runs `info resource cluster_load1m`: it prints `value=(none)`. +2. `bash src/tools/xce-build.sh lib` -- test_zarray_get_negative_index_returns_null (src/lib/testsuite/test_zarray.c) calls zarray_get(z, -1): ASAN `heap-buffer-overflow ... READ of size 8 ... in zarray_get_ex ../lib/zarray.c:356`. +- Frequency: deterministic (both tests), 2026-09-29 +- Evidence: the two tests above; build logs not kept ## Root cause repl_info_resource_one()'s pre-check compares r->idx against sim->core.sharedInst.count (populated-slot count) instead of relying on zarray_get_ex()'s own bounds check, which is against .caps (allocated capacity) -- these two counters diverge for any sparsely-populated zarray. Confirmed by direct reading of src/lib/zarray.c: zarray_get_ex() (~line 341-362) checks only `idx >= array->caps` before dereferencing array->data[idx]; zarray_put_ex()/zarray_store() are the only writers of .count, incrementing it exclusively when filling a previously-NULL slot -- so .count is a 'high-water mark of contiguous-from-zero fill,' not a true capacity bound. Separately, and more seriously: neither the .count-based check NOR zarray_get_ex()'s own .caps check safely rejects a NEGATIVE idx -- `idx >= caps` is false for any negative idx when caps >= 0, so a negative idx falls through to `array->data[idx]`, a genuine out-of-bounds read. A negative idx is reachable in this codebase: add_vres() (src/lib/xcelimit.c ~407-435) unconditionally sets a newly-allocated xce_resource_t's ->idx to -1 before key_add() runs, only overwriting it with the real slot index on success -- a caller that reaches a resource-value read before/without that overwrite succeeding would pass a negative idx through. repl_info_resource_one()'s existing `r->idx < 0` half of its check does happen to guard this specific negative case correctly (by luck of ordering, not by understanding zarray's real contract) -- the count-vs-caps confusion is the part that is a genuine, distinct latent bug. ## Fix -NOT FIXED -- discovered and characterized during scheduler-sim-diagnostics task 3.1 (limit-usage display), out of that task's scope (task 3.1 only touches its OWN new code path, repl_print_limit_usage(), which was written from scratch to avoid this exact mistake: it omits the imprecise .count pre-check entirely, relies on zarray_get_ex()'s own .caps bound for the upper end, and adds an explicit idx<0 guard of its own -- verified correct and reviewed independently, see this session's git history for scheduler-sim-diagnostics task 3.1). repl_info_resource_one() itself was deliberately left untouched to keep task 3.1 in its declared boundary. Recommended fix when picked up: replace the `r->idx >= sim->core.sharedInst.count` half of the pre-check with reliance on zarray_get()'s own .caps-based bound (mirroring task 3.1's new repl_print_limit_usage() pattern), keeping the `r->idx < 0` guard. Likely benign in practice today only because resource registration order in this codebase's current production paths tends to be dense/contiguous from index 0, so .count and .caps rarely diverge for sim->core.sharedInst specifically -- but this is an incidental property of current callers, not a guarantee, and worth a regression test with a deliberately sparse fixture (zarray_put() at a high index with no lower indices ever populated) before relying on it. +src/lib/zarray.c: zarray_get_ex() returns NULL for idx < 0 as well as idx >= caps, so every zarray_get() caller is safe from a negative index. src/scheduler/src/repl_info.c: repl_info_resource_one() no longer pre-bounds the slot by sharedInst.count or sharedInUse.count (populated-slot counts); it relies on zarray_get()'s own capacity check, which returns NULL for an unpopulated or out-of-range slot. A search for other `idx < / >= .count` bounds in scheduler, controller, lib, worker and harness found no other site. ## Verification -_Not recorded (migrated from buglog.json)._ +- Test: test_info_resource_shows_value_when_slot_is_above_count (src/scheduler/testsuite/test_repl_resource_table.c): RED before the fix (`bug-1824: a populated shared slot at an index >= sharedInst.count must still display its value`), GREEN after. test_zarray_get_negative_index_returns_null (src/lib/testsuite/test_zarray.c): RED before (ASAN heap-buffer-overflow in zarray_get_ex), GREEN after +- Suites: xcelib_test 700/700, scheduler 90/90, controller 1258/1258, worker 13/13, asan, 0 sanitizer reports +- Live: n/a -- a bounds check in a library getter and a REPL display path, both covered by the unit tests above ## References -_Not recorded (migrated from buglog.json)._ +- Fix commit: 16b7aa4b +- Related: bug-1688 (same count-vs-capacity mistake in info worker), scheduler-sim-diagnostics task 3.1