From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: fixup shutdown reason handling in notification
To: tech@openbsd.org
Date: Fri, 22 Mar 2024 20:39:50 +1000

On Fri, Mar 22, 2024 at 11:23:34AM +0100, Claudio Jeker wrote:
> Just after commit I realized that the previous change to
> log_notification() was not quite right since the code is called both for
> received and sent notifications but the reason always points at the
> received one.

Oh. Right.

> 
> It is better to peak at the ibuf passed to log_notification() so both
> cases work reliably. For this to work I moved the ibuf_get_string()
> function from rtr_proto.c to util.c and moved the log_notification() call
> back up in parse_notification() since the code there to process the reason
> consumes the data from the ibuf.
> 
> Tested both with sent and received shutdown reasons.

ok tb one tiny nit inline

> -- 
> :wq Claudio
> 
> Index: bgpd.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> diff -u -p -r1.488 bgpd.h
> --- bgpd.h	22 Mar 2024 07:19:28 -0000	1.488
> +++ bgpd.h	22 Mar 2024 09:38:12 -0000
> @@ -1545,6 +1545,7 @@ int	trie_equal(struct trie_head *, struc
>  time_t			 getmonotime(void);
>  
>  /* util.c */
> +char		*ibuf_get_string(struct ibuf *, size_t);
>  const char	*log_addr(const struct bgpd_addr *);
>  const char	*log_in6addr(const struct in6_addr *);
>  const char	*log_sockaddr(struct sockaddr *, socklen_t);
> Index: logmsg.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/logmsg.c,v
> diff -u -p -r1.12 logmsg.c
> --- logmsg.c	22 Mar 2024 07:19:28 -0000	1.12
> +++ logmsg.c	22 Mar 2024 09:41:04 -0000
> @@ -189,13 +189,19 @@ log_notification(const struct peer *peer
>  
>  		if (subcode == ERR_CEASE_ADMIN_DOWN ||
>  		    subcode == ERR_CEASE_ADMIN_RESET) {
> -			if (peer->stats.last_reason[0] != '\0') {
> -				logit(LOG_ERR, "%s: %s notification: %s, %s: "
> -				    "reason \"%s\"", p, dir,
> -				    errnames[errcode], suberrname,
> -				    log_reason(peer->stats.last_reason));
> -				free(p);
> -				return;
> +			uint8_t len;
> +			/* check if shutdown reason is included */
> +			if (ibuf_get_n8(&ibuf, &len) != -1 && len != 0) {
> +				char *s;
> +				if ((s = ibuf_get_string(&ibuf, len)) != NULL) {
> +					logit(LOG_ERR, "%s: %s notification: "
> +					"%s, %s: reason \"%s\"", p, dir,

The previous line needs 4 additional spaces indentation

> +					    errnames[errcode], suberrname,
> +					    log_reason(s));
> +					free(s);
> +					free(p);
> +					return;
> +				}
>  			}
>  		}
>  		break;
> Index: rtr_proto.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rtr_proto.c,v
> diff -u -p -r1.33 rtr_proto.c
> --- rtr_proto.c	23 Jan 2024 15:59:56 -0000	1.33
> +++ rtr_proto.c	22 Mar 2024 09:37:42 -0000
> @@ -915,22 +915,6 @@ rtr_parse_cache_reset(struct rtr_session
>  	return -1;
>  }
>  
> -static char *
> -ibuf_get_string(struct ibuf *buf, size_t len)
> -{
> -	char *str;
> -
> -	if (ibuf_size(buf) < len) {
> -		errno = EBADMSG;
> -		return (NULL);
> -	}
> -	str = strndup(ibuf_data(buf), len);
> -	if (str == NULL)
> -		return (NULL);
> -	ibuf_skip(buf, len);
> -	return (str);
> -}
> -
>  /*
>   * Parse an Error Response message. This function behaves a bit different
>   * from other parse functions since on error the connection needs to be
> Index: session.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> diff -u -p -r1.465 session.c
> --- session.c	22 Mar 2024 07:19:28 -0000	1.465
> +++ session.c	22 Mar 2024 09:40:23 -0000
> @@ -2505,6 +2505,8 @@ parse_notification(struct peer *peer)
>  	peer->stats.last_rcvd_errcode = errcode;
>  	peer->stats.last_rcvd_suberr = subcode;
>  
> +	log_notification(peer, errcode, subcode, &ibuf, "received");
> +
>  	CTASSERT(sizeof(peer->stats.last_reason) > UINT8_MAX);
>  	memset(peer->stats.last_reason, 0, sizeof(peer->stats.last_reason));
>  	if (errcode == ERR_CEASE &&
> @@ -2518,8 +2520,6 @@ parse_notification(struct peer *peer)
>  				    "received truncated shutdown reason");
>  		}
>  	}
> -
> -	log_notification(peer, errcode, subcode, &ibuf, "received");
>  
>  	if (errcode == ERR_OPEN && subcode == ERR_OPEN_OPT) {
>  		session_capa_ann_none(peer);
> Index: util.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
> diff -u -p -r1.84 util.c
> --- util.c	22 Mar 2024 07:19:28 -0000	1.84
> +++ util.c	22 Mar 2024 09:37:49 -0000
> @@ -32,6 +32,22 @@
>  #include "rde.h"
>  #include "log.h"
>  
> +char *
> +ibuf_get_string(struct ibuf *buf, size_t len)
> +{
> +	char *str;
> +
> +	if (ibuf_size(buf) < len) {
> +		errno = EBADMSG;
> +		return (NULL);
> +	}
> +	str = strndup(ibuf_data(buf), len);
> +	if (str == NULL)
> +		return (NULL);
> +	ibuf_skip(buf, len);
> +	return (str);
> +}
> +
>  const char *
>  log_addr(const struct bgpd_addr *addr)
>  {
>