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.

-- 
:wq Claudio

Index: xlock/privsep.c
===================================================================
RCS file: /cvs/xenocara/app/xlockmore/xlock/privsep.c,v
diff -u -p -r1.3 privsep.c
--- xlock/privsep.c	14 Dec 2023 09:44:15 -0000	1.3
+++ xlock/privsep.c	18 Jan 2024 14:15:03 -0000
@@ -118,7 +118,6 @@ send_cmd(struct imsgbuf *ibuf, char *use
 			warn("imsg_add");
 			return -1;
 		}
-	wbuf->fd = -1;
 	imsg_close(ibuf, wbuf);
 
 	if ((n = msgbuf_write(&ibuf->w)) == -1 && errno != EAGAIN) {
@@ -130,63 +129,69 @@ 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;
 
-	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) {
+	*name = NULL;
+	*pass = NULL;
+	*style = NULL;
+
+	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 +207,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;
 }