From: Omar Polo <op@omarpolo.com>
Subject: Re: gotd: fork repo children on demand
To: Omar Polo <op@omarpolo.com>
Cc: Stefan Sperling <stsp@stsp.name>, gameoftrees@openbsd.org
Date: Thu, 29 Dec 2022 12:24:26 +0100

On 2022/12/29 11:00:49 +0100, Omar Polo <op@omarpolo.com> wrote:
> On 2022/12/27 09:21:31 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> > On Mon, Dec 26, 2022 at 06:22:06PM +0100, Stefan Sperling wrote:
> > > Fork gotd repo children on demand, instead of just once at startup.
> > > This is another step on the road towards better privsep.
> > > 
> > > For now, we need to add the "exec" pledge promise back to the parent process.
> > > It will be removed again in a later step.
> > > 
> > > ok?
> > 
> > The previous version of this patch had two problems:
> > 
> > - The parent did not wait for repo children after sending them a
> >   SIGTERM, which could lead to zombie processes accumulating.
> > 
> > - The parent forgot to free its struct proc during disconnect(),
> >   resulting in a memory leak.
> > 
> > Both issues fixed here.
> 
> just a comment below, but otherwise ok for me.

please disregard, i've seen only now that this was all fixed in the
follow-up diff.

ok op@ with the style tweaks from jamsek

> > diff 9d0feb8b5d4a20276efaf3f29df59ade82cd38aa 34d54e2690024fb12b89c1c160170efaa286c2df
> > commit - 9d0feb8b5d4a20276efaf3f29df59ade82cd38aa
> > commit + 34d54e2690024fb12b89c1c160170efaa286c2df
> > blob - 97731f1e9b245bfea3cec151f3d56b6c81205390
> > blob + 3040877532eed07c6a4bd0d310bf4b9564ceab06
> > --- gotd/gotd.c
> > +++ gotd/gotd.c
> > [...]
> > @@ -295,11 +315,35 @@ disconnect(struct gotd_client *client)
> >  }
> >  
> >  static void
> > +wait_for_children(pid_t child_pid)
> > +{
> > +	pid_t pid;
> > +	int status;
> > +
> > +	if (child_pid == 0)
> > +		log_debug("waiting for children to terminate");
> 
> wait_for_children is always called with proc->pid, can child_pid be 0?
> is it for the future?
> 
> > +	else
> > +		log_debug("waiting for child PID %ld to terminate",
> > +		    (long)child_pid);
> > +
> > +	do {
> > +		pid = wait(&status);
> 
> it's not really waiting for `child_pid'.  Could it catch another
> process dieing and not the targeted one?
> 
> > +		if (pid == -1) {
> > +			if (errno != EINTR && errno != ECHILD)
> > +				fatal("wait");
> > +		} else if (WIFSIGNALED(status)) {
> > +			log_warnx("child PID %ld terminated; signal %d",
> > +			    (long)pid, WTERMSIG(status));
> > +		}	
> > +	} while (pid != -1 || (pid == -1 && errno == EINTR));
> 
> not an issue, but can be simplified to
> 
> +	} while (pid != -1 || errno == EINTR);