From: Omar Polo Subject: Re: fix some fd leaks in error paths and avoid some double close To: Theo Buehler Cc: Stefan Sperling , gameoftrees@openbsd.org Date: Fri, 03 Nov 2023 16:39:25 +0100 On 2023/11/03 16:00:32 +0100, Theo Buehler wrote: > On Fri, Nov 03, 2023 at 03:48:45PM +0100, Omar Polo wrote: > > On 2023/11/03 11:01:50 +0100, Stefan Sperling 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. ah ok, good to know, I'll go ahead with this then. > 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()). don't know either, but I've dropped them all just before sending this diff. https://got.omarpolo.com/?action=diff&commit=ac4f092c7c8e090ee733c90b8b0f0274872c8662&headref=HEAD&path=got.git > > > > 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); > > > >