On Thu, Sep 3, 2026 at 9:35 AM Dave Voutila <dv@sisu.io> wrote:
>
> Nick Owens <mischief@offblast.org> writes:
>
> > unlike libutil's imsg, vmd's proc_forward_imsg does not rewind. if a
> > caller consumed it, then the forwarded message will appear to be empty.
> >
> > for me, this fixes
> >
> >       vmd: control: vmop_result_read: Bad message
> >
> > which happens during IMSG_VMDOP_TERMINATE_VM_RESPONSE handling on vm
> > shutdown, when doing lots of concurrent vm starts/stops, in the
> > particular case of where a vm might have exited itself before vmd has
> > seen it.
> >
>
> This appears to be a consequence of switching vmd over to the new imsg
> API to consume the buffer instead of reading it directly like vmd did
> previously.
>
> > i don't think imsg_forward can be used here because proc_forward_imsg
> > can change the peer, which control.c uses, and it forwards the fd, which
> > imsg_forward drops.
>
> I have a counter proposal below that doesn't use ibuf_rewind(3) and
> ibuf_skip(3). I want to avoid touching ibufs directly. Instead lets just
> remove the usage of proc_forward_imsg() from the one case that causes
> this issue and instead compse a new imsg.

ok, this passed my torture test. vmd stays alive.

no rewind is still a footgun for future changes, but i guess that can
be addressed another time.

thanks!

>
> >
> > diff --git a/usr.sbin/vmd/proc.c b/usr.sbin/vmd/proc.c
> > index bcc85ac4630..9e123306624 100644
> > --- a/usr.sbin/vmd/proc.c
> > +++ b/usr.sbin/vmd/proc.c
> > @@ -632,6 +632,14 @@ proc_forward_imsg(struct privsep *ps, struct imsg *imsg, enum privsep_procid id,
> >       void            *data = NULL;
> >
> >       fd = imsg_get_fd(imsg);
> > +
> > +     /* rewind before forwarding, as imsg_forward(3) does: a getter
> > +      * may already have consumed this imsg.
> > +      */
> > +     ibuf_rewind(imsg->buf);
> > +     if (ibuf_skip(imsg->buf, sizeof(imsg->hdr)) == -1)
> > +             return (EINVAL);
> > +
> >       sz = imsg_get_len(imsg);
> >       type = imsg_get_type(imsg);
> >
>
> diff refs/heads/master refs/heads/vmd-rewind
> commit - 805f38d58fcea0569f8a0d41d85fdc0922c0fda0
> commit + 8cf7f3405b547f64d480ef357a2d9178954841e6
> blob - ee54ad31f8fc38111dc30efcea56cdfdea792bc6
> blob + 240d5b5dad79abb2a859fdba6a1ddb9a2331de5d
> --- usr.sbin/vmd/vmd.c
> +++ usr.sbin/vmd/vmd.c
> @@ -373,7 +373,8 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
>                 if (vmr.vmr_result) {
>                         DPRINTF("%s: forwarding TERMINATE VM for vm id %d",
>                             __func__, vmr.vmr_id);
> -                       proc_forward_imsg(ps, imsg, PROC_CONTROL, -1);
> +                       proc_compose_imsg(ps, PROC_CONTROL, type, peer_id, -1,
> +                           &vmr, sizeof(vmr));
>                 } else {
>                         if ((vm = vm_getbyvmid(vmr.vmr_id)) == NULL)
>                                 break;