Merge branch 'saned-foreground-listener' into 'master'

saned: fix child lifecycle management in standalone mode

Closes #776

See merge request sane-project/backends!854

configure.ac

@@ -313,6 +313,7 @@ AC_FUNC_MMAP
 AC_CHECK_FUNCS(atexit ioperm i386_set_ioperm \
     mkdir strftime strstr strtod  \
     cfmakeraw tcsendbreak strcasecmp strncasecmp _portaccess \
+    pidfd_open \
     getaddrinfo getnameinfo poll setitimer iopl getuid getpass)
 
 dnl sys/io.h might provide ioperm but not inb,outb (like for

frontend/saned.c

@@ -79,6 +79,9 @@
 #include <arpa/inet.h>
 
 #include <sys/wait.h>
+#ifdef HAVE_PIDFD_OPEN
+#include <sys/pidfd.h>
+#endif
 
 #include <pwd.h>
 #include <grp.h>
@@ -168,8 +171,6 @@ poll (struct pollfd *ufds, unsigned int nfds, int timeout)
 # define SANED_SERVICE_DNS "_sane-port._tcp"
 # define SANED_NAME "saned"
 
-pid_t avahi_pid = -1;
-
 char *avahi_svc_name;
 
 static AvahiClient *avahi_client = NULL;
@@ -222,13 +223,36 @@ static AvahiEntryGroup *avahi_group = NULL;
 # define PATH_MAX 1024
 #endif
 
+/* Linked list of child processes */
+enum saned_child_type {
+  SANED_CHILD_CLIENT,
+  SANED_CHILD_AVAHI,
+};
 struct saned_child {
+  enum saned_child_type type;
   pid_t pid;
+  int pidfd;
   struct saned_child *next;
 };
 struct saned_child *children;
 int numchildren;
 
+/* Linked list of fds to be polled */
+enum saned_fd_type {
+  SANED_FD_LISTENER,
+  SANED_FD_PROCESS,
+};
+struct saned_fd
+{
+  enum saned_fd_type type;
+  int fd;
+  short interesting_events;
+  struct saned_fd *next;
+};
+struct saned_fd *saned_fds;
+int num_saned_fds;
+
+
 #define SANED_CONFIG_FILE "saned.conf"
 #define SANED_PID_FILE    "/var/run/saned.pid"
 
@@ -2295,12 +2319,85 @@ process_request (Wire * w)
   return 0;
 }
 
+static int
+add_fd (int fd, enum saned_fd_type type, short interesting_events)
+{
+  struct saned_fd *f;
+
+  f = (struct saned_fd *) malloc (sizeof(struct saned_fd));
+  if (f == NULL)
+    goto fail;
+
+  f->type = type;
+  f->fd = fd;
+  f->interesting_events = interesting_events;
+  f->next = saned_fds;
+
+  saned_fds = f;
+  num_saned_fds++;
+
+  return fd;
+
+fail:
+  DBG (DBG_ERR, "add_fd: cannot manage fd, %s\n", strerror(errno));
+  close(fd);
+  return -1;
+}
+
+static void
+close_fds(unsigned type_mask, int specific_fd)
+{
+  struct saned_fd *f, **nextp;
+
+  for (nextp = &saned_fds; (f = *nextp); /* */) {
+    if (type_mask & (1 << f->type) ||
+	specific_fd == f->fd) {
+      close(f->fd);
+      *nextp = f->next;
+      num_saned_fds--;
+      free(f);
+    } else
+      nextp = &f->next;
+  }
+}
+
+static void
+add_child (pid_t pid, enum saned_child_type type)
+{
+  struct saned_child *c;
+
+  c = (struct saned_child *) malloc (sizeof(struct saned_child));
+  if (c == NULL)
+    goto fail;
+
+  c->type = type;
+  c->pid = pid;
+  c->pidfd = -1;
+  c->next = children;
+
+#ifdef HAVE_PIDFD_OPEN
+  c->pidfd = pidfd_open(pid, 0);
+  if (c->pidfd == -1)
+    DBG (DBG_DBG, "add_child: could not open pidfd for child process, %s\n", strerror(errno));
+  else
+    add_fd(c->pidfd, SANED_FD_PROCESS, POLLIN);
+#endif
+
+  children = c;
+  numchildren++;
+  return;
+
+fail:
+  /* If the child process cannot be managed, kill it now. */
+  DBG (DBG_ERR, "add_child: cannot manage child process, %s\n", strerror(errno));
+  kill(pid, SIGTERM);
+}
+
 
 static int
 wait_child (pid_t pid, int *status, int options)
 {
-  struct saned_child *c;
+  struct saned_child *c, **nextp;
-  struct saned_child *p = NULL;
   int ret;
 
   ret = waitpid(pid, status, options);
@@ -2308,56 +2405,19 @@ wait_child (pid_t pid, int *status, int options)
   if (ret <= 0)
     return ret;
 
-#if WITH_AVAHI
+  for (nextp = &children; (c = *nextp); /* */) {
-  if ((avahi_pid > 0) && (ret == avahi_pid))
+    if (c->pid == ret) {
-    {
+      *nextp = c->next;
-      avahi_pid = -1;
       numchildren--;
-      return ret;
+      close_fds(0, c->pidfd);
-    }
+      free(c);
-#endif /* WITH_AVAHI */
+      break;
-
+    } else
-  for (c = children; (c != NULL) && (c->next != NULL); p = c, c = c->next)
+      nextp = &c->next;
-    {
+  }
-      if (c->pid == ret)
-	{
-	  if (c == children)
-	    children = c->next;
-	  else if (p != NULL)
-	    p->next = c->next;
-
-	  free(c);
-
-	  numchildren--;
-
-	  break;
-	}
-    }
-
   return ret;
 }
 
-static int
-add_child (pid_t pid)
-{
-  struct saned_child *c;
-
-  c = (struct saned_child *) malloc (sizeof(struct saned_child));
-
-  if (c == NULL)
-    {
-      DBG (DBG_ERR, "add_child: out of memory\n");
-      return -1;
-    }
-
-  c->pid = pid;
-  c->next = children;
-
-  children = c;
-
-  return 0;
-}
-
 
 static void
 handle_connection (int fd)
@@ -2384,7 +2444,7 @@ handle_connection (int fd)
     p = getprotobyname ("tcp");
     if (p == 0)
       {
-	DBG (DBG_WARN, "handle_connection: cannot look up `tcp' protocol number");
+	DBG (DBG_WARN, "handle_connection: cannot look up `tcp' protocol number\n");
       }
     else
       level = p->p_proto;
@@ -2392,7 +2452,7 @@ handle_connection (int fd)
 # endif	/* SOL_TCP */
   if (level == -1
       || setsockopt (wire.io.fd, level, TCP_NODELAY, &on, sizeof (on)))
-    DBG (DBG_WARN, "handle_connection: failed to put socket in TCP_NODELAY mode (%s)",
+    DBG (DBG_WARN, "handle_connection: failed to put socket in TCP_NODELAY mode (%s)\n",
 	 strerror (errno));
 #endif /* !TCP_NODELAY */
 
@@ -2434,8 +2494,8 @@ handle_client (int fd)
   else if (pid > 0)
     {
       /* parent */
-      add_child (pid);
       close(fd);
+      add_child (pid, SANED_CHILD_CLIENT);
     }
   else
     {
@@ -2445,21 +2505,29 @@ handle_client (int fd)
     }
 }
 
+static void
+kill_children(int sig)
+{
+  struct saned_child *c;
+
+  /* The only type of child we kill is Avahi. */
+  for (c = children; c; c = c->next)
+    if (c->type == SANED_CHILD_AVAHI)
+      kill(c->pid, sig);
+}
+
 static void
 bail_out (int error)
 {
   DBG (DBG_ERR, "%sbailing out, waiting for children...\n", (error) ? "FATAL ERROR; " : "");
 
-#if WITH_AVAHI
+  kill_children(SIGTERM);
-  if (avahi_pid > 0)
-    kill (avahi_pid, SIGTERM);
-#endif /* WITH_AVAHI */
-
   while (numchildren > 0)
     wait_child (-1, NULL, 0);
 
   DBG (DBG_ERR, "bail_out: all children exited\n");
 
+  close_fds(-1, -1);
   exit ((error) ? 1 : 0);
 }
 
@@ -2481,7 +2549,7 @@ sig_int_term_handler (int signum)
 
 #if WITH_AVAHI
 static void
-saned_avahi (struct pollfd *fds, int nfds);
+saned_avahi (void);
 
 static void
 saned_create_avahi_services (AvahiClient *c);
@@ -2494,16 +2562,16 @@ saned_avahi_group_callback (AvahiEntryGroup *g, AvahiEntryGroupState state, void
 
 
 static void
-saned_avahi (struct pollfd *fds, int nfds)
+saned_avahi (void)
 {
-  struct pollfd *fdp = NULL;
+  int avahi_pid;
   int error;
 
   avahi_pid = fork ();
 
   if (avahi_pid > 0)
     {
-      numchildren++;
+      add_child(avahi_pid, SANED_CHILD_AVAHI);
       return;
     }
   else if (avahi_pid < 0)
@@ -2515,11 +2583,8 @@ saned_avahi (struct pollfd *fds, int nfds)
   signal (SIGINT, NULL);
   signal (SIGTERM, NULL);
 
-  /* Close network fds */
+  /* Close parent fds */
-  for (fdp = fds; nfds > 0; nfds--, fdp++)
+  close_fds(-1, -1);
-    close (fdp->fd);
-
-  free(fds);
 
   avahi_svc_name = avahi_strdup(SANED_NAME);
 
@@ -2854,17 +2919,15 @@ read_config (void)
 
 #ifdef SANED_USES_AF_INDEP
 static void
-do_bindings_family (int family, int *nfds, struct pollfd **fds, struct addrinfo *res)
+do_bindings_family (int family, struct addrinfo *res)
 {
   struct addrinfo *resp;
-  struct pollfd *fdp;
   short sane_port;
   int fd = -1;
   int on = 1;
   int i;
 
   sane_port = bind_port;
-  fdp = *fds;
 
   for (resp = res, i = 0; resp != NULL; resp = resp->ai_next, i++)
     {
@@ -2954,23 +3017,15 @@ do_bindings_family (int family, int *nfds, struct pollfd **fds, struct addrinfo
 	    }
 	}
 
-      fdp->fd = fd;
+      add_fd(fd, SANED_FD_LISTENER, POLLIN);
-      fdp->events = POLLIN;
-
-      (*nfds)++;
-      fdp++;
     }
-
-  *fds = fdp;
 }
 
 static void
-do_bindings (int *nfds, struct pollfd **fds)
+do_bindings (void)
 {
   struct addrinfo *res;
-  struct addrinfo *resp;
   struct addrinfo hints;
-  struct pollfd *fdp;
   int err;
 
   DBG (DBG_DBG, "do_bindings: trying to get port for service \"%s\" (getaddrinfo)\n", SANED_SERVICE_NAME);
@@ -2995,31 +3050,15 @@ do_bindings (int *nfds, struct pollfd **fds)
 	}
     }
 
-  for (resp = res, *nfds = 0; resp != NULL; resp = resp->ai_next, (*nfds)++)
-    ;
-
-  *fds = malloc (*nfds * sizeof (struct pollfd));
-
-  if (fds == NULL)
-    {
-      DBG (DBG_ERR, "do_bindings: not enough memory for fds\n");
-      freeaddrinfo (res);
-      bail_out (1);
-    }
-
-  fdp = *fds;
-  *nfds = 0;
-
   /* bind IPv6 first, IPv4 second */
 #ifdef ENABLE_IPV6
-  do_bindings_family (AF_INET6, nfds, &fdp, res);
+  do_bindings_family (AF_INET6, res);
 #endif
-  do_bindings_family (AF_INET, nfds, &fdp, res);
+  do_bindings_family (AF_INET, res);
 
-  resp = NULL;
   freeaddrinfo (res);
 
-  if (*nfds <= 0)
+  if (res == NULL)
     {
       DBG (DBG_ERR, "do_bindings: couldn't bind an address. Exiting.\n");
       bail_out (1);
@@ -3029,7 +3068,7 @@ do_bindings (int *nfds, struct pollfd **fds)
 #else /* !SANED_USES_AF_INDEP */
 
 static void
-do_bindings (int *nfds, struct pollfd **fds)
+do_bindings (void)
 {
   struct sockaddr_in sin;
   struct servent *serv;
@@ -3053,15 +3092,6 @@ do_bindings (int *nfds, struct pollfd **fds)
       DBG (DBG_WARN, "do_bindings: to your /etc/services file (or equivalent). Proceeding anyway.\n");
     }
 
-  *nfds = 1;
-  *fds = malloc (*nfds * sizeof (struct pollfd));
-
-  if (fds == NULL)
-    {
-      DBG (DBG_ERR, "do_bindings: not enough memory for fds\n");
-      bail_out (1);
-    }
-
   memset (&sin, 0, sizeof (sin));
 
   sin.sin_family = AF_INET;
@@ -3076,24 +3106,23 @@ do_bindings (int *nfds, struct pollfd **fds)
 
   DBG (DBG_DBG, "do_bindings: setsockopt ()\n");
   if (setsockopt (fd, SOL_SOCKET, SO_REUSEADDR, &on, sizeof (on)))
-    DBG (DBG_ERR, "do_bindings: failed to put socket in SO_REUSEADDR mode (%s)", strerror (errno));
+    DBG (DBG_ERR, "do_bindings: failed to put socket in SO_REUSEADDR mode (%s)\n", strerror (errno));
 
   DBG (DBG_DBG, "do_bindings: bind ()\n");
   if (bind (fd, (struct sockaddr *) &sin, sizeof (sin)) < 0)
     {
-      DBG (DBG_ERR, "do_bindings: bind failed: %s", strerror (errno));
+      DBG (DBG_ERR, "do_bindings: bind failed: %s\n", strerror (errno));
       bail_out (1);
     }
 
   DBG (DBG_DBG, "do_bindings: listen ()\n");
   if (listen (fd, 1) < 0)
     {
-      DBG (DBG_ERR, "do_bindings: listen failed: %s", strerror (errno));
+      DBG (DBG_ERR, "do_bindings: listen failed: %s\n", strerror (errno));
       bail_out (1);
     }
 
-  (*fds)->fd = fd;
+  add_fd(fd, SANED_FD_LISTENER, POLLIN);
-  (*fds)->events = POLLIN;
 }
 
 #endif /* SANED_USES_AF_INDEP */
@@ -3210,16 +3239,16 @@ runas_user (char *user)
 static void
 run_standalone (char *user)
 {
-  struct pollfd *fds = NULL;
+  struct pollfd *poll_set = NULL;
-  struct pollfd *fdp = NULL;
+  int poll_set_valid = SANE_FALSE;
-  int nfds;
+  int running = SANE_TRUE;
   int fd = -1;
   int i;
   int ret;
 
   FILE *pidfile;
 
-  do_bindings (&nfds, &fds);
+  do_bindings ();
 
   if (run_foreground == SANE_FALSE)
     {
@@ -3255,7 +3284,11 @@ run_standalone (char *user)
       else
 	DBG (DBG_ERR, "Could not write PID file: %s\n", strerror (errno));
 
-      chdir ("/");
+      if (chdir ("/") != 0)
+	{
+	  DBG (DBG_ERR, "Could not change to root directory: %s\n", strerror (errno));
+	  exit(1);
+	}
 
       dup2 (fd, STDIN_FILENO);
       dup2 (fd, STDOUT_FILENO);
@@ -3264,26 +3297,55 @@ run_standalone (char *user)
       close (fd);
 
       setsid ();
-
-      signal(SIGINT, sig_int_term_handler);
-      signal(SIGTERM, sig_int_term_handler);
     }
 
+  signal(SIGINT, sig_int_term_handler);
+  signal(SIGTERM, sig_int_term_handler);
+
   if (user)
     runas_user(user);
 
 #if WITH_AVAHI
   DBG (DBG_INFO, "run_standalone: spawning Avahi process\n");
-  saned_avahi (fds, nfds);
+  saned_avahi ();
 
   /* NOT REACHED (Avahi process) */
 #endif /* WITH_AVAHI */
 
   DBG (DBG_MSG, "run_standalone: waiting for control connection\n");
 
-  while (1)
+  while (running)
     {
-      ret = poll (fds, nfds, 500);
+      struct saned_child *child;
+      struct saned_fd *sfd;
+      int timeout_needed = SANE_FALSE;
+      int do_rebind = SANE_FALSE;
+      int do_reap;
+
+      for (child = children; child; child = child->next)
+	if (child->pidfd == -1)
+	  timeout_needed = SANE_TRUE;
+      do_reap = timeout_needed;
+
+      if (!poll_set_valid)
+	{
+	  void *new_poll_set = realloc(poll_set, num_saned_fds * sizeof *poll_set);
+	  if (new_poll_set == NULL && num_saned_fds != 0)
+	    {
+	      DBG (DBG_ERR, "run_standalone: poll set allocation failed: %s\n", strerror (errno));
+	      free(poll_set);
+	      bail_out (1);
+	    }
+	  poll_set = (struct pollfd *) new_poll_set;
+	  for (sfd = saned_fds, i = 0; sfd; sfd = sfd->next, i++) {
+	    poll_set[i].fd = sfd->fd;
+	    poll_set[i].events = sfd->interesting_events;
+	  }
+	  assert(i == num_saned_fds);
+	  poll_set_valid = SANE_TRUE;
+	}
+
+      ret = poll (poll_set, num_saned_fds, timeout_needed ? 500 : -1);
       if (ret < 0)
 	{
 	  if (errno == EINTR)
@@ -3291,59 +3353,67 @@ run_standalone (char *user)
 	  else
 	    {
 	      DBG (DBG_ERR, "run_standalone: poll failed: %s\n", strerror (errno));
-	      free (fds);
+	      close_fds(-1, -1);
 	      bail_out (1);
 	    }
 	}
 
-      /* Wait for children */
+      /* Do not allow fd list to change while iterating over poll events, otherwise
-      while (wait_child (-1, NULL, WNOHANG) > 0)
+       * we shall have to look them up each time. */
-	;
+      for (sfd = saned_fds, i = 0; ret != 0 && i < num_saned_fds; i++, sfd = sfd->next)
-
-      if (ret == 0)
-	continue;
-
-      for (i = 0, fdp = fds; i < nfds; i++, fdp++)
 	{
-	  /* Error on an fd */
+	  struct pollfd *pfd = poll_set + i;
-	  if (fdp->revents & (POLLERR | POLLHUP | POLLNVAL))
-	    {
-	      for (i = 0, fdp = fds; i < nfds; i++, fdp++)
-		close (fdp->fd);
 
-	      free (fds);
+	  assert(sfd);
+	  assert(sfd->fd == pfd->fd);
 
-	      DBG (DBG_WARN, "run_standalone: invalid fd in set, attempting to re-bind\n");
+	  if (pfd->revents == 0)
-
-	      /* Reopen sockets */
-	      do_bindings (&nfds, &fds);
-
-	      break;
-	    }
-	  else if (! (fdp->revents & POLLIN))
 	    continue;
+	  else
+	    ret--;
 
-	  fd = accept (fdp->fd, 0, 0);
+	  switch (sfd->type) {
-	  if (fd < 0)
+	  case SANED_FD_LISTENER:
-	    {
+	    if (pfd->revents & (POLLERR | POLLHUP | POLLNVAL))
-	      DBG (DBG_ERR, "run_standalone: accept failed: %s", strerror (errno));
+	      do_rebind = SANE_TRUE;
-	      continue;
+	    else if (pfd->revents & POLLIN)
+	      {
+		fd = accept (sfd->fd, 0, 0);
+		if (fd < 0 && errno != EAGAIN)
+		  DBG (DBG_ERR, "run_standalone: accept failed: %s\n", strerror (errno));
+		else if (fd >= 0)
+		  {
+		    handle_client (fd);
+		    if (run_once == SANE_TRUE)
+		      running = SANE_FALSE; /* We have handled the only connection we're going to handle */
+		  }
+	      }
+	    break;
+	  case SANED_FD_PROCESS:
+	    if (pfd->revents & POLLIN) {
+	      do_reap = SANE_TRUE;
+	      poll_set_valid = SANE_FALSE; /* We will expect to drop a pidfd */
 	    }
+	  }
+        }
 
-	  handle_client (fd);
+      if (do_rebind)
-
+        {
-	  if (run_once == SANE_TRUE)
+	  DBG (DBG_WARN, "run_standalone: invalid fd in set, attempting to re-bind\n");
-	    break; /* We have handled the only connection we're going to handle */
+	  close_fds(1 << SANED_FD_LISTENER, -1);
+	  do_bindings ();
+	  poll_set_valid = SANE_FALSE;
 	}
 
-      if (run_once == SANE_TRUE)
+      if (do_reap)
-	break;
+	while (wait_child (-1, NULL, WNOHANG) > 0);
     }
 
-  for (i = 0, fdp = fds; i < nfds; i++, fdp++)
+  free(poll_set);
-    close (fdp->fd);
+  close_fds(-1, -1);
-
+  kill_children(SIGTERM);
-  free (fds);
+  while (numchildren > 0)
+    wait_child (-1, NULL, 0);
 }
 
 
@@ -3390,7 +3460,7 @@ run_inetd (char __sane_unused__ *sock)
 
           if (fd == -1)
       	    {
-              DBG (DBG_ERR, "run_inetd: duplicating fd failed: %s", strerror (errno));
+              DBG (DBG_ERR, "run_inetd: duplicating fd failed: %s\n", strerror (errno));
               return;
             }
         }
@@ -3400,7 +3470,7 @@ run_inetd (char __sane_unused__ *sock)
       dave_null = open ("/dev/null", O_RDWR);
       if (dave_null < 0)
         {
-          DBG (DBG_ERR, "run_inetd: could not open /dev/null: %s", strerror (errno));
+          DBG (DBG_ERR, "run_inetd: could not open /dev/null: %s\n", strerror (errno));
           return;
         }