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");
 		}
 	}