fix(libxce): bug-1600 -- makeGroup() freed rtti makeString() pointer via raw free()

makeGroup()'s existing-group update path (hit whenever a config-push
re-applies a group name that already has members, e.g. a retried config
push during controller failover) released xce_g_member_t.u.name via
FREEUP() (plain free()). u.name is allocated by makeString(), an
rtti-managed, refcounted pointer that is not a malloc()-base address --
freeing it directly corrupts the heap. group_killer() in the same file
already releases the identical field correctly via dereference().

Switch both FREEUP(m->u.name) call sites to dereference(m->u.name) to
match. Add a regression test that re-applies config for the same group
name twice, reproducing the exact ASAN bad-free at group.c:234 pre-fix.

This is the root cause of the reproducible xce_scheduler
"free(): invalid pointer" SIGABRT crashes seen during 3-attempt
controller-failover config-push retries (8 confirmed core dumps).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Rongsong Shen 2026-08-20 04:06:05 +00:00
parent 6522a73459
commit ddee884c1b
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
4 changed files with 62 additions and 3 deletions

View file

@ -61,6 +61,9 @@
- **`repl_info.c` (scheduler REPL state-inspection dumps) was already OVER this project's 800-line cap (883 lines) before this session even started** -- accumulated from three prior sessions' bug fixes (bug-1587/1590/1591) each adding a little without ever triggering a new sibling-file split. A task-12.5 rewrite of the resource-display functions brought it down to 856, still over. Flag this to the user rather than silently ignoring it or unilaterally doing a 4th-generation file split (repl.c -> repl_break.c/repl_info.c -> repl_params.c precedent) without being asked -- a split touches meson.build, test_sim_isolation.c's per-file scan macros, AND every white-box test TU's #include list (real, mechanical work, not worth taking on speculatively).
- **A `repl_split_tokens()`-style whitespace tokenizer with zero quote-awareness silently breaks any downstream parser that requires a whole-string-anchored regex match** (e.g. `parse_jobid()`'s `pmatch[0].rm_so==0 && pmatch[0].rm_eo==len` check) when a user quotes an argument out of habit (common when copy-pasting a value from another command's own output). The numeric fallback path (`strtoull()`) then silently parses the leading quote character as "not a digit" and returns 0 rather than erroring -- producing a confusing wrong-answer, not a clear parse failure. Worth checking for in any REPL/CLI tokenizer this codebase adds. See bug-1596.
- **A pointer returned by `makeString()` (rtti-managed, `lib/xstring.c`) must always be released via `dereference()`, never `FREEUP()`/plain `free()`** — `makeInstance()`/`objAlloc()` returns an offset into a combined header+data block, not the block's own `malloc()`-returned base address, and the string is refcounted (freed only when the refcount hits zero), not directly owned. bug-1600 (2026-08-19, production, not a test fixture — see also bug-1503's earlier test-fixture instance of the identical mismatch): `lib/group.c`'s `makeGroup()` "existing group" update branch (hit every time a config-push re-applies a group name that already has members, e.g. any retried/duplicate config push — no thread concurrency required) called `FREEUP(m->u.name)` on a `makeString()`-allocated field, while the SAME FILE's `group_killer()` destructor released the identical field correctly via `dereference(m->u.name)`. This caused the real, reproducible (8 confirmed core dumps) `xce_scheduler` `free(): invalid pointer` SIGABRT crash during controller-failover config-push retries. **When adding or auditing any free/release path for a field populated by `makeString()`, grep the same file/struct for an existing destructor and match its release call — don't assume `FREEUP()` is safe just because it's used for adjacent plain-`calloc`'d fields in the same struct.**
- **`lib/group.c`'s `makeGroup()` "existing group" update path takes only `RTTI_LOCK_READ` (a genuine shared `pthread_rwlock_rdlock`, confirmed via `kvmgr.c`) while mutating shared group member-list state, and the scheduler's entire config-push handler chain (`sched_init_config()` → `plugin_config_update()` → ... → `makeGroup()`) holds no serializing lock at all — while `scheduler_req_handler()` genuinely runs on whichever of the scheduler's 4 vcpu worker pthreads picked up that vtask (`vtaskmgr_init(..., 4)`).** `xce_ctx_reconnect()` reuses the same `ctx->ep` struct and `endpoint_init()` unconditionally resets `ep->busy=0`, so a reconnect racing with a still-running config-push handler can let a second, concurrently-dispatched config-push run `makeGroup()` on the same group object at the same time as the first. This is a REAL latent concurrency bug distinct from bug-1600's allocator mismatch (which alone was sufficient to explain the observed crash and was the one fixed) — flagged as a follow-up, not yet fixed; would need a write-lock scope redesign since `init_group_by_config()`'s callees (`get_group()`) take their own internal read lock, so naively holding a write lock across the whole function would self-deadlock on the non-recursive rwlock.
## Do-Not-Repeat
- Don't run `clang-format -i` on whole existing files here — the repo isn't clang-format-clean at baseline, so a full reformat buries the real diff; recovering via `git checkout HEAD -- <file>` on a file with other uncommitted work destroys that work irrecoverably.
- Before citing a bug as "still open" or a spec as "blocked," cross-check `.wolf/buglog.json`'s actual fix status and the spec's own tasks.md/commit history — blocked_on notes and prior narrative claims (including this file's own past entries) can be stale. **Self-caught recurrence (2026-08-19)**: while reconciling OTHER stale doc references, wrote a new tasks.md status line asserting "tasks 2.1-2.6 still carry no `_Verified:` annotations" by trusting a 2026-08-07 validation finding's own wording verbatim, without re-checking the file — they'd actually been backfilled the very next day (commit `d7b49690`, 2026-08-08). A dispatched verification agent caught it via `git log`/direct read. Lesson holds even for a claim you are ABOUT to write, not just one you're citing from memory: grep/read the actual current state of the specific lines before asserting their status, every time, no exceptions for "this one's probably still true."

View file

@ -2493,3 +2493,19 @@
| 20:00 | Edited src/scheduler/testsuite/test_repl_breakpoints_inspection.c | 1→2 lines | ~39 |
| 20:00 | Session end: 59 writes across 21 files (test_repl_breakpoints_inspection.c, test_sim_startup.c, test_sim_event_loop.c, meson.build, test_core_isolation.c) | 56 reads | ~622851 tok |
| 20:02 | Edited .kiro/steering/roadmap.md | 1→2 lines | ~394 |
| 20:03 | Session end: 60 writes across 21 files (test_repl_breakpoints_inspection.c, test_sim_startup.c, test_sim_event_loop.c, meson.build, test_core_isolation.c) | 56 reads | ~623792 tok |
| 20:21 | Session end: 60 writes across 21 files (test_repl_breakpoints_inspection.c, test_sim_startup.c, test_sim_event_loop.c, meson.build, test_core_isolation.c) | 56 reads | ~623792 tok |
| 20:26 | Session end: 60 writes across 21 files (test_repl_breakpoints_inspection.c, test_sim_startup.c, test_sim_event_loop.c, meson.build, test_core_isolation.c) | 57 reads | ~635646 tok |
| 20:29 | Session end: 60 writes across 21 files (test_repl_breakpoints_inspection.c, test_sim_startup.c, test_sim_event_loop.c, meson.build, test_core_isolation.c) | 59 reads | ~635646 tok |
| 20:37 | Session end: 60 writes across 21 files (test_repl_breakpoints_inspection.c, test_sim_startup.c, test_sim_event_loop.c, meson.build, test_core_isolation.c) | 60 reads | ~644876 tok |
## Session: 2026-08-20 20:42
| Time | Action | File(s) | Outcome | ~Tokens |
|------|--------|---------|---------|--------|
| 20:55 | Edited src/lib/testsuite/test_group.c | modified test_reconfigure_same_group_members() | ~370 |
| 20:55 | Session end: 1 writes across 1 files (test_group.c) | 19 reads | ~25243 tok |
| 20:57 | Session end: 1 writes across 1 files (test_group.c) | 19 reads | ~25243 tok |
| 21:01 | Edited src/lib/group.c | FREEUP() → dereference() | ~24 |
| 21:02 | Session end: 2 writes across 2 files (test_group.c, group.c) | 19 reads | ~25269 tok |
| 21:06 | Fixed production heap-corruption crash: makeGroup() FREEUP() vs dereference() allocator mismatch | src/lib/group.c, src/lib/testsuite/test_group.c | RED->GREEN confirmed via ASAN, logged bug-1600 | ~15000 |

View file

@ -231,7 +231,7 @@ xce_group_t* makeGroup(var mgr, xce_group_config_t* config,
continue;
}
if (m->is_group == 0) {
FREEUP(m->u.name);
dereference(m->u.name);
}
FREEUP(m);
@ -244,7 +244,7 @@ xce_group_t* makeGroup(var mgr, xce_group_config_t* config,
continue;
}
if (m->is_group == 0) {
FREEUP(m->u.name);
dereference(m->u.name);
}
FREEUP(m);

View file

@ -255,12 +255,51 @@ static void test_nest2(void)
destroy(mgr);
}
static void test_reconfigure_same_group_members(void)
{
var mgr;
xce_group_config_t gdata1 = {
.name = "g1",
.members = { .count = 1, .data = (char*[]) { "rshen" } }
};
xce_group_config_t gdata2 = {
.name = "g1",
.members = { .count = 1, .data = (char*[]) { "alice" } }
};
xce_group_t* g = NULL;
int rc = 0;
groupmgr_init(&mgr, 16, 16);
g = makeGroup(mgr, &gdata1, NULL);
TEST_ASSERT_TRUE(g != NULL);
/* Re-applying config for the SAME group name (e.g. a repeated config
* push) exercises makeGroup()'s existing-group update path, which must
* release the old members without corrupting the heap. */
g = makeGroup(mgr, &gdata2, NULL);
TEST_ASSERT_TRUE(g != NULL);
rc = is_in_group(g, "alice");
TEST_ASSERT_TRUE(rc != 0);
rc = is_in_group(g, "rshen");
TEST_ASSERT_TRUE(rc == 0);
destroy(mgr);
}
void test_group(void)
{
printf("\n>> group test\n\n");
xstring_table_init(139);
RUN_TEST(test_all);
RUN_TEST(test_empty);
RUN_TEST(test_all_1);
@ -271,6 +310,7 @@ void test_group(void)
RUN_TEST(test_exclude);
RUN_TEST(test_nest);
RUN_TEST(test_nest2);
RUN_TEST(test_reconfigure_same_group_members);
xstring_table_fini();
}