From: Sebastian Benoit <benno@openbsd.org>
Subject: Re: bgpd: add missing error check for IMSG_RECONF_ROA_ITEM
To: tech@openbsd.org
Date: Wed, 12 Nov 2025 17:46:32 +0100

Claudio Jeker(cjeker@diehard.n-r-g.com) on 2025-11-12:
> On Wed, Nov 12, 2025 at 05:33:20PM +0100, Claudio Jeker wrote:
> > On Wed, Nov 12, 2025 at 05:29:36PM +0100, Sebastian Benoit wrote:
> > > Claudio Jeker(cjeker@diehard.n-r-g.com) on 2025-11-12:
> > > > Somehow the check for trie_roa_add failure was lost.
> > > > We do the same just log the fact handling in IMSG_RECONF_PREFIX_SET_ITEM.
> > > > This should only trigger in out of memory situations.
> > > > 
> > > > Fixes CID 492365
> > > > -- 
> > > > :wq Claudio
> > > > 
> > > > Index: rde.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> > > > diff -u -p -r1.662 rde.c
> > > > --- rde.c	12 Nov 2025 15:17:43 -0000	1.662
> > > > +++ rde.c	12 Nov 2025 15:59:28 -0000
> > > > @@ -1144,6 +1144,9 @@ rde_dispatch_imsg_parent(struct imsgbuf 
> > > >  			if (imsg_get_data(&imsg, &roa, sizeof(roa)) == -1)
> > > >  				fatalx("IMSG_RECONF_ROA_ITEM bad len");
> > > >  			rv = trie_roa_add(&last_prefixset->th, &roa);
> > > > +			if (rv == -1)
> > > > +				log_warnx("trie_roa_add %s failed",
> > > > +				    log_roa(&roa));
> > > >  			break;
> > > >  		case IMSG_RECONF_PREFIX_SET_ITEM:
> > > >  			if (imsg_get_data(&imsg, &psi, sizeof(psi)) == -1)
> > > > 
> > > 
> > > ok benno@
> > > 
> > > maybe related: i notice that in rde_dispatch_imsg_rtr() under
> > > IMSG_RECONF_ROA_ITEM there is a similar pattern where you dont use
> > > log_roa() but handroll the printing of a prefix. 
> > 
> > Guess I should switch that over to log_roa as well.
> 
> I think this is better. Kill the rv complication, always check using != 0
> and use log_roa in both cases.

reads ok

> -- 
> :wq Claudio
> 
> Index: rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> diff -u -p -r1.662 rde.c
> --- rde.c	12 Nov 2025 15:17:43 -0000	1.662
> +++ rde.c	12 Nov 2025 16:36:21 -0000
> @@ -867,7 +867,7 @@ rde_dispatch_imsg_parent(struct imsgbuf 
>  	struct rde_prefixset	*ps;
>  	struct rde_aspath	*asp;
>  	size_t			 nmemb;
> -	int			 n, fd, rv;
> +	int			 n, fd;
>  	uint16_t		 rid;
>  
>  	while (imsgbuf) {
> @@ -1143,20 +1143,23 @@ rde_dispatch_imsg_parent(struct imsgbuf 
>  		case IMSG_RECONF_ROA_ITEM:
>  			if (imsg_get_data(&imsg, &roa, sizeof(roa)) == -1)
>  				fatalx("IMSG_RECONF_ROA_ITEM bad len");
> -			rv = trie_roa_add(&last_prefixset->th, &roa);
> +			if (trie_roa_add(&last_prefixset->th, &roa) != 0) {
> +				log_warnx("trie_roa_add %s failed",
> +				    log_roa(&roa));
> +			}
>  			break;
>  		case IMSG_RECONF_PREFIX_SET_ITEM:
>  			if (imsg_get_data(&imsg, &psi, sizeof(psi)) == -1)
>  				fatalx("IMSG_RECONF_PREFIX_SET_ITEM bad len");
>  			if (last_prefixset == NULL)
>  				fatalx("King Bula has no prefixset");
> -			rv = trie_add(&last_prefixset->th,
> +			if (trie_add(&last_prefixset->th,
>  			    &psi.p.addr, psi.p.len,
> -			    psi.p.len_min, psi.p.len_max);
> -			if (rv == -1)
> +			    psi.p.len_min, psi.p.len_max) != 0) {
>  				log_warnx("trie_add(%s) %s/%u failed",
>  				    last_prefixset->name, log_addr(&psi.p.addr),
>  				    psi.p.len);
> +			}
>  			break;
>  		case IMSG_RECONF_AS_SET:
>  			if (imsg_get_ibuf(&imsg, &ibuf) == -1 ||
> @@ -1289,12 +1292,8 @@ rde_dispatch_imsg_rtr(struct imsgbuf *im
>  			if (imsg_get_data(&imsg, &roa, sizeof(roa)) == -1)
>  				fatalx("IMSG_RECONF_ROA_ITEM bad len");
>  			if (trie_roa_add(&roa_new.th, &roa) != 0) {
> -				struct bgpd_addr p = {
> -					.aid = roa.aid,
> -					.v6 = roa.prefix.inet6
> -				};
> -				log_warnx("trie_roa_add %s/%u failed",
> -				    log_addr(&p), roa.prefixlen);
> +				log_warnx("trie_roa_add %s failed",
> +				    log_roa(&roa));
>  			}
>  			break;
>  		case IMSG_RECONF_ASPA_PREP:
>