Download raw body.
xlock: convert to new imsg API
On Thu, Jan 18, 2024 at 03:22:39PM +0100, Claudio Jeker wrote:
> >
> > > ... 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.
Hi,
You've not removed the NULL checks on freezero(). But either way,
ok matthieu@
> --
> :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