This is now working on www.openbsd.org. I upgraded my
6.0 system to current today off the latest snap and httpd would
not start, same problem.
This diff lets current httpd start again.
ok beck@
On Tue, Oct 04, 2016 at 11:54:37PM +0200, Rafael Zalamena wrote:
> On Tue, Oct 04, 2016 at 07:46:52PM +0200, Rafael Zalamena wrote:
> > This diff makes proc.c daemons to use less file descriptors on startup,
> > this way we increase the number of child we can have considerably. This
> > also improves the solution on a bug reported in bugs@
> > "httpd errors out with 'too many open files'".
> >
> > To achieve that I delayed the socket distribution and made a minimal
> > socket allocation in proc_init(), so only the necessary children socket
> > are allocated and passed with proc_exec(). After the event_init() is called
> > we call proc_connect() which creates the socketpair() and immediatly after
> > each call we already sends them without accumulating.
> >
> > Note: We still have to calculate how many fds we will want to have and
> > then limit the daemon prefork configuration.
> >
> > ok?
> >
>
> Paul de Weerd still found problems with the diff, because the httpd(8)
> would not exit successfully with '-n' flag. It happened because the
> new proc_connect() code tried to write fds to children process that
> did not start caused by the ps_noaction flag. (thanks Paul!)
>
> This new diff just adds a check for ps_noaction in proc_init() and
> proc_connect() so it doesn't try to do anything if we are not really
> going to start the daemon. Also I removed the ps_noaction from proc_run()
> since we are not going to get to this point anymore.
>
> ok?
>
>
> Index: proc.c
> ===================================================================
> RCS file: /home/obsdcvs/src/usr.sbin/httpd/proc.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 proc.c
> --- proc.c 28 Sep 2016 12:01:04 -0000 1.27
> +++ proc.c 4 Oct 2016 21:50:43 -0000
> @@ -36,8 +36,6 @@
>
> void proc_exec(struct privsep *, struct privsep_proc *, unsigned int,
> int, char **);
> -void proc_connectpeer(struct privsep *, enum privsep_procid, int,
> - struct privsep_pipes *);
> void proc_setup(struct privsep *, struct privsep_proc *, unsigned int);
> void proc_open(struct privsep *, int, int);
> void proc_accept(struct privsep *, int, enum privsep_procid,
> @@ -147,72 +145,18 @@ proc_exec(struct privsep *ps, struct pri
> }
>
> void
> -proc_connectpeer(struct privsep *ps, enum privsep_procid id, int inst,
> - struct privsep_pipes *pp)
> -{
> - unsigned int i, j;
> - struct privsep_fd pf;
> -
> - for (i = 0; i < PROC_MAX; i++) {
> - /* Parent is already connected with everyone. */
> - if (i == PROC_PARENT)
> - continue;
> -
> - for (j = 0; j < ps->ps_instances[i]; j++) {
> - /* Don't send socket to child itself. */
> - if (i == (unsigned int)id &&
> - j == (unsigned int)inst)
> - continue;
> - if (pp->pp_pipes[i][j] == -1)
> - continue;
> -
> - pf.pf_procid = i;
> - pf.pf_instance = j;
> - proc_compose_imsg(ps, id, inst, IMSG_CTL_PROCFD,
> - -1, pp->pp_pipes[i][j], &pf, sizeof(pf));
> - pp->pp_pipes[i][j] = -1;
> - }
> - }
> -}
> -
> -/* Inter-connect all process except with ourself. */
> -void
> proc_connect(struct privsep *ps)
> {
> - unsigned int src, i, j;
> - struct privsep_pipes *pp;
> - struct imsgev *iev;
> -
> - /* Listen on appropriate pipes. */
> - src = privsep_process;
> - pp = &ps->ps_pipes[src][ps->ps_instance];
> -
> - for (i = 0; i < PROC_MAX; i++) {
> - /* Don't listen to ourself. */
> - if (i == src)
> - continue;
> -
> - for (j = 0; j < ps->ps_instances[i]; j++) {
> - if (pp->pp_pipes[i][j] == -1)
> - continue;
> -
> - iev = &ps->ps_ievs[i][j];
> - imsg_init(&iev->ibuf, pp->pp_pipes[i][j]);
> - event_set(&iev->ev, iev->ibuf.fd, iev->events,
> - iev->handler, iev->data);
> - event_add(&iev->ev, NULL);
> - }
> - }
> + unsigned int src, dst;
>
> - /* Exchange pipes between process. */
> - for (i = 0; i < PROC_MAX; i++) {
> - /* Parent is already connected with everyone. */
> - if (i == PROC_PARENT)
> - continue;
> + /* Don't distribute any sockets if we are not really going to run. */
> + if (ps->ps_noaction)
> + return;
>
> - for (j = 0; j < ps->ps_instances[i]; j++)
> - proc_connectpeer(ps, i, j, &ps->ps_pipes[i][j]);
> - }
> + /* Distribute the socketpair()s for everyone. */
> + for (src = 0; src < PROC_MAX; src++)
> + for (dst = src; dst < PROC_MAX; dst++)
> + proc_open(ps, src, dst);
> }
>
> void
> @@ -220,17 +164,41 @@ proc_init(struct privsep *ps, struct pri
> int argc, char **argv, enum privsep_procid proc_id)
> {
> struct privsep_proc *p = NULL;
> + struct privsep_pipes *pa, *pb;
> unsigned int proc;
> - unsigned int src, dst;
> + unsigned int dst;
> + int fds[2];
> +
> + /* Don't initiate anything if we are not really going to run. */
> + if (ps->ps_noaction)
> + return;
>
> if (proc_id == PROC_PARENT) {
> privsep_process = PROC_PARENT;
> proc_setup(ps, procs, nproc);
>
> - /* Open socketpair()s for everyone. */
> - for (src = 0; src < PROC_MAX; src++)
> - for (dst = 0; dst < PROC_MAX; dst++)
> - proc_open(ps, src, dst);
> + /*
> + * Create the children sockets so we can use them
> + * to distribute the rest of the socketpair()s using
> + * proc_connect() later.
> + */
> + for (dst = 0; dst < PROC_MAX; dst++) {
> + /* Don't create socket for ourselves. */
> + if (dst == PROC_PARENT)
> + continue;
> +
> + for (proc = 0; proc < ps->ps_instances[dst]; proc++) {
> + pa = &ps->ps_pipes[proc_id][ps->ps_instance];
> + pb = &ps->ps_pipes[dst][proc];
> + if (socketpair(AF_UNIX,
> + SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC,
> + PF_UNSPEC, fds) == -1)
> + fatal("socketpair");
> +
> + pa->pp_pipes[dst][proc] = fds[0];
> + pb->pp_pipes[proc_id][ps->ps_instance] = fds[1];
> + }
> + }
>
> /* Engage! */
> proc_exec(ps, procs, nproc, argc, argv);
> @@ -399,9 +367,16 @@ void
> proc_open(struct privsep *ps, int src, int dst)
> {
> struct privsep_pipes *pa, *pb;
> + struct imsgev *iev;
> + struct privsep_fd pf;
> int fds[2];
> unsigned int i, j;
>
> + /* We don't have connections to the target process. */
> + if (ps->ps_ievs[dst] == NULL)
> + return;
> +
> + /* Exchange pipes between process. */
> for (i = 0; i < ps->ps_instances[src]; i++) {
> for (j = 0; j < ps->ps_instances[dst]; j++) {
> /* Don't create sockets for ourself. */
> @@ -410,16 +385,63 @@ proc_open(struct privsep *ps, int src, i
>
> pa = &ps->ps_pipes[src][i];
> pb = &ps->ps_pipes[dst][j];
> - if (pb->pp_pipes[dst][j] != -1)
> + if (pa->pp_pipes[dst][j] == -1 &&
> + pb->pp_pipes[src][i] == -1) {
> + if (socketpair(AF_UNIX,
> + SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC,
> + PF_UNSPEC, fds) == -1)
> + fatal("socketpair");
> +
> + pa->pp_pipes[dst][j] = fds[0];
> + pb->pp_pipes[src][i] = fds[1];
> + }
> +
> + iev = &ps->ps_ievs[dst][j];
> + if (iev->ibuf.fd == 0) {
> + imsg_init(&iev->ibuf, pa->pp_pipes[dst][j]);
> + event_set(&iev->ev, iev->ibuf.fd, iev->events,
> + iev->handler, iev->data);
> + event_add(&iev->ev, NULL);
> + }
> +
> + /*
> + * The parent/child pipe was handed by proc_exec()
> + * and ps_ievs[PROC_PARENT] is not allocated here,
> + * so skip imsg_init() and fd send.
> + */
> + if (src == PROC_PARENT)
> continue;
>
> - if (socketpair(AF_UNIX,
> - SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC,
> - PF_UNSPEC, fds) == -1)
> - fatal(__func__);
> + iev = &ps->ps_ievs[src][i];
> + if (iev->ibuf.fd == 0) {
> + imsg_init(&iev->ibuf, pa->pp_pipes[src][i]);
> + event_set(&iev->ev, iev->ibuf.fd, iev->events,
> + iev->handler, iev->data);
> + event_add(&iev->ev, NULL);
> + }
> +
> + pf.pf_procid = src;
> + pf.pf_instance = i;
> + if (proc_compose_imsg(ps, dst, j, IMSG_CTL_PROCFD,
> + -1, pb->pp_pipes[src][i], &pf, sizeof(pf)) == -1)
> + fatal("proc_compose_imsg");
>
> - pa->pp_pipes[dst][j] = fds[0];
> - pb->pp_pipes[src][i] = fds[1];
> + pf.pf_procid = dst;
> + pf.pf_instance = j;
> + if (proc_compose_imsg(ps, src, i, IMSG_CTL_PROCFD,
> + -1, pa->pp_pipes[dst][j], &pf, sizeof(pf)) == -1)
> + fatal("proc_compose_imsg");
> +
> + /*
> + * We have to flush to send the descriptors and close
> + * them to avoid the fd ramp on startup.
> + */
> + if (imsg_flush(&ps->ps_ievs[dst][j].ibuf) == -1 ||
> + imsg_flush(&ps->ps_ievs[src][i].ibuf) == -1)
> + fatal("imsg_flush");
> +
> + pa->pp_pipes[dst][j] = -1;
> + pb->pp_pipes[src][i] = -1;
> }
> }
> }
> @@ -501,9 +523,6 @@ proc_run(struct privsep *ps, struct priv
> struct passwd *pw = ps->ps_pw;
> const char *root;
> struct control_sock *rcs;
> -
> - if (ps->ps_noaction)
> - exit(0);
>
> log_procinit(p->p_title);
>
>
