From: Theo Buehler <tb@theobuehler.org>
Subject: Re: xlock: convert to new imsg API
To: tech@openbsd.org
Date: Thu, 18 Jan 2024 15:07:35 +0100

On Thu, Jan 18, 2024 at 03:04:37PM +0100, Theo Buehler wrote:
> On Thu, Jan 18, 2024 at 02:33:25PM +0100, Claudio Jeker wrote:
> > This diff converts the privsep code in xlock to use the new imsg API.
> > Overall I think the resulting code is easier to understand which is
> > important.
> > 
> > Use a do { } while (n == 0); loop to read until a full message was
> > received. I'm not sure if this pathological case can be triggered but
> > better to fix this.
> 
> ok, but please see below
> 
> > -- 
> > :wq Claudio
> > 
> > Index: app/xlockmore/xlock/privsep.c
> > ===================================================================
> > RCS file: /cvs/xenocara/app/xlockmore/xlock/privsep.c,v
> > diff -u -p -r1.3 privsep.c
> > --- app/xlockmore/xlock/privsep.c	14 Dec 2023 09:44:15 -0000	1.3
> > +++ app/xlockmore/xlock/privsep.c	5 Jan 2024 16:56:28 -0000
> > @@ -118,7 +118,6 @@ send_cmd(struct imsgbuf *ibuf, char *use
> >  			warn("imsg_add");
> >  			return -1;
> >  		}
> > -	wbuf->fd = -1;
> >  	imsg_close(ibuf, wbuf);
> 
> Out of curiosity, do you know where this idiom of setting the fd to -1
> before imsg_close() from? Did people malloc ibufs before this was
> centralized in libutil?
> 
> >  
> >  	if ((n = msgbuf_write(&ibuf->w)) == -1 && errno != EAGAIN) {
> > @@ -130,63 +129,65 @@ send_cmd(struct imsgbuf *ibuf, char *use
> >  	return 0;
> >  }
> >  
> > +static char *
> > +ibuf_get_string(struct ibuf *buf, size_t len)
> > +{
> > +	char *str;
> > +
> > +	if (ibuf_size(buf) < len) {
> > +		errno = EBADMSG;
> > +		return (NULL);
> > +	}
> > +	str = strndup(ibuf_data(buf), len);
> > +	if (str == NULL)
> > +		return (NULL);
> > +	buf->rpos += len;
> > +	return (str);
> > +}
> > +
> >  static int
> >  receive_cmd(struct imsgbuf *ibuf, char **name, char **pass, char **style)
> >  {
> >  	struct imsg imsg;
> > +	struct ibuf buf;
> >  	struct priv_cmd_hdr hdr;
> > -	ssize_t n, nread, datalen;
> > -	void *data;
> > +	ssize_t n, nread;
> 
> Not your bug, but please initialize *name, *pass, and *style to NULL,
> otherwise there will be bad frees in priv_init() if you goto fail before
> the last ibuf_get_string().

... and of course the freezeros need to be done conditionally on user/pass
!= NULL

> 
> >  
> > -	if ((nread = imsg_read(ibuf)) == -1 && errno != EAGAIN) {
> > -		warn("imsg_read");
> > -		return -1;
> > -	}
> > -	if (nread == 0) {
> > -		/* parent exited */
> > -		exit(0);
> > -	}
> > -	if ((n = imsg_get(ibuf, &imsg)) == -1) {
> > -		warnx("imsg_get");
> > -		return -1;
> > -	}
> > -	if (imsg.hdr.type != XLOCK_CHECKPW_CMD) {
> > +	do {
> > +		if ((nread = imsg_read(ibuf)) == -1 && errno != EAGAIN) {
> > +			warn("imsg_read");
> > +			return -1;
> > +		}
> > +		if (nread == 0) {
> > +			/* parent exited */
> > +			exit(0);
> > +		}
> > +		if ((n = imsg_get(ibuf, &imsg)) == -1) {
> > +			warnx("imsg_get");
> > +			return -1;
> > +		}
> > +	} while (n == 0);
> > +	if (imsg_get_type(&imsg) != XLOCK_CHECKPW_CMD) {
> >  		warnx("invalid command");
> > -		imsg_free(&imsg);
> > -		return -1;
> > -	}
> > -	datalen = imsg.hdr.len - IMSG_HEADER_SIZE;
> > -	data = imsg.data;
> > -	if (datalen < sizeof(struct priv_cmd_hdr)) {
> > -		warnx("truncated header");
> > -		imsg_free(&imsg);
> > -		return -1;
> > +		goto fail;
> >  	}
> > -	memcpy(&hdr, data, sizeof(struct priv_cmd_hdr));
> > -	if (datalen != sizeof(hdr) + hdr.namelen + hdr.passlen + hdr.stylelen) {
> > -		warnx("truncated strings");
> > -		imsg_free(&imsg);
> > -		return -1;
> > +	if (imsg_get_ibuf(&imsg, &buf) == -1 ||
> > +	    ibuf_get(&buf, &hdr, sizeof(hdr)) == -1 ||
> > +	    (*name = ibuf_get_string(&buf, hdr.namelen)) == NULL ||
> > +	    (*pass = ibuf_get_string(&buf, hdr.passlen)) == NULL) {
> > +		warn("truncated message");
> > +		goto fail;
> >  	}
> > -	data += sizeof(struct priv_cmd_hdr);
> > -	*name = strndup(data, hdr.namelen);
> > -	if (*name == NULL)
> > -		goto nomem;
> > -	data += hdr.namelen;
> > -	*pass = strndup(data, hdr.passlen);
> > -	if (*pass == NULL)
> > -		goto  nomem;
> > -	data += hdr.passlen;
> >  	if (hdr.stylelen != 0) {
> > -		*style = strndup(data, hdr.stylelen);
> > -		if (*style == NULL)
> > -			goto nomem;
> > +		if ((*style = ibuf_get_string(&buf, hdr.stylelen)) == NULL) {
> > +			warn("truncated message");
> > +			goto fail;
> > +		}
> >  	} else
> >  		*style = NULL;
> >  	imsg_free(&imsg);
> >  	return 0;
> > -nomem:
> > -	warn("strndup");
> > +fail:
> >  	imsg_free(&imsg);
> >  	return -1;
> >  }
> > @@ -202,21 +203,21 @@ send_result(struct imsgbuf *ibuf, int re
> >  static int
> >  receive_result(struct imsgbuf *ibuf, int *presult)
> >  {
> > -	ssize_t nread, n;
> > +	ssize_t n, nread;
> >  	int retval = 0;
> >  	struct imsg imsg;
> >  
> > -	if ((nread = imsg_read(ibuf)) == -1 && errno != EAGAIN)
> > -		return -1;
> > -	if (nread == 0)
> > -		return -1;
> > -	if ((n = imsg_get(ibuf, &imsg)) == -1)
> > -		return -1;
> > -	if (imsg.hdr.len - IMSG_HEADER_SIZE != sizeof(int))
> > -		retval = -1;
> > -	if (imsg.hdr.type != XLOCK_CHECKPW_RESULT)
> > +	do {
> > +		if ((nread = imsg_read(ibuf)) == -1 && errno != EAGAIN)
> > +			return -1;
> > +		if (nread == 0)
> > +			return -1;
> > +		if ((n = imsg_get(ibuf, &imsg)) == -1)
> > +			return -1;
> > +	} while (n == 0);
> > +	if (imsg_get_type(&imsg) != XLOCK_CHECKPW_RESULT ||
> > +	    imsg_get_data(&imsg, presult, sizeof(*presult)) == -1)
> >  		retval = -1;
> > -	memcpy(presult, imsg.data, sizeof(int));
> >  	imsg_free(&imsg);
> >  	return retval;
> >  }
> > 
>