On Thu, Jul 20, 2023 at 05:22:25PM +0200, Theo Buehler wrote:
> On Thu, Jul 20, 2023 at 05:06:00PM +0200, Claudio Jeker wrote:
> > I think it is better to use a safe ideom when matching against a peer name
> > instead of forcefully NUL terminate the string somewhere unrelated.
> > By default all these string buffers use the same size so strncmp() will
> > not clip since the peer description is enforced by bgpd to be smaller.
> >
> > Another option would be to move
> > neighbor->descr[PEER_DESCR_LEN - 1] = 0;
> > into the match functions. At least then it is certainly done.
>
> I prefer strncpy(). So this diff is ok.
Ugh. strncmp...
>
> However this makes me wonder: in your earlier diff today you adjusted
> the strlcpy with source neighbor->reason. If we can't be sure that
> neighbor->descr is NUL terminated, why can we assume this to be the
> case for neighbor->reason? (strlcpy walks the source string until it
> hits NUL).
>
> >
> > --
> > :wq Claudio
> >
> > Index: control.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/control.c,v
> > retrieving revision 1.111
> > diff -u -p -r1.111 control.c
> > --- control.c 20 Jul 2023 11:10:03 -0000 1.111
> > +++ control.c 20 Jul 2023 14:04:33 -0000
> > @@ -314,7 +314,6 @@ control_dispatch_msg(struct pollfd *pfd,
> > if (imsg.hdr.len == IMSG_HEADER_SIZE +
> > sizeof(struct ctl_neighbor)) {
> > neighbor = imsg.data;
> > - neighbor->descr[PEER_DESCR_LEN - 1] = 0;
> > } else {
> > neighbor = NULL;
> > }
> > @@ -370,7 +369,6 @@ control_dispatch_msg(struct pollfd *pfd,
> > }
> >
> > neighbor = imsg.data;
> > - neighbor->descr[PEER_DESCR_LEN - 1] = 0;
> >
> > matched = 0;
> > RB_FOREACH(p, peer_head, peers) {
> > @@ -474,7 +472,6 @@ control_dispatch_msg(struct pollfd *pfd,
> >
> > ribreq = imsg.data;
> > neighbor = &ribreq->neighbor;
> > - neighbor->descr[PEER_DESCR_LEN - 1] = 0;
> >
> > /* check if at least one neighbor exists */
> > RB_FOREACH(p, peer_head, peers)
> > Index: rde.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> > retrieving revision 1.608
> > diff -u -p -r1.608 rde.c
> > --- rde.c 12 Jul 2023 14:45:42 -0000 1.608
> > +++ rde.c 20 Jul 2023 14:01:47 -0000
> > @@ -2947,7 +2947,8 @@ rde_match_peer(struct rde_peer *p, struc
> > return 0;
> > } else if (n && n->descr[0]) {
> > s = n->is_group ? p->conf.group : p->conf.descr;
> > - if (strcmp(s, n->descr))
> > + /* cannot trust n->descr to be properly terminated */
> > + if (strncmp(s, n->descr, sizeof(n->descr)))
> > return 0;
> > }
> > return 1;
> > Index: session.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> > retrieving revision 1.446
> > diff -u -p -r1.446 session.c
> > --- session.c 12 Jul 2023 14:45:43 -0000 1.446
> > +++ session.c 20 Jul 2023 14:01:25 -0000
> > @@ -3461,7 +3461,8 @@ peer_matched(struct peer *p, struct ctl_
> > return 0;
> > } else if (n && n->descr[0]) {
> > s = n->is_group ? p->conf.group : p->conf.descr;
> > - if (strcmp(s, n->descr))
> > + /* cannot trust n->descr to be properly terminated */
> > + if (strncmp(s, n->descr, sizeof(n->descr)))
> > return 0;
> > }
> > return 1;
> >
>
