From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: bgpd: rde attribute cleanup and fixes
To: tech@openbsd.org
Date: Mon, 14 Apr 2025 10:49:11 +0200

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.
-- 
:wq Claudio

Index: rde_attr.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_attr.c,v
diff -u -p -r1.135 rde_attr.c
--- rde_attr.c	10 Sep 2024 09:38:45 -0000	1.135
+++ rde_attr.c	10 Apr 2025 09:29:00 -0000
@@ -80,21 +80,20 @@ attr_optadd(struct rde_aspath *asp, uint
 	struct attr	*a, *t;
 	void		*p;
 
-	/* known optional attributes were validated previously */
-	if ((a = attr_lookup(flags, type, data, len)) == NULL)
-		a = attr_alloc(flags, type, data, len);
-
 	/* attribute allowed only once */
 	for (l = 0; l < asp->others_len; l++) {
 		if (asp->others[l] == NULL)
 			break;
-		if (type == asp->others[l]->type) {
-			if (a->refcnt == 0)
-				attr_put(a);
+		if (type == asp->others[l]->type)
 			return (-1);
-		}
+		if (type < asp->others[l]->type)
+			break;
 	}
 
+	/* known optional attributes were validated previously */
+	if ((a = attr_lookup(flags, type, data, len)) == NULL)
+		a = attr_alloc(flags, type, data, len);
+
 	/* add attribute to the table but first bump refcnt */
 	a->refcnt++;
 	rdemem.attr_refs++;
@@ -114,7 +113,7 @@ attr_optadd(struct rde_aspath *asp, uint
 
 	/* no empty slot found, need to realloc */
 	if (asp->others_len == UCHAR_MAX)
-		fatalx("attr_optadd: others_len overflow");
+		fatalx("attr_optadd: attribute overflow");
 
 	asp->others_len++;
 	if ((p = reallocarray(asp->others,
@@ -190,6 +189,8 @@ attr_diff(struct attr *oa, struct attr *
 		return (1);
 	if (oa->len < ob->len)
 		return (-1);
+	if (oa->len == 0)
+		return (0);
 	r = memcmp(oa->data, ob->data, oa->len);
 	if (r > 0)
 		return (1);