On Thu, Oct 19, 2023 at 12:59:17PM +0200, Theo Buehler wrote:
> On Thu, Oct 19, 2023 at 10:41:07AM +0200, Claudio Jeker wrote:
> > More ibuf cleanup. rtr_proto.c still uses ibuf_add() where it could use
> > the new functions.
> >
> > Two bits I'm unsure about:
> > - I had to change some sizeof() to use native types (I especially dislike
> > the sizeof(struct rtr_header).
>
> Yes, that's not very nice.
>
> > - ibuf_add_nXX() can fail if the value is too large. Which should be
> > impossible but still maybe it would be better to check for errors.
>
> While it should be impossible, the length calculations are non-trivial,
> so it seems wiser to check.
>
> It's a bit longer than what you have now, but maybe it's an option
> to combine the length calculation with the errs += idiom.
>
> len += sizeof(rs->version);
> len += sizeof(type);
> len += sizeof(session_id);
> len += sizeof(len);
>
> if ((buf = ibuf_open(len)) == NULL)
> return NULL;
>
> errs += ibuf_add_n8(buf, rs->version);
> errs += ibuf_add_n8(buf, type);
> errs += ibuf_add_n16(buf, session_id);
> errs += ibuf_add_n32(buf, len);
>
> if (errs) {
> ibuf_free(ibuf);
> return NULL;
> }
>
> I'm ok with the diff as it is and you can ponder how you want to shave
> this particular Yak.
I like my yaks shaved like this...
The sizeof yak is still in queue... not sure about it.
--
:wq Claudio
Index: rtr_proto.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rtr_proto.c,v
retrieving revision 1.18
diff -u -p -r1.18 rtr_proto.c
--- rtr_proto.c 19 Oct 2023 11:12:10 -0000 1.18
+++ rtr_proto.c 19 Oct 2023 11:24:56 -0000
@@ -233,6 +233,7 @@ rtr_newmsg(struct rtr_session *rs, enum
uint16_t session_id)
{
struct ibuf *buf;
+ int saved_errno;
if (len > RTR_MAX_LEN) {
errno = ERANGE;
@@ -240,15 +241,23 @@ rtr_newmsg(struct rtr_session *rs, enum
}
len += sizeof(struct rtr_header);
if ((buf = ibuf_open(len)) == NULL)
- return NULL;
-
- /* cannot fail with fixed buffers */
- ibuf_add_n8(buf, rs->version);
- ibuf_add_n8(buf, type);
- ibuf_add_n16(buf, session_id);
- ibuf_add_n32(buf, len);
+ goto fail;
+ if (ibuf_add_n8(buf, rs->version) == -1)
+ goto fail;
+ if (ibuf_add_n8(buf, type) == -1)
+ goto fail;
+ if (ibuf_add_n16(buf, session_id) == -1)
+ goto fail;
+ if (ibuf_add_n32(buf, len) == -1)
+ goto fail;
return buf;
+
+ fail:
+ saved_errno = errno;
+ ibuf_free(buf);
+ errno = saved_errno;
+ return NULL;
}
/*
@@ -271,22 +280,27 @@ rtr_send_error(struct rtr_session *rs, e
buf = rtr_newmsg(rs, ERROR_REPORT, 2 * sizeof(uint32_t) + len + mlen,
err);
- if (buf == NULL) {
- log_warn("rtr %s: send error report", log_rtr(rs));
- return;
- }
-
- /* cannot fail with fixed buffers */
- ibuf_add_n32(buf, len);
- ibuf_add(buf, pdu, len);
- ibuf_add_n32(buf, mlen);
- ibuf_add(buf, msg, mlen);
+ if (buf == NULL)
+ goto fail;
+ if (ibuf_add_n32(buf, len) == -1)
+ goto fail;
+ if (ibuf_add(buf, pdu, len) == -1)
+ goto fail;
+ if (ibuf_add_n32(buf, mlen) == -1)
+ goto fail;
+ if (ibuf_add(buf, msg, mlen) == -1)
+ goto fail;
ibuf_close(&rs->w, buf);
log_warnx("rtr %s: sending error report[%u] %s", log_rtr(rs), err,
msg ? msg : "");
rtr_fsm(rs, RTR_EVNT_SEND_ERROR);
+ return;
+
+ fail:
+ log_warn("rtr %s: send error report", log_rtr(rs));
+ ibuf_free(buf);
}
static void
@@ -309,15 +323,17 @@ rtr_send_serial_query(struct rtr_session
struct ibuf *buf;
buf = rtr_newmsg(rs, SERIAL_QUERY, sizeof(uint32_t), rs->session_id);
- if (buf == NULL) {
- log_warn("rtr %s: send serial query", log_rtr(rs));
- rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL, 0);
- return;
- }
-
- /* cannot fail with fixed buffers */
- ibuf_add_n32(buf, rs->serial);
+ if (buf == NULL)
+ goto fail;
+ if (ibuf_add_n32(buf, rs->serial) == -1)
+ goto fail;
ibuf_close(&rs->w, buf);
+ return;
+
+ fail:
+ log_warn("rtr %s: send serial query", log_rtr(rs));
+ ibuf_free(buf);
+ rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL, 0);
}
/*
