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

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.

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