From: Claudio Jeker Subject: Re: bgpd: rde attribute cleanup and fixes To: Theo Buehler Cc: tech@openbsd.org Date: Mon, 14 Apr 2025 13:18:48 +0200 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