| Author | ![[]](/git/avatar/c50933ca2aa61e0fe2c43d46bb6b59cb_avatar) Andy Green <andy@warmcat.com> 2026-09-25 02:05 UTC | | Committer | Andy Green <andy@warmcat.com> 2026-09-25 02:05 UTC | | Tree | 319845b725729a7bfe551424aa37a0014be4fc1f ![]() Raw Patch | | | ![]() builder: fix nspawn lifecycle NULL derefs and the reap cb success path | builder: fix nspawn lifecycle NULL derefs and the reap cb success path
Three NULL derefs:
- the stdwsi CLOSE handler reads ns->reap_cb_called inside
if (op && op->lsp), but ns is op->ns, which saib_sub_cleaner_cb()
deliberately clears when it gives up on a child that refused to die
- the nspawn census loop dereferences xns->task->uuid, which the
sai-device log proxy UDS failures below could leave NULL
- those UDS failures returned -1 with the nspawn already on
sp->nspawn_owner and no task bound, leaking it onto the list forever
and leaving the server waiting on a task nothing would ever finish.
Fail the task through bail: instead
And two flow bugs in sai_lsp_reap_cb()'s success path, either of which
could turn a step that exited 0 into a failure or a hang:
- sai_metrics_hash() failing did goto fail, reporting "FAILED, exit
code: 0" for a step that succeeded
- failing to queue the metrics did a bare return, leaving the ns in
EXECUTING_STEPS with a dead child, no final log chunk, no grace timer
and no status update, so the task hung on the server until something
else knocked it over
Neither is a reason not to complete a step, so both just skip the
metrics now. ns->spm->ss is also checked before the send: only ns->spm
was, and saib_srv_queue_tx() dereferences the handle straight away, so a
server link that had dropped was a NULL deref.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
diff --git a/src/builder/b-nspawn.c b/src/builder/b-nspawn.c
index d0bd5c5..da79338 100644
--- a/src/builder/b-nspawn.c
+++ b/src/builder/b-nspawn.c
@@ -268,8 +268,13 @@ callback_sai_stdwsi(struct lws *wsi, enum lws_callback_reasons reason,
op->ns->stdwsi[ch] = NULL;
}
if (op && op->lsp) {
+ /*
+ * ns is op->ns, which saib_sub_cleaner_cb() clears
+ * when it gives up on a child that refused to die, so
+ * it can legitimately be NULL here
+ */
if (lws_spawn_stdwsi_closed(op->lsp, wsi) &&
- ns->reap_cb_called) {
+ ns && ns->reap_cb_called) {
lwsl_notice("%s: freeing op from stdwsi_cb\n", __func__);
free(op);
}
@@ -476,14 +481,20 @@ sai_lsp_reap_cb(void *opaque, const lws_spawn_resource_us_t *res, siginfo_t *si,
* server so it can store them.
*/
- if (!op->spawn || !ns->spm)
+ if (!op->spawn || !ns->spm || !ns->spm->ss)
goto skip;
memset(&m, 0, sizeof(m));
+ /*
+ * Not being able to key the metrics is not a reason to fail a step
+ * that exited 0... the metrics are just bookkeeping
+ */
if (sai_metrics_hash((uint8_t *)m.key, sizeof(m.key),
- ns->sp->name, ns->task->build, ns->project_name, ns->ref))
- goto fail;
+ ns->sp->name, ns->task->build, ns->project_name, ns->ref)) {
+ lwsl_notice("%s: unable to hash metrics key\n", __func__);
+ goto skip;
+ }
lws_strncpy(m.builder_name, ns->sp->name, sizeof(m.builder_name));
lws_strncpy(m.project_name, ns->project_name, sizeof(m.project_name));
@@ -499,10 +510,16 @@ sai_lsp_reap_cb(void *opaque, const lws_spawn_resource_us_t *res, siginfo_t *si,
m.parallel = ns->task->parallel;
m.step = ns->task->build_step + 1;
+ /*
+ * Likewise: if we can't get the metrics away, complete the step
+ * anyway. Returning here used to leave the ns in EXECUTING_STEPS with
+ * a dead child, no grace timer and no status update, so the task hung
+ * on the server until something else knocked it over.
+ */
if (saib_srv_queue_json_fragments_helper(ns->spm->ss,
lsm_schema_map_build_metric,
LWS_ARRAY_SIZE(lsm_schema_map_build_metric), &m))
- return;
+ lwsl_notice("%s: unable to queue step metrics\n", __func__);
skip:
diff --git a/src/builder/b-task.c b/src/builder/b-task.c
index f423e06..ed89401 100644
--- a/src/builder/b-task.c
+++ b/src/builder/b-task.c
@@ -922,7 +922,8 @@ saib_consider_allocating_task(struct sai_plat_server *spm, lws_struct_args_t *a,
lws_start_foreach_dll_safe(struct lws_dll2 *, d, d1, sp->nspawn_owner.head) {
struct sai_nspawn *xns = lws_container_of(d, struct sai_nspawn, list);
- lwsl_notice("%s: nspawn_census: %s\n", __func__, xns->task->uuid);
+ lwsl_notice("%s: nspawn_census: %s\n", __func__,
+ xns->task ? xns->task->uuid : "(no task)");
} lws_end_foreach_dll_safe(d, d1);
lwsl_notice("%s:\n", __func__);
@@ -1050,9 +1051,11 @@ saib_consider_allocating_task(struct sai_plat_server *spm, lws_struct_args_t *a,
if (saib_create_listen_uds(builder.context, &ns->slp_control,
&ns->vhosts[0])) {
- lwsl_err("%s: Failed to create ctl log proxy listen UDS %s\n",
- __func__, ns->slp_control.sockpath);
- return -1;
+ saib_task_logf(spm, ns, NULL,
+ "Unable to create the sai-device control "
+ "log proxy socket %s",
+ ns->slp_control.sockpath);
+ goto bail;
}
for (n = 0; n < (int)LWS_ARRAY_SIZE(ns->slp); n++) {
@@ -1070,9 +1073,11 @@ saib_consider_allocating_task(struct sai_plat_server *spm, lws_struct_args_t *a,
if (saib_create_listen_uds(builder.context, &ns->slp[n],
&ns->vhosts[n + 1])) {
- lwsl_err("%s: Failed to create log proxy listen UDS %s\n",
- __func__, ns->slp[n].sockpath);
- return -1;
+ saib_task_logf(spm, ns, NULL,
+ "Unable to create the sai-device "
+ "tty%d log proxy socket %s",
+ n, ns->slp[n].sockpath);
+ goto bail;
}
}
}
|