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.
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;
>
