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.

>
> 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;