popen2 vs pclose

[John W. Eaton]

Octave's popen2 function will create zombie processes, even if you use
pclose to close streams returned by popen2.  One problem is that
popen2 does not set the close function for the stream to be pclose.
But just doing that is not enough since pclose does not know about
files opened with popen2.  It only knows about child processes that
are created with popen.

The attached diff shows a draft of a solution I came up with that I
think will fix the popen2 zombie problem.  The idea is to keep track
of the pid for the process created by popen2, then wait for it when
either the input or output stream is closed.  I'm not sure whether
this is the best thing to do.  Maybe there are reasons to close one
end of the connection to the process but not wait for it to exit?  If
so, then this fix will also cause some trouble.

Also, the two streams returned by popen2 are not associated with
one another and with this patch, both store the pid.  This means that
waitpid is called once for each stream.  That doesn't seem like the
best thing to do.

Maybe it is just best to expect users of the popen2 function to do
something like

  [in, out, pid] = popen2 (...);

  ...

  fclose (in);
  fclose (out);

  waitpid (pid);

and not try to do this automatically for them.  If so, then I think we
just need to document this behavior and make sure that this is done
properly for any code in Octave that uses popen2.

Comments or suggestions?

Thanks,

jwe

diff --git a/src/oct-prcstrm.cc b/src/oct-prcstrm.cc
--- a/src/oct-prcstrm.cc
+++ b/src/oct-prcstrm.cc
@@ -1,7 +1,7 @@
 /*
 
-Copyright (C) 1996, 1997, 1999, 2000, 2002, 2003, 2005, 2006, 2007
-              John W. Eaton
+Copyright (C) 1996, 1997, 1999, 2000, 2002, 2003, 2005, 2006, 2007,
+              2009 John W. Eaton
 
 This file is part of Octave.
 
@@ -25,8 +25,19 @@
 #include <config.h>
 #endif
 
+#include <cerrno>
 #include <cstdio>
 
+#ifdef HAVE_UNISTD_H
+#ifdef HAVE_SYS_TYPES_H
+#include <sys/types.h>
+#endif
+#include <unistd.h>
+#endif
+
+#include "oct-syscalls.h"
+#include "syswait.h"
+
 #include "oct-prcstrm.h"
 #include "sysdep.h"
 
@@ -37,11 +48,29 @@
   return octave_stream (new octave_iprocstream (n, arg_md, ff));
 }
 
+// We don't set the close function to be octave_pclose because these
+// streams are assumed to NOT be created by octave_popen.  See Fpopen2.
+octave_stream
+octave_iprocstream::create (const std::string& n, FILE *f, pid_t arg_pid,
+                           std::ios::openmode arg_md,
+                           oct_mach_info::float_format ff)
+{
+  return octave_stream (new octave_iprocstream (n, f, arg_pid, arg_md, ff));
+}
+
 octave_iprocstream::octave_iprocstream (const std::string& n,
                                        std::ios::openmode arg_md,
                                        oct_mach_info::float_format ff)
   : octave_stdiostream (n, octave_popen (n.c_str (), "r"),
-                       arg_md, ff, octave_pclose)
+                       arg_md, ff, octave_pclose), proc_pid (0)
+{
+}
+
+octave_iprocstream::octave_iprocstream (const std::string& n,
+                                       FILE *f, pid_t arg_pid,
+                                       std::ios::openmode arg_md,
+                                       oct_mach_info::float_format ff)
+  : octave_stdiostream (n, f, arg_md, ff), proc_pid (arg_pid)
 {
 }
 
@@ -50,6 +79,30 @@
   do_close ();
 }
 
+void
+octave_iprocstream::do_close (void)
+{
+  octave_stdiostream::do_close ();
+
+#if defined (HAVE_SYS_WAIT_H)
+  if (proc_pid > 0)
+    {
+      using namespace std;
+
+      int wstatus;
+      pid_t wait_pid;
+
+      do
+       {
+         wait_pid = octave_syscalls::waitpid (proc_pid, &wstatus, 0);
+       }
+      while (wait_pid == -1 && errno == EINTR);
+
+      proc_pid = 0;
+    }
+#endif
+}
+
 octave_stream
 octave_oprocstream::create (const std::string& n, std::ios::openmode arg_md,
                            oct_mach_info::float_format ff)
@@ -57,11 +110,29 @@
   return octave_stream (new octave_oprocstream (n, arg_md, ff));
 }
 
+// We don't set the close function to be octave_pclose because these
+// streams are assumed to NOT be created by octave_popen.  See Fpopen2.
+octave_stream
+octave_oprocstream::create (const std::string& n, FILE *f, pid_t arg_pid,
+                           std::ios::openmode arg_md,
+                           oct_mach_info::float_format ff)
+{
+  return octave_stream (new octave_oprocstream (n, f, arg_pid, arg_md, ff));
+}
+
 octave_oprocstream::octave_oprocstream (const std::string& n,
                                        std::ios::openmode arg_md,
                                        oct_mach_info::float_format ff)
   : octave_stdiostream (n, octave_popen (n.c_str (), "w"),
-                       arg_md, ff, octave_pclose)
+                       arg_md, ff, octave_pclose), proc_pid (0)
+{
+}
+
+octave_oprocstream::octave_oprocstream (const std::string& n,
+                                       FILE *f, pid_t arg_pid,
+                                       std::ios::openmode arg_md,
+                                       oct_mach_info::float_format ff)
+  : octave_stdiostream (n, f, arg_md, ff), proc_pid (arg_pid)
 {
 }
 
@@ -70,6 +141,30 @@
   do_close ();
 }
 
+void
+octave_oprocstream::do_close (void)
+{
+  octave_stdiostream::do_close ();
+
+#if defined (HAVE_SYS_WAIT_H)
+  if (proc_pid > 0)
+    {
+      using namespace std;
+
+      int wstatus;
+      pid_t wait_pid;
+
+      do
+       {
+         wait_pid = octave_syscalls::waitpid (proc_pid, &wstatus, 0);
+       }
+      while (wait_pid == -1 && errno == EINTR);
+
+      proc_pid = 0;
+    }
+#endif
+}
+
 /*
 ;;; Local Variables: ***
 ;;; mode: C++ ***
diff --git a/src/oct-prcstrm.h b/src/oct-prcstrm.h
--- a/src/oct-prcstrm.h
+++ b/src/oct-prcstrm.h
@@ -1,7 +1,7 @@
 /*
 
-Copyright (C) 1996, 1997, 1999, 2000, 2003, 2004, 2005, 2006, 2007
-              John W. Eaton
+Copyright (C) 1996, 1997, 1999, 2000, 2003, 2004, 2005, 2006, 2007,
+              2009 John W. Eaton
 
 This file is part of Octave.
 
@@ -24,6 +24,11 @@
 #if !defined (octave_octave_procstream_h)
 #define octave_octave_procstream_h 1
 
+
+#ifdef HAVE_SYS_TYPES_H
+#include <sys/types.h>
+#endif
+
 #include "oct-stdstrm.h"
 
 // FIXME -- why don't these classes use iprocstream and
@@ -39,17 +44,32 @@
                      oct_mach_info::float_format flt_fmt
                        = oct_mach_info::native_float_format ());
 
+  octave_iprocstream (const std::string& n, FILE *f, pid_t arg_pid,
+                     std::ios::openmode arg_md = std::ios::in,
+                     oct_mach_info::float_format flt_fmt
+                       = oct_mach_info::native_float_format ());
+
   static octave_stream
   create (const std::string& n, std::ios::openmode arg_md = std::ios::in,
          oct_mach_info::float_format flt_fmt
            = oct_mach_info::native_float_format ());
 
+  static octave_stream
+  create (const std::string& n, FILE *f, pid_t arg_pid,
+         std::ios::openmode arg_md = std::ios::in,
+         oct_mach_info::float_format flt_fmt
+           = oct_mach_info::native_float_format ());
+
+  void do_close (void);
+
 protected:
 
   ~octave_iprocstream (void);
 
 private:
 
+  pid_t proc_pid;
+
   // No copying!
 
   octave_iprocstream (const octave_iprocstream&);
@@ -67,17 +87,32 @@
                      oct_mach_info::float_format flt_fmt
                        = oct_mach_info::native_float_format ());
 
+  octave_oprocstream (const std::string& n, FILE *f, pid_t arg_pid,
+                     std::ios::openmode arg_md = std::ios::out,
+                     oct_mach_info::float_format flt_fmt
+                       = oct_mach_info::native_float_format ());
+
   static octave_stream
   create (const std::string& n, std::ios::openmode arg_md = std::ios::out,
          oct_mach_info::float_format flt_fmt
            = oct_mach_info::native_float_format ());
 
+  static octave_stream
+  create (const std::string& n, FILE *f, pid_t arg_pid,
+         std::ios::openmode arg_md = std::ios::out,
+         oct_mach_info::float_format flt_fmt
+           = oct_mach_info::native_float_format ());
+
+  void do_close (void);
+
 protected:
 
   ~octave_oprocstream (void);
 
 private:
 
+  pid_t proc_pid;
+
   // No copying!
 
   octave_oprocstream (const octave_oprocstream&);
diff --git a/src/oct-procbuf.cc b/src/oct-procbuf.cc
--- a/src/oct-procbuf.cc
+++ b/src/oct-procbuf.cc
@@ -1,7 +1,7 @@
 /*
 
 Copyright (C) 1996, 1997, 1998, 1999, 2000, 2002, 2003, 2005, 2006
-              2007 John W. Eaton
+              2007, 2009 John W. Eaton
 
 This file is part of Octave.
 
@@ -40,12 +40,13 @@
 #include "lo-utils.h"
 #include "oct-procbuf.h"
 #include "oct-syscalls.h"
-#include "sysdep.h"
-#include "variables.h"
+#include "syswait.h"
 
 #include "defun.h"
 #include "gripes.h"
+#include "sysdep.h"
 #include "utils.h"
+#include "variables.h"
 
 // This class is based on the procbuf class from libg++, written by
 // Per Bothner, Copyright (C) 1993 Free Software Foundation.
diff --git a/src/syscalls.cc b/src/syscalls.cc
--- a/src/syscalls.cc
+++ b/src/syscalls.cc
@@ -55,6 +55,7 @@
 #include "lo-utils.h"
 #include "oct-map.h"
 #include "oct-obj.h"
+#include "oct-prcstrm.h"
 #include "oct-stdstrm.h"
 #include "oct-stream.h"
 #include "sysdep.h"
@@ -310,7 +311,10 @@
                   std::string msg;
                   pid_t pid;
 
-                  pid = octave_syscalls::popen2 (exec_file, arg_list, 
sync_mode, fildes, msg, interactive);
+                  pid = octave_syscalls::popen2 (exec_file, arg_list,
+                                                sync_mode, fildes, msg,
+                                                interactive);
+
                   if (pid >= 0)
                     {
                       FILE *ifile = fdopen (fildes[1], "r");
@@ -318,19 +322,24 @@
 
                       std::string nm;
 
-                      octave_stream is = octave_stdiostream::create (nm, ifile,
-                          std::ios::in);
+                     oct_mach_info::float_format ff
+                       = oct_mach_info::native_float_format ();
 
-                      octave_stream os = octave_stdiostream::create (nm, ofile,
-                          std::ios::out);
+                      octave_stream is
+                       = octave_iprocstream::create (nm, ifile, pid,
+                                                     std::ios::in, ff);
+
+                      octave_stream os
+                       = octave_oprocstream::create (nm, ofile, pid,
+                                                     std::ios::out, ff);
 
                       Cell file_ids (1, 2);
 
                       retval(0) = octave_stream_list::insert (os);
                       retval(1) = octave_stream_list::insert (is);
-                                         retval(2) = pid;
+                     retval(2) = pid;
                     }
-                                 else
+                 else
                     error (msg.c_str ());
                 }
             }