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

On Thu, Jan 18, 2024 at 03:22:39PM +0100, Claudio Jeker wrote:
> On Thu, Jan 18, 2024 at 03:16:01PM +0100, Claudio Jeker wrote:
> > On Thu, Jan 18, 2024 at 03:07:35PM +0100, Theo Buehler wrote:
> > > 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?
> > 
> > No idea. At least I did not see this pattern often.
> > I think there was never a case where wbuf->fd had to be set by hand.
> > 
> > > > >  	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().
> > 
> > Right. New diff below.
> >  
> > > ... and of course the freezeros need to be done conditionally on user/pass
> > > != NULL
> > > 
> > 
> > Uhm. The man page tells me an other story:
> >      The freezero() function is similar to the free() function except it
> >      ensures memory is explicitly discarded.  If ptr is NULL, no action
> >      occurs.  If ptr is not NULL, the size argument must be equal to or
> >      smaller than the size of the earlier allocation that returned ptr.
> 
> On the other hand the strlen(user) used as argument to freezero will
> explode. Also shouldn't that be strlen(user) + 1?

Doesn't really matter, but it is more correct with the +1.

ok tb