Download raw body.
xlock: convert to new imsg API
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?
New version below.
--
: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:21:08 -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,64 @@ 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) {
+ 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;
- } else
- *style = NULL;
+ if ((*style = ibuf_get_string(&buf, hdr.stylelen)) == NULL) {
+ warn("truncated message");
+ goto fail;
+ }
+ }
imsg_free(&imsg);
return 0;
-nomem:
- warn("strndup");
+fail:
imsg_free(&imsg);
return -1;
}
@@ -202,21 +202,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;
}
@@ -267,13 +267,16 @@ priv_init(gid_t gid)
err(1, "pledge");
while (1) {
+ user = pass = style = NULL;
if (receive_cmd(&child_ibuf, &user, &pass, &style) == -1) {
warn("receive_cmd");
result = 0;
} else
result = pw_check(user, pass, style);
- freezero(user, strlen(user));
- freezero(pass, strlen(pass));
+ if (user != NULL)
+ freezero(user, strlen(user) + 1);
+ if (pass != NULL)
+ freezero(pass, strlen(pass) + 1);
free(style);
if (send_result(&child_ibuf, result) == -1)
warn("send_result");
xlock: convert to new imsg API