From: Omar Polo Subject: Re: gotd: run authentication in separate process To: Stefan Sperling Cc: gameoftrees@openbsd.org Date: Thu, 29 Dec 2022 16:48:31 +0100 On 2022/12/29 16:11:57 +0100, Stefan Sperling wrote: > On Thu, Dec 29, 2022 at 12:55:03PM +0100, Omar Polo wrote: > > > + switch (sig) { > > > + case SIGHUP: > > > + break; > > > + case SIGUSR1: > > > + break; > > > > just for curiosity, what's the point of catching SIGUSR1 too? the > > listener process and gotwebd do that too (but gotwebd due to proc.c I > > guess), but otherwise they don't do anything with it. > > No idea. This was just copied along from other daemons. > The HUP and USR signals are often used to trigger actions such as > reloading config files. I suspect that is why they are present in > all these signal handlers. I see. diff below drops it for gotd but I wouldn't mind to keep it since it's there even in other daemons that don't do anything with it (e.g. see relayd.) > > > + if (client->auth->iev.ibuf.fd != fd) > > > + fatalx("%s: unexpected fd %d", __func__, fd); > > > > it's a bit redundant to check the fd, since find_client_by_proc_fd > > should return a client with that fd. what i'm missing? > > Indeed. This is just a guard against accidents. I am not very happy > with find_client_by_proc_fd() because it is looping over the entire > client hash table. But I don't want to store a pointer inside struct > gotd_imsgev either, because this makes use-after-free bugs more likely > to happen. I am not sure yet if there is a better way. > > Once we have all separate processes in place, we can do another sweep > of all the sanity checks and see which ones still make sense. There is > also the verify_imsg_src() function which is currently just being copied > along, and could eventually be dropped or refined. ACK! :) > > > + if ((n = imsg_get(ibuf, &imsg)) == -1) > > > + fatal("%s: imsg_get error", __func__); > > > + if (n == 0) /* No more messages. */ > > > + return; > > > > any reason not to do the usual loop here around imsg_get? it works > > like this, but if we ever grow the set of messages it could lead to > > issues, and looping would reduce the diff with the other dispatch > > functions. > > > > (well, not that the auth process should do more, so it's fine like it > > is already) > > The reason is that we expect the auth process to send us only one message > before we will dispose of it. I don't expect this to change, provided we > will not extend our authentication feature-set in the future. Sure. diff /home/op/w/gotd commit - 5e25db14db9eb20ee11b68048b45b3e0f54d50eb path + /home/op/w/gotd blob - 57b17a1dd26385f232f8f989e4fa041babfeafe3 file + gotd/auth.c --- gotd/auth.c +++ gotd/auth.c @@ -59,8 +59,6 @@ auth_sighdlr(int sig, short event, void *arg) switch (sig) { case SIGHUP: break; - case SIGUSR1: - break; case SIGTERM: case SIGINT: auth_shutdown(); @@ -276,7 +274,7 @@ auth_main(const char *title, struct gotd_repolist *rep { struct gotd_repo *repo = NULL; struct gotd_imsgev iev; - struct event evsigint, evsigterm, evsighup, evsigusr1; + struct event evsigint, evsigterm, evsighup; gotd_auth.title = title; gotd_auth.pid = getpid(); @@ -292,13 +290,11 @@ auth_main(const char *title, struct gotd_repolist *rep signal_set(&evsigint, SIGINT, auth_sighdlr, NULL); signal_set(&evsigterm, SIGTERM, auth_sighdlr, NULL); signal_set(&evsighup, SIGHUP, auth_sighdlr, NULL); - signal_set(&evsigusr1, SIGUSR1, auth_sighdlr, NULL); signal(SIGPIPE, SIG_IGN); signal_add(&evsigint, NULL); signal_add(&evsigterm, NULL); signal_add(&evsighup, NULL); - signal_add(&evsigusr1, NULL); imsg_init(&iev.ibuf, GOTD_FILENO_MSG_PIPE); iev.handler = auth_dispatch; blob - 2ae681053c360d76a04ee62d816fbcb536e57c4b file + gotd/gotd.c --- gotd/gotd.c +++ gotd/gotd.c @@ -1349,9 +1349,6 @@ gotd_sighdlr(int sig, short event, void *arg) case SIGHUP: log_info("%s: ignoring SIGHUP", __func__); break; - case SIGUSR1: - log_info("%s: ignoring SIGUSR1", __func__); - break; case SIGTERM: case SIGINT: gotd_shutdown(); @@ -2396,7 +2393,7 @@ main(int argc, char **argv) struct group *gr = NULL; char *repo_path = NULL; enum gotd_procid proc_id = PROC_GOTD; - struct event evsigint, evsigterm, evsighup, evsigusr1; + struct event evsigint, evsigterm, evsighup; int *pack_fds = NULL, *temp_fds = NULL; log_init(1, LOG_DAEMON); /* Log to stderr until daemonized. */ @@ -2610,13 +2607,11 @@ main(int argc, char **argv) signal_set(&evsigint, SIGINT, gotd_sighdlr, NULL); signal_set(&evsigterm, SIGTERM, gotd_sighdlr, NULL); signal_set(&evsighup, SIGHUP, gotd_sighdlr, NULL); - signal_set(&evsigusr1, SIGUSR1, gotd_sighdlr, NULL); signal(SIGPIPE, SIG_IGN); signal_add(&evsigint, NULL); signal_add(&evsigterm, NULL); signal_add(&evsighup, NULL); - signal_add(&evsigusr1, NULL); gotd_imsg_event_add(&gotd.listen_proc.iev); blob - 96427e857a4b693de2dec2665df312e682a43f73 file + gotd/listen.c --- gotd/listen.c +++ gotd/listen.c @@ -77,8 +77,6 @@ listen_sighdlr(int sig, short event, void *arg) switch (sig) { case SIGHUP: break; - case SIGUSR1: - break; case SIGTERM: case SIGINT: listen_shutdown(); @@ -340,7 +338,7 @@ listen_main(const char *title, int gotd_socket) listen_main(const char *title, int gotd_socket) { struct gotd_imsgev iev; - struct event evsigint, evsigterm, evsighup, evsigusr1; + struct event evsigint, evsigterm, evsighup; gotd_listen.title = title; gotd_listen.pid = getpid(); @@ -349,13 +347,11 @@ listen_main(const char *title, int gotd_socket) signal_set(&evsigint, SIGINT, listen_sighdlr, NULL); signal_set(&evsigterm, SIGTERM, listen_sighdlr, NULL); signal_set(&evsighup, SIGHUP, listen_sighdlr, NULL); - signal_set(&evsigusr1, SIGUSR1, listen_sighdlr, NULL); signal(SIGPIPE, SIG_IGN); signal_add(&evsigint, NULL); signal_add(&evsigterm, NULL); signal_add(&evsighup, NULL); - signal_add(&evsigusr1, NULL); imsg_init(&iev.ibuf, GOTD_FILENO_MSG_PIPE); iev.handler = listen_dispatch;