From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: IMSG_CTL_SHOW_NEIGHBOR handling
To: tech@openbsd.org
Date: Wed, 12 Feb 2025 14:07:01 +0100

On Wed, Feb 12, 2025 at 02:01:27PM +0100, Claudio Jeker wrote:
> 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.

Much better. And agreed re logging.

ok tb

> 
> -- 
> :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)