From: Theo Buehler <tb@theobuehler.org>
Subject: Re: fix some fd leaks in error paths and avoid some double close
To: Omar Polo <op@omarpolo.com>
Cc: Stefan Sperling <stsp@stsp.name>, gameoftrees@openbsd.org
Date: Fri, 3 Nov 2023 16:00:32 +0100

On Fri, Nov 03, 2023 at 03:48:45PM +0100, Omar Polo wrote:
> On 2023/11/03 11:01:50 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> > On Thu, Oct 26, 2023 at 09:20:13AM +0200, Omar Polo wrote:
> > > Should we also start to use ibuf_fd_set() instead of
> > > reaching into the struct?
> > 
> > Sure, why not.
> 
> Then the diff would be this.  I'm not particularly compelled to commit
> it, since the main interesting thing about ibuf_fd_set() seems to be
> that it closes the the previous fd passed (if any).  Here we always
> create a fresh ibuf and send it.

I think it would be best if got worked under the assumption that ibufs
are opaque, even if this may not actually happen in libutil.

I'm also not sure why got always does wbuf->fd = -1 before imsg_close().
This fd is already -1 from imsg_create() (via ibuf_dynamic()).

> 
> diff /home/op/w/got
> commit - ac4f092c7c8e090ee733c90b8b0f0274872c8662
> path + /home/op/w/got
> blob - 7d479df74800ae6ac1809d68db5f4d54482efd23
> file + lib/privsep.c
> --- lib/privsep.c
> +++ lib/privsep.c
> @@ -419,7 +419,7 @@ got_privsep_send_tree_req(struct imsgbuf *ibuf, int fd
>  		}
>  	}
>  
> -	wbuf->fd = fd;
> +	ibuf_fd_set(wbuf, fd);
>  	imsg_close(ibuf, wbuf);
>  
>  	return flush_imsg(ibuf);
> @@ -612,7 +612,7 @@ got_privsep_send_fetch_req(struct imsgbuf *ibuf, int f
>  			return err;
>  		}
>  	}
> -	wbuf->fd = fd;
> +	ibuf_fd_set(wbuf, fd);
>  	fd = -1;
>  	imsg_close(ibuf, wbuf);
>  
>