On Mon, Apr 14, 2025 at 11:16:24AM +0200, Theo Buehler wrote:
> On Mon, Apr 14, 2025 at 10:49:11AM +0200, Claudio Jeker wrote:
> > Noticed while working on something similar.
> > attr_optadd() first inserts the attribute in the global table and only
> > later checks if an attribute is already present for this aspath. Since
> > that check does not depend an 'a' we can swap the checks. This solves
> > the possible memleak between attr_alloc() and this return (-1) in a
> > much nicer way.
> > 
> > The 2nd thing is a UB fix in attr_diff(). If oa->len == 0 then oa->data ==
> > NULL and calling memcmp() with a NULL pointer summons the UB dragon so
> > better return early.  Right now there is no BGP attribute with no data
> > specified so this should not matter.
> 
> ok tb
> 
> For consistency, I'd add this on top to align the only caller of
> attr_optadd() that doesn't explicitly compare against -1 with the rest:
> 
> Index: rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> diff -u -p -r1.654 rde.c
> --- rde.c	27 Feb 2025 14:03:32 -0000	1.654
> +++ rde.c	14 Apr 2025 09:13:05 -0000
> @@ -2692,7 +2692,7 @@ rde_as4byte_fixup(struct rde_peer *peer,
>  			/* switch over to new AGGREGATOR */
>  			attr_free(a, oaggr);
>  			if (attr_optadd(a, ATTR_OPTIONAL | ATTR_TRANSITIVE,
> -			    ATTR_AGGREGATOR, naggr->data, naggr->len))
> +			    ATTR_AGGREGATOR, naggr->data, naggr->len) == -1)
>  				fatalx("attr_optadd failed but impossible");
>  		}
>  	}
> 

You can commit that one, OK claudio@

-- 
:wq Claudio