From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: bgpd: IMSG_CTL_SHOW_NEIGHBOR handling
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Wed, 12 Feb 2025 14:01:27 +0100

On Wed, Feb 12, 2025 at 01:47:39PM +0100, Theo Buehler wrote:
> On Wed, Feb 12, 2025 at 01:32:25PM +0100, Claudio Jeker wrote:
> > As requested by tb@ move the getpeerbyid() call from control_imsg_relay()
> > back to the session imsg dispatcher. So all of them are in the same place.
> > 
> > I left the p == NULL check just in case but I'm torn about that.
> 
> I think it makes sense to keep it, seeing as all other types accept
> NULL. Better one check too many than one too few.
> 
> ok tb
> 
> It doesn't really matter since no caller checks the returned value, but I
> think control_imsg_relay() should either return (-1) on error or the two
> "return imsg_*" should translate -1 to 0 like it's done for imsg_get_data().

Yeah, lets fix this proper. How about the below?
Not much we can do on failure apart from logging the fact.

-- 
:wq Claudio

Index: control.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/control.c,v
diff -u -p -r1.132 control.c
--- control.c	2 Dec 2024 15:03:17 -0000	1.132
+++ control.c	12 Feb 2025 12:57:57 -0000
@@ -544,6 +544,7 @@ control_imsg_relay(struct imsg *imsg, st
 	type = imsg_get_type(imsg);
 	pid = imsg_get_pid(imsg);
 
+	/* not an error if the connection got closed */
 	if ((c = control_connbypid(pid)) == NULL)
 		return (0);
 
@@ -553,14 +554,12 @@ control_imsg_relay(struct imsg *imsg, st
 		struct peer peer;
 
 		if (p == NULL) {
-			log_warnx("%s: no such peer: id=%u", __func__,
-			    imsg_get_id(imsg));
-			return (0);
-		}
-		if (imsg_get_data(imsg, &stats, sizeof(stats)) == -1) {
-			log_warnx("%s: imsg_get_data", __func__);
-			return (0);
+			errno = EINVAL;
+			return (-1);
 		}
+		if (imsg_get_data(imsg, &stats, sizeof(stats)) == -1)
+			return (-1);
+
 		peer = *p;
 		explicit_bzero(&peer.auth_conf, sizeof(peer.auth_conf));
 		peer.auth_conf.method = p->auth_conf.method;
@@ -590,7 +589,7 @@ control_imsg_relay(struct imsg *imsg, st
 			c->throttled = 1;
 	}
 
-	return (imsg_forward(&c->imsgbuf, imsg));
+	return imsg_forward(&c->imsgbuf, imsg);
 }
 
 void
Index: session.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
diff -u -p -r1.513 session.c
--- session.c	11 Feb 2025 19:28:45 -0000	1.513
+++ session.c	12 Feb 2025 12:59:33 -0000
@@ -3312,13 +3312,19 @@ session_dispatch_imsg(struct imsgbuf *im
 		case IMSG_CTL_SHOW_TIMER:
 			if (idx != PFD_PIPE_MAIN)
 				fatalx("ctl kroute request not from parent");
-			control_imsg_relay(&imsg, NULL);
+			if (control_imsg_relay(&imsg, NULL) == -1)
+				log_warn("control_imsg_relay");
 			break;
 		case IMSG_CTL_SHOW_NEIGHBOR:
 			if (idx != PFD_PIPE_ROUTE_CTL)
 				fatalx("ctl rib request not from RDE");
-			p = getpeerbyid(conf, peerid);
-			control_imsg_relay(&imsg, p);
+			if ((p = getpeerbyid(conf, peerid)) == NULL) {
+				log_warnx("%s: no such peer: id=%u",
+				    "IMSG_CTL_SHOW_NEIGHBOR", peerid);
+				break;
+			}
+			if (control_imsg_relay(&imsg, p) == -1)
+				log_warn("control_imsg_relay");
 			break;
 		case IMSG_CTL_SHOW_RIB:
 		case IMSG_CTL_SHOW_RIB_PREFIX:
@@ -3330,11 +3336,13 @@ session_dispatch_imsg(struct imsgbuf *im
 		case IMSG_CTL_SHOW_SET:
 			if (idx != PFD_PIPE_ROUTE_CTL)
 				fatalx("ctl rib request not from RDE");
-			control_imsg_relay(&imsg, NULL);
+			if (control_imsg_relay(&imsg, NULL) == -1)
+				log_warn("control_imsg_relay");
 			break;
 		case IMSG_CTL_END:
 		case IMSG_CTL_RESULT:
-			control_imsg_relay(&imsg, NULL);
+			if (control_imsg_relay(&imsg, NULL) == -1)
+				log_warn("control_imsg_relay");
 			break;
 		case IMSG_UPDATE:
 			if (idx != PFD_PIPE_ROUTE)