2002-08-28 Marcus Brinkmann <marcus@g10code.de>

* posix-io.c (_gpgme_io_spawn): Use a double-fork approach.
	Return 0 on success, -1 on error.
	* version.c (_gpgme_get_program_version): Don't wait for the child.
	* engine.c (_gpgme_engine_housecleaning): Function removed.
	(do_reaping): Likewise.
	(_gpgme_engine_add_child_to_reap_list): Likewise.
	(struct reap_s): Removed.
	(reap_list): Likewise.
	(reap_list_lock): Likewise.
	* engine.h (_gpgme_engine_io_event): Remove prototypes for
	_gpgme_engine_housecleaning and
	_gpgme_engine_add_child_to_reap_list.
	* rungpg.c (_gpgme_gpg_release): Don't add child to reap list.
	(struct gpg_object_s): Remove PID member.
	(_gpgme_gpg_new): Don't initialize GPG->pid.
	(_gpgme_gpg_spawn): Don't set GPG->pid.
	* wait.c (run_idle): Removed.
	(gpgme_wait): Run idle_function directly.

gpgme/ChangeLog

@@ -1,3 +1,24 @@
+2002-08-28  Marcus Brinkmann  <marcus@g10code.de>
+
+	* posix-io.c (_gpgme_io_spawn): Use a double-fork approach.
+	Return 0 on success, -1 on error.
+	* version.c (_gpgme_get_program_version): Don't wait for the child.
+	* engine.c (_gpgme_engine_housecleaning): Function removed.
+	(do_reaping): Likewise.
+	(_gpgme_engine_add_child_to_reap_list): Likewise.
+	(struct reap_s): Removed.
+	(reap_list): Likewise.
+	(reap_list_lock): Likewise.
+	* engine.h (_gpgme_engine_io_event): Remove prototypes for
+	_gpgme_engine_housecleaning and
+	_gpgme_engine_add_child_to_reap_list.
+	* rungpg.c (_gpgme_gpg_release): Don't add child to reap list.
+	(struct gpg_object_s): Remove PID member.
+	(_gpgme_gpg_new): Don't initialize GPG->pid.
+	(_gpgme_gpg_spawn): Don't set GPG->pid.
+	* wait.c (run_idle): Removed.
+	(gpgme_wait): Run idle_function directly.
+
 2002-08-21  Marcus Brinkmann  <marcus@g10code.de>
 
 	* encrypt-sign.c (encrypt_sign_status_handler): Remove dead

gpgme/engine.c

@@ -52,18 +52,6 @@ struct engine_object_s
 };
 
 
-struct reap_s
-{
-  struct reap_s *next;
-  int pid;
-  time_t entered;
-  int term_send;
-};
-
-static struct reap_s *reap_list;
-DEFINE_STATIC_LOCK (reap_list_lock);
-
-
 /* Get the path of the engine for PROTOCOL.  */
 const char *
 _gpgme_engine_get_path (GpgmeProtocol proto)
@@ -627,85 +615,3 @@ _gpgme_engine_io_event (EngineObject engine,
       break;
     }
 }
-
-
-void
-_gpgme_engine_add_child_to_reap_list (void *buf, int buflen, pid_t pid)
-{
-  /* Reuse the memory, so that we don't need to allocate another
-     memory block and to handle errors.  */
-  struct reap_s *child = buf;
-
-  assert (buflen >= sizeof *child);
-  memset (child, 0, sizeof *child);
-  child->pid = pid;
-  child->entered = time (NULL);
-  LOCK (reap_list_lock);
-  child->next = reap_list;
-  reap_list = child;
-  UNLOCK (reap_list_lock);
-}
-
-static void
-do_reaping (void)
-{
-  struct reap_s *r, *rlast;
-  static time_t last_check;
-  time_t cur_time = time (NULL);
-
-  /* A race does not matter here.  */
-  if (!last_check)
-    last_check = time (NULL);
-
-  if (last_check >= cur_time)
-    return;  /* We check only every second.  */
-
-  /* Fixme: it would be nice if to have a TRYLOCK here.  */
-  LOCK (reap_list_lock);
-  for (r = reap_list, rlast = NULL; r; rlast = r, r = r ? r->next : NULL)
-    {
-      int dummy1, dummy2;
-
-      if (_gpgme_io_waitpid (r->pid, 0, &dummy1, &dummy2))
-	{
-	  /* The process has terminated - remove it from the queue.  */
-	  void *p = r;
-	  if (!rlast)
-	    {
-	      reap_list = r->next;
-	      r = reap_list;
-            }
-	  else
-	    {
-	      rlast->next = r->next;
-	      r = rlast;
-            }
-	  xfree (p);
-        }
-      else if (!r->term_send)
-	{
-	  if (r->entered + 1 >= cur_time)
-	    {
-	      _gpgme_io_kill (r->pid, 0);
-	      r->term_send = 1;
-	      r->entered = cur_time;
-            }
-        }
-      else
-	{
-	  /* Give it 5 second before we are going to send the killer.  */
-	  if (r->entered + 5 >= cur_time)
-	    {
-	      _gpgme_io_kill (r->pid, 1);
-	      r->entered = cur_time; /* Just in case we have to repeat it.  */
-            }
-        }
-    }
-  UNLOCK (reap_list_lock);  
-}
-
-void
-_gpgme_engine_housecleaning (void)
-{
-  do_reaping ();
-}

gpgme/engine.h

@@ -84,7 +84,4 @@ void _gpgme_engine_set_io_cbs (EngineObject engine,
 void _gpgme_engine_io_event (EngineObject engine,
 			     GpgmeEventIO type, void *type_data);
 
-void _gpgme_engine_add_child_to_reap_list (void *buf, int buflen, pid_t pid);
-void _gpgme_engine_housecleaning (void);
-
 #endif /* ENGINE_H */

gpgme/posix-io.c

@@ -146,6 +146,7 @@ _gpgme_io_set_nonblocking (int fd)
 }
 
 
+/* Returns 0 on success, -1 on error.  */
 int
 _gpgme_io_spawn (const char *path, char **argv,
 		 struct spawn_fd_item_s *fd_child_list,
@@ -155,6 +156,7 @@ _gpgme_io_spawn (const char *path, char **argv,
   DEFINE_STATIC_LOCK (fixed_signals_lock);
   pid_t pid;
   int i;
+  int status, signo;
 
   LOCK (fixed_signals_lock);
   if (!fixed_signals)
@@ -179,73 +181,85 @@ _gpgme_io_spawn (const char *path, char **argv,
 
   if (!pid)
     {
-      /* Child.  */
+      /* Intermediate child to prevent zombie processes.  */
-      int duped_stdin = 0;
+      if ((pid = fork ()) == 0)
-      int duped_stderr = 0;
-
-      /* First close all fds which will not be duped.  */
-      for (i=0; fd_child_list[i].fd != -1; i++)
-	if (fd_child_list[i].dup_to == -1)
-	  close (fd_child_list[i].fd);
-
-      /* And now dup and close the rest.  */
-      for (i=0; fd_child_list[i].fd != -1; i++)
 	{
-	  if (fd_child_list[i].dup_to != -1)
+	  /* Child.  */
-	    {
+	  int duped_stdin = 0;
-	      if (dup2 (fd_child_list[i].fd,
+	  int duped_stderr = 0;
-			 fd_child_list[i].dup_to) == -1)
+
-		{
+	  /* First close all fds which will not be duped.  */
-		  DEBUG1 ("dup2 failed in child: %s\n", strerror (errno));
+	  for (i=0; fd_child_list[i].fd != -1; i++)
-		  _exit (8);
+	    if (fd_child_list[i].dup_to == -1)
-                }
-	      if (fd_child_list[i].dup_to == 0)
-		duped_stdin=1;
-	      if (fd_child_list[i].dup_to == 2)
-		duped_stderr=1;
 	      close (fd_child_list[i].fd);
-            }
-        }
 
-      if (!duped_stdin || !duped_stderr)
+	  /* And now dup and close the rest.  */
-	{
+	  for (i=0; fd_child_list[i].fd != -1; i++)
-	  int fd = open ("/dev/null", O_RDWR);
-	  if (fd == -1)
 	    {
-	      DEBUG1 ("can't open `/dev/null': %s\n", strerror (errno));
+	      if (fd_child_list[i].dup_to != -1)
-	      _exit (8);
-            }
-	  /* Make sure that the process has a connected stdin.  */
-	  if (!duped_stdin)
-	    {
-	      if (dup2 (fd, 0) == -1)
 		{
-		  DEBUG1("dup2(/dev/null, 0) failed: %s\n",
+		  if (dup2 (fd_child_list[i].fd,
-			 strerror (errno));
+			    fd_child_list[i].dup_to) == -1)
+		    {
+		      DEBUG1 ("dup2 failed in child: %s\n", strerror (errno));
+		      _exit (8);
+		    }
+		  if (fd_child_list[i].dup_to == 0)
+		    duped_stdin=1;
+		  if (fd_child_list[i].dup_to == 2)
+		    duped_stderr=1;
+		  close (fd_child_list[i].fd);
+		}
+	    }
+	  
+	  if (!duped_stdin || !duped_stderr)
+	    {
+	      int fd = open ("/dev/null", O_RDWR);
+	      if (fd == -1)
+		{
+		  DEBUG1 ("can't open `/dev/null': %s\n", strerror (errno));
 		  _exit (8);
-                }
+		}
-            }
+	      /* Make sure that the process has a connected stdin.  */
-	  if (!duped_stderr)
+	      if (!duped_stdin)
-	    if (dup2 (fd, 2) == -1)
+		{
-	      {
+		  if (dup2 (fd, 0) == -1)
-		DEBUG1 ("dup2(dev/null, 2) failed: %s\n", strerror (errno));
+		    {
-		_exit (8);
+		      DEBUG1("dup2(/dev/null, 0) failed: %s\n",
-	      }
+			     strerror (errno));
-	  close (fd);
+		      _exit (8);
-	}
+		    }
+		}
+	      if (!duped_stderr)
+		if (dup2 (fd, 2) == -1)
+		  {
+		    DEBUG1 ("dup2(dev/null, 2) failed: %s\n", strerror (errno));
+		    _exit (8);
+		  }
+	      close (fd);
+	    }
     
-      execv ( path, argv );
+	  execv ( path, argv );
-      /* Hmm: in that case we could write a special status code to the
+	  /* Hmm: in that case we could write a special status code to the
-	 status-pipe.  */
+	     status-pipe.  */
-      DEBUG1 ("exec of `%s' failed\n", path);
+	  DEBUG1 ("exec of `%s' failed\n", path);
-      _exit (8);
+	  _exit (8);
-    } /* End child.  */
+	} /* End child.  */
+      if (pid == -1)
+	_exit (1);
+      else
+	_exit (0);
+    }
     
+  _gpgme_io_waitpid (pid, 1, &status, &signo);
+  if (status)
+    return -1;
+
   /* .dup_to is not used in the parent list.  */
-  for (i=0; fd_parent_list[i].fd != -1; i++)
+  for (i = 0; fd_parent_list[i].fd != -1; i++)
     close (fd_parent_list[i].fd);
 
-  return (int) pid;
+  return 0;
 }
 
 

gpgme/rungpg.c

@@ -100,8 +100,6 @@ struct gpg_object_s
   char **argv;  
   struct fd_data_map_s *fd_data_map;
 
-  int pid; /* we can't use pid_t because we don't use it in Windoze */
-
   /* stuff needed for pipemode */
   struct
   {
@@ -264,8 +262,6 @@ _gpgme_gpg_new (GpgObject *r_gpg)
   gpg->cmd.linked_data = NULL;
   gpg->cmd.linked_idx = -1;
 
-  gpg->pid = -1;
-
   /* Allocate the read buffer for the status pipe.  */
   gpg->status.bufsize = 1024;
   gpg->status.readpos = 0;
@@ -345,10 +341,7 @@ _gpgme_gpg_release (GpgObject gpg)
   free_fd_data_map (gpg->fd_data_map);
   if (gpg->cmd.fd != -1)
     _gpgme_io_close (gpg->cmd.fd);
-  if (gpg->pid != -1)
+  xfree (gpg);
-    _gpgme_engine_add_child_to_reap_list (gpg, sizeof *gpg, gpg->pid);
-  else
-    xfree (gpg);
 }
 
 void
@@ -841,7 +834,7 @@ _gpgme_gpg_spawn (GpgObject gpg, void *opaque)
 {
   GpgmeError rc;
   int i, n;
-  int pid;
+  int status;
   struct spawn_fd_item_s *fd_child_list, *fd_parent_list;
 
   if (!gpg)
@@ -916,13 +909,12 @@ _gpgme_gpg_spawn (GpgObject gpg, void *opaque)
   fd_parent_list[n].fd = -1;
   fd_parent_list[n].dup_to = -1;
 
-  pid = _gpgme_io_spawn (_gpgme_get_gpg_path (),
+  status = _gpgme_io_spawn (_gpgme_get_gpg_path (),
-			 gpg->argv, fd_child_list, fd_parent_list);
+			    gpg->argv, fd_child_list, fd_parent_list);
   xfree (fd_child_list);
-  if (pid == -1)
+  if (status == -1)
     return mk_error (Exec_Error);
 
-  gpg->pid = pid;
   if (gpg->pm.used)
     gpg->pm.active = 1;
 

gpgme/version.c

@@ -219,12 +219,11 @@ _gpgme_get_program_version (const char *const path)
   int linelen = 0;
   char *mark = NULL;
   int rp[2];
-  pid_t pid;
   int nread;
   char *argv[] = {(char *) path, "--version", 0};
   struct spawn_fd_item_s pfd[] = { {0, -1}, {-1, -1} };
   struct spawn_fd_item_s cfd[] = { {-1, 1 /* STDOUT_FILENO */}, {-1, -1} };
-  int status, signal;
+  int status;
 
   if (!path)
     return NULL;
@@ -235,8 +234,8 @@ _gpgme_get_program_version (const char *const path)
   pfd[0].fd = rp[1];
   cfd[0].fd = rp[1];
 
-  pid = _gpgme_io_spawn (path, argv, cfd, pfd);
+  status = _gpgme_io_spawn (path, argv, cfd, pfd);
-  if (pid < 0)
+  if (status < 0)
     {
       _gpgme_io_close (rp[0]);
       _gpgme_io_close (rp[1]);
@@ -261,7 +260,6 @@ _gpgme_get_program_version (const char *const path)
   while (nread > 0 && linelen < LINELENGTH - 1);
 
   _gpgme_io_close (rp[0]);
-  _gpgme_io_waitpid (pid, 1, &status, &signal);
 
   if (mark)
     {

gpgme/wait.c

@@ -52,8 +52,6 @@ struct wait_item_s
   int dir;
 };
 
-static void run_idle (void);
-
 
 void
 _gpgme_fd_table_init (fd_table_t fdt)
@@ -133,14 +131,6 @@ gpgme_register_idle (GpgmeIdleFunc idle)
   return old_idle;
 }
 
-static void
-run_idle ()
-{
-  _gpgme_engine_housecleaning ();
-  if (idle_function)
-    idle_function ();
-}
-
 
 /* Wait on all file descriptors listed in FDT and process them using
    the registered callbacks.  Returns -1 on error (with errno set), 0
@@ -254,8 +244,8 @@ gpgme_wait (GpgmeCtx ctx, GpgmeError *status, int hang)
 	}
       UNLOCK (ctx_done_list_lock);
 
-      if (hang)
+      if (hang && idle_function)
-	run_idle ();
+	idle_function ();
     }
   while (hang && (!ctx || !ctx->cancel));