fix(controller): string resvals own their bytes -- rejoin use-after-free killed new primary (bug-1851)

update_val_res() memcpy'd the xce_val_t into w->host.resvals, so the
"type"/"model" string resources aliased the interned xstrings owned by
w->host.attrs. rejoin_worker() drops the old attrs strings, then
apply_join_state() -> resync_arm_round() resolves the previous join's
still-outstanding round, firing the worker hooks before
add_hostattrs_resvals() re-seeds resvals; the apiserver hook's
collect_resval() strdup()ed the freed hostmodel. Live: a newly promoted
primary died ~8s after election during an HA soak.

A string resval now stores a private strdup (taken before the old copy is
freed), and the owning arrays free their entries: host.resvals gets
kill_resval (replacing the FIXME NULL killer) and c->sharedinst's
resval_obj_killer (previously a no-op) frees the string and the value --
both also leaked every xce_val_t before. Also closes the same alias for
worker-reported string resvals and config values.

Regression test: test_rejoin_worker_string_resvals_survive_outstanding_round
(RED reproduced the live ASAN stack; GREEN after). meson test 109/109.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

src/controller/cluster.c

@@ -1561,11 +1561,19 @@ static void queue_obj_killer(void* arg)
     }
 }
 
+/* bug-1851: c->sharedinst owns each xce_val_t (update_val_res, worker.c)
+ * and, for a string value, a private strdup()ed copy. */
 static void resval_obj_killer(void* arg)
 {
-    if (arg == NULL) {
+    xce_val_t* v = (xce_val_t*)arg;
+
+    if (v == NULL) {
         return;
     }
+    if (v->which == Value_u_svalue) {
+        FREEUP(v->u.svalue);
+    }
+    free(v);
 }
 
 static void config_obj_killer(void* arg)

src/controller/testsuite/test_worker_join.c

@@ -14,6 +14,7 @@
 #include <rtti.h>
 
 #include <errno.h>
+#include <stdarg.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -28,6 +29,7 @@
                                * Request_body_* constants */
 #include "jobj.h" /* worker-rejoin-resync task 6.1: makeJob, for a real
                    * non-terminal job the arm-round wiring tests need */
+#include "hook.h" /* bug-1851: xce_hook_init/xce_add_hook */
 #include "kvmgr.h"
 #include "netio.h" /* worker-rejoin-resync task 6.1: endpoint_get/
                     * endpoint_deq/ITPTR_TO_PACKET, for real wire capture */
@@ -1007,6 +1009,137 @@ static void test_rejoin_worker_no_round_when_only_terminal_jobs(void)
     printf("test_rejoin_worker_no_round_when_only_terminal_jobs: PASS\n");
 }
 
+/*
+ * bug-1851: a string resval ("type"/"model") must own its bytes rather
+ * than alias the interned xstring in w->host.attrs. rejoin_worker drops
+ * the old attrs strings, then apply_join_state -> resync_arm_round
+ * resolves the PREVIOUS join's still-outstanding round, which fires the
+ * worker hooks (resync_resolve_round -> persist_notify_worker_status_
+ * updated) BEFORE add_hostattrs_resvals re-seeds resvals. Live, the
+ * apiserver hook's collect_resval strdup()ed the freed hostmodel and the
+ * newly-promoted primary died (ASAN heap-use-after-free). This hook stands
+ * in for it: it reads both string resvals at exactly that point.
+ */
+static char g_1851_seen_type[64];
+static char g_1851_seen_model[64];
+static int  g_1851_hook_calls;
+
+static void copy_string_resval(worker_t* w, const char* name, char* out,
+                               size_t out_sz)
+{
+    xce_resource_t* r = NULL;
+    xce_val_t*      v;
+
+    lock(g_tc.c.resmgr, RTTI_LOCK_READ);
+    key_find(g_tc.c.resmgr, (void*)name, (void**)&r);
+    unlock(g_tc.c.resmgr);
+    if (r == NULL || r->idx < 0 || r->idx >= w->host.resvals.caps)
+        return;
+    v = (xce_val_t*)w->host.resvals.data[r->idx];
+    if (v != NULL && v->which == Value_u_svalue && v->u.svalue != NULL)
+        snprintf(out, out_sz, "%s", v->u.svalue);
+}
+
+static int bug1851_worker_hook(int type, int situation, int detail,
+                               void* cls, va_list ap)
+{
+    (void)type;
+    (void)situation;
+    (void)detail;
+    (void)cls;
+    (void)va_arg(ap, cluster_t*);
+    worker_t* w = va_arg(ap, worker_t*);
+
+    g_1851_hook_calls++;
+    copy_string_resval(w, "type", g_1851_seen_type, sizeof(g_1851_seen_type));
+    copy_string_resval(w, "model", g_1851_seen_model,
+                       sizeof(g_1851_seen_model));
+    return 0;
+}
+
+static void test_rejoin_worker_string_resvals_survive_outstanding_round(void)
+{
+    setup_test_cluster();
+    TEST_ASSERT_EQUAL_INT(
+        0, xce_hook_init(&g_tc.c.hooksys, XCE_CONTROLLER_HOOK_MAX));
+    TEST_ASSERT_EQUAL_INT(0, xce_add_hook(&g_tc.c.hooksys,
+                                          XCE_CONTROLLER_HOOK_WORKER,
+                                          bug1851_worker_hook, NULL));
+    g_1851_hook_calls    = 0;
+    g_1851_seen_type[0]  = '\0';
+    g_1851_seen_model[0] = '\0';
+
+    /* A non-terminal job on the host, so each rejoin arms a round and the
+     * second rejoin finds the first one still outstanding. */
+    job_t* j = seed_test_job(96851, JOB_STAT_RUN);
+    TEST_ASSERT_NOT_NULL(j);
+    j->job.allocs = make_test_job_alloc(96851, "uaf-host-1851");
+    TEST_ASSERT_NOT_NULL(j->job.allocs);
+
+    hostattrs_t attrs;
+    memset(&attrs, 0, sizeof(attrs));
+    attrs.hosttype  = makeString("TYPE-1851-A");
+    attrs.hostmodel = makeString("MODEL-1851-A");
+    attrs.maxcpus   = 4;
+    worker_t* w     = make_test_worker("uaf-host-1851", &attrs);
+    TEST_ASSERT_NOT_NULL(w);
+    /* makeWorker took its own references; drop the test's so w->host.attrs
+     * holds the ONLY reference to the "A" strings. */
+    dereference(attrs.hosttype);
+    dereference(attrs.hostmodel);
+
+    xce_hostconfig_t hconfig;
+    memset(&hconfig, 0, sizeof(hconfig));
+    hconfig.hosttype  = (char*)"!";
+    hconfig.hostmodel = (char*)"!";
+    hconfig.mxj       = 4;
+
+    endpoint_t ep;
+    memset(&ep, 0, sizeof(ep));
+
+    /* First join: seeds resvals from the "A" strings, arms round #1. */
+    TEST_ASSERT_GREATER_OR_EQUAL(0, apply_join_state(&g_tc.c, w, &ep, &hconfig));
+    TEST_ASSERT_NOT_NULL_MESSAGE(w->resync,
+        "fixture: the first join must leave a round outstanding");
+
+    /* Second join with different strings: must resolve round #1 (firing
+     * the hook) while "A" is being released. */
+    hostattrs_t new_attrs;
+    memset(&new_attrs, 0, sizeof(new_attrs));
+    new_attrs.hosttype  = makeString("TYPE-1851-B");
+    new_attrs.hostmodel = makeString("MODEL-1851-B");
+    new_attrs.maxcpus   = 4;
+    g_1851_hook_calls   = 0;
+
+    TEST_ASSERT_GREATER_OR_EQUAL(
+        0, rejoin_worker(&g_tc.c, w, &ep, &new_attrs, &hconfig));
+
+    TEST_ASSERT_GREATER_THAN_INT_MESSAGE(0, g_1851_hook_calls,
+        "fixture: resolving the outstanding round must fire the worker hook");
+    TEST_ASSERT_EQUAL_STRING_MESSAGE("TYPE-1851-A", g_1851_seen_type,
+        "the hook must read the previous (still-valid) type resval, not "
+        "freed memory");
+    TEST_ASSERT_EQUAL_STRING_MESSAGE("MODEL-1851-A", g_1851_seen_model,
+        "the hook must read the previous (still-valid) model resval, not "
+        "freed memory");
+
+    /* Ownership: the resval is a private copy, never the attrs xstring. */
+    xce_val_t val;
+    memset(&val, 0, sizeof(val));
+    TEST_ASSERT_EQUAL_INT(1, get_resval(&g_tc.c, (char*)"model",
+                                        (char*)"uaf-host-1851", &val));
+    TEST_ASSERT_EQUAL_STRING("MODEL-1851-B", val.u.svalue);
+    TEST_ASSERT_TRUE_MESSAGE(val.u.svalue != w->host.attrs->hostmodel,
+        "a string resval must own its bytes, not alias w->host.attrs");
+
+    dereference(new_attrs.hosttype);
+    dereference(new_attrs.hostmodel);
+    xce_hook_fini(&g_tc.c.hooksys);
+    teardown_test_cluster();
+    printf("test_rejoin_worker_string_resvals_survive_outstanding_round: "
+           "PASS\n");
+}
+
 /*
  * Req 1.3: the SAME trigger condition -- no special-casing needed -- must
  * apply to a host's first-ever join (init_worker), not just a rejoin.
@@ -1526,6 +1659,7 @@ void test_worker_join(void)
     RUN_TEST(test_rejoin_worker_arms_reconciliation_round_when_jobs_exist);
     RUN_TEST(test_rejoin_worker_no_round_when_no_jobs);
     RUN_TEST(test_rejoin_worker_no_round_when_only_terminal_jobs);
+    RUN_TEST(test_rejoin_worker_string_resvals_survive_outstanding_round);
     RUN_TEST(test_init_worker_arms_reconciliation_round_when_jobs_exist);
     RUN_TEST(test_worker_send_resync_query_sends_armed_round);
     RUN_TEST(test_worker_send_resync_query_sends_nothing_when_no_round);

src/controller/worker.c

@@ -96,7 +96,29 @@ static int update_val_res(cluster_t* c, worker_t* w, xce_resource_t* r,
         }
     }
 
-    memcpy(v, val, sizeof(*v));
+    /* bug-1851: a stored string value owns a private copy. Aliasing the
+     * caller's pointer (w->host.attrs' interned hosttype/hostmodel, a
+     * config value, or a decoded request) left it dangling once that owner
+     * released it -- rejoin_worker drops the old attrs strings before the
+     * resvals are re-seeded, and a worker hook read them in between. The
+     * copy is made before the old one is freed, so val may alias v. */
+    if (val->which == Value_u_svalue) {
+        char* s = strdup(val->u.svalue != NULL ? val->u.svalue : "");
+
+        if (s == NULL) {
+            return -ENOMEM;
+        }
+        if (v->which == Value_u_svalue) {
+            FREEUP(v->u.svalue);
+        }
+        v->which    = Value_u_svalue;
+        v->u.svalue = s;
+    } else {
+        if (v->which == Value_u_svalue) {
+            FREEUP(v->u.svalue);
+        }
+        memcpy(v, val, sizeof(*v));
+    }
 
     /* bug-1568 instrumentation: the actual VALUE being written into
      * w->host.resvals[idx] (or c->sharedinst[idx] for a shared resource)