On Fri, May 26, 2023 at 10:11:32AM +0200, Claudio Jeker wrote:
> Kill ibuf_prepend() it is used only once and the function does unholy
> things to the ibuf passed in. Just do the obivous dance in the callee.
> The only thing to be careful about is the fact that all pointers of buf
> are replaced (msg->msg_data).
>
> Tested with iked -t (which should use this codepath).
What can I say. Unholy is a nice way of putting it. This is rather
horrible. I agree, inlining it makes sense.
It is a bit scary to do ibuf_free(buf); (which ibuf_prepend() is careful
not to do) since that is passed in by the caller. In turn, pointers are
no longer transferred via memcpy, which is a good thing in my book...
Arguably, ownership of the caller's buf is transferred to resp.msg_data
in all cases, so that is fine except that it is still a bit of a trap.
If this was my code I would probably insist on invalidating the buf in
the caller after hanging it onto resp.
ok tb
But please fix the leak indicated below.
> --
> :wq Claudio
>
> Index: iked.h
> ===================================================================
> RCS file: /cvs/src/sbin/iked/iked.h,v
> retrieving revision 1.213
> diff -u -p -r1.213 iked.h
> --- iked.h 23 May 2023 13:57:14 -0000 1.213
> +++ iked.h 26 May 2023 07:21:43 -0000
> @@ -1279,7 +1279,6 @@ struct ibuf *
> ibuf_dup(struct ibuf *);
> struct ibuf *
> ibuf_random(size_t);
> -int ibuf_prepend(struct ibuf *, void *, size_t);
> int ibuf_strcat(struct ibuf **, const char *);
> int ibuf_strlen(struct ibuf *);
>
> Index: ikev2_msg.c
> ===================================================================
> RCS file: /cvs/src/sbin/iked/ikev2_msg.c,v
> retrieving revision 1.92
> diff -u -p -r1.92 ikev2_msg.c
> --- ikev2_msg.c 23 May 2023 13:57:14 -0000 1.92
> +++ ikev2_msg.c 26 May 2023 07:20:13 -0000
> @@ -290,10 +290,17 @@ ikev2_msg_send(struct iked *env, struct
> ibuf_length(buf), isnatt ? ", NAT-T" : "");
>
> if (isnatt) {
> - if (ibuf_prepend(buf, &natt, sizeof(natt)) == -1) {
> + struct ibuf *new;
> + if ((new = ibuf_new(&natt, sizeof(natt))) == NULL) {
> log_debug("%s: failed to set NAT-T", __func__);
> return (-1);
> }
> + if (ibuf_cat(new, buf) == -1) {
This needs an
ibuf_free(new);
> + log_debug("%s: failed to set NAT-T", __func__);
> + return (-1);
> + }
> + ibuf_free(buf);
> + buf = msg->msg_data = new;
> }
>
> if (sendtofrom(msg->msg_fd, ibuf_data(buf), ibuf_size(buf), 0,
> Index: imsg_util.c
> ===================================================================
> RCS file: /cvs/src/sbin/iked/imsg_util.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 imsg_util.c
> --- imsg_util.c 23 May 2023 13:57:14 -0000 1.16
> +++ imsg_util.c 26 May 2023 07:21:26 -0000
> @@ -146,25 +146,6 @@ ibuf_setsize(struct ibuf *buf, size_t le
> }
>
> int
> -ibuf_prepend(struct ibuf *buf, void *data, size_t len)
> -{
> - struct ibuf *new;
> -
> - /* Swap buffers (we could also use memmove here) */
> - if ((new = ibuf_new(data, len)) == NULL)
> - return (-1);
> - if (ibuf_cat(new, buf) == -1) {
> - ibuf_free(new);
> - return (-1);
> - }
> - free(buf->buf);
> - memcpy(buf, new, sizeof(*buf));
> - free(new);
> -
> - return (0);
> -}
> -
> -int
> ibuf_strcat(struct ibuf **buf, const char *s)
> {
> size_t slen;
>
