On Fri, May 26, 2023 at 11:18:26AM +0200, Theo Buehler wrote:
> 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...
This ibuf_prepend() pattern is scary. Now if we have more places where
something like this is used then it may make sense to add an
ibuf_prepend() to the real ibuf API but right now I only found this one
use case.
> 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.
The livecycle of these bufs is a bit of a mysterybox. Now the
msg->msg_data buf is created in ikev2_msg_init() and should not have other
pointers lingering around. So I agree with your assesment and that this is
still a bit of a trap.
This codebase is using ibufs in very creative ways and it is not always
clear to me why it is done that way.
> ok tb
>
> But please fix the leak indicated below.
Fixed, thanks for spotting.
> > --
> > :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;
> >
>
--
:wq Claudio
