Re: iked: ibuf saga step 2
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 - 1.213
> > +++ iked.h 26 May 2023 07:21:43 -
> > @@ -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 - 1.92
> > +++ ikev2_msg.c 26 May 2023 07:20:13 -
> > @@ -290,10 +290,17 @@ ikev2_msg_send(struct iked *env, struct
> > ibuf_length(buf), isnatt ? ", NAT-T" : "");
> >
> > if (isnatt) {
> > - if (ibuf_prepend(buf, , sizeof(natt)) == -1) {
> > + struct ibuf *new;
> > + if ((new = ibuf_new(, 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 - 1.16
> > +++ imsg_util.c 26 May 2023 07:21:26 -
> > @@ -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
Re: iked: ibuf saga step 2
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.h23 May 2023 13:57:14 - 1.213
> +++ iked.h26 May 2023 07:21:43 -
> @@ -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 - 1.92
> +++ ikev2_msg.c 26 May 2023 07:20:13 -
> @@ -290,10 +290,17 @@ ikev2_msg_send(struct iked *env, struct
> ibuf_length(buf), isnatt ? ", NAT-T" : "");
>
> if (isnatt) {
> - if (ibuf_prepend(buf, , sizeof(natt)) == -1) {
> + struct ibuf *new;
> + if ((new = ibuf_new(, 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 - 1.16
> +++ imsg_util.c 26 May 2023 07:21:26 -
> @@ -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;
>
iked: ibuf saga step 2
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).
--
: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 - 1.213
+++ iked.h 26 May 2023 07:21:43 -
@@ -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 - 1.92
+++ ikev2_msg.c 26 May 2023 07:20:13 -
@@ -290,10 +290,17 @@ ikev2_msg_send(struct iked *env, struct
ibuf_length(buf), isnatt ? ", NAT-T" : "");
if (isnatt) {
- if (ibuf_prepend(buf, , sizeof(natt)) == -1) {
+ struct ibuf *new;
+ if ((new = ibuf_new(, sizeof(natt))) == NULL) {
log_debug("%s: failed to set NAT-T", __func__);
return (-1);
}
+ if (ibuf_cat(new, buf) == -1) {
+ 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 - 1.16
+++ imsg_util.c 26 May 2023 07:21:26 -
@@ -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;
