Implemented support for posix_spawn() and (main change): always use vfork() for starting external commands

src/utils/Makefile

@@ -6,7 +6,7 @@ include $(depth)/mk/sysconf
 # are part of the official distrib anyway
 LIBRECOLL = ../lib/librecoll.a
 
-PROGS = pxattr trecrontab \
+PROGS = pxattr trclosefrom trecrontab \
       trnetcon trcopyfile trcircache trmd5 trreadfile trfileudi \
       trconftree wipedir smallut  trfstreewalk trpathut transcode trbase64 \
       trmimeparse trexecmd utf8iter idfile
@@ -27,6 +27,14 @@ trecrontab.o : ecrontab.cpp ecrontab.h
 	$(CXX) -o trecrontab.o -c $(ALL_CXXFLAGS) \
 	       -DTEST_ECRONTAB ecrontab.cpp
 
+CLOSEFROM_OBJS= trclosefrom.o 
+trclosefrom : $(CLOSEFROM_OBJS)
+	$(CXX) -o trclosefrom $(CLOSEFROM_OBJS) \
+            $(LIBRECOLL) $(LIBICONV) $(LIBSYS)
+trclosefrom.o : closefrom.cpp closefrom.h
+	$(CXX) -o trclosefrom.o -c $(ALL_CXXFLAGS) \
+	       -DTEST_CLOSEFROM closefrom.cpp
+
 FSTREEWALK_OBJS= trfstreewalk.o 
 trfstreewalk : $(FSTREEWALK_OBJS)
 	$(CXX) -o trfstreewalk $(FSTREEWALK_OBJS) \

src/utils/closefrom.cpp

@@ -50,7 +50,18 @@
  *     descriptors. Only descriptors 0, 1, 2 are shown except if
  *     fdescfs is mounted which it is not by default
  *
+ *  Solaris:
+ *   - Solaris 10+ has closefrom, and can specify closefrom to posix_spawn()
+ *
+ *  Linux:
+ *   - Has nothing. The method we used (opening /dev/fd) was very
+ *     unsafe in multithread fork/exec context. We now use a close()
+ *     loop. glibc maintainers think that closefrom() is a bad idea
+ *     *especially* because it is implemented on *BSD and Solaris. Go
+ *     figure...: https://sourceware.org/bugzilla/show_bug.cgi?id=10353
+ *
  * Interface:
+ *
  * int libclf_closefrom(fd)
  *  @param fd All open file descriptors with equal or higher numeric 
  *       values will be closed. fd needs not be a valid descriptor.
@@ -62,8 +73,12 @@
 #include <fcntl.h>
 #include <string.h>
 #include <sys/param.h>
+#include <sys/time.h>
+#include <sys/resource.h>
 
-/* #define DEBUG_CLOSEFROM*/
+#include "closefrom.h"
+
+/* #define DEBUG_CLOSEFROM */
 #ifdef DEBUG_CLOSEFROM
 #define DPRINT(X) fprintf X
 #else
@@ -110,7 +125,12 @@ int libclf_closefrom(int fd0)
 }
 
 /*************************************************************************/
-#elif (defined(linux) || defined(__linux))
+#elif 0 && (defined(linux) || defined(__linux))
+
+/* We don't do this on linux anymore because opendir() may call
+   malloc which is unsafe in the [fork-exec] interval for a
+   multithreaded program. Linux does not have a good solution for
+   implementing closefrom as far as I know */
 
 /* Use /proc/self/fd directory */
 #include <sys/types.h>
@@ -142,11 +162,16 @@ int libclf_closefrom(int fd0)
 
 /*************************************************************************/
 #else 
-/* System has no native support for this functionality whatsoever.
+
+/* System has no native support for this functionality.
  *
  * Close all descriptors up to compiled/configured maximum.
  * The caller will usually have an idea of a reasonable maximum, else
  * we retrieve a value from the system.
+ *
+ * Note that there is actually no real guarantee that no open
+ * descriptor higher than the reported limit can exist, as noted by
+ * the Solaris man page for closefrom()
  */
 
 static int closefrom_maxfd = -1;
@@ -156,21 +181,18 @@ void libclf_setmaxfd(int max)
     closefrom_maxfd = max;
 }
 
-#ifdef sun
 #include <limits.h>
+
+#ifndef OPEN_MAX
+#define OPEN_MAX 1024
 #endif
+
 int libclf_closefrom(int fd0)
 {
     int i, maxfd = closefrom_maxfd;
 
     if (maxfd < 0) {
-#ifdef _SC_OPEN_MAX
-        maxfd = sysconf(_SC_OPEN_MAX);
-        DPRINT((stderr, "Maxfd is %d after sysconf()\n", maxfd));
-#else
-        maxfd = getdtablesize();
-        DPRINT((stderr, "Maxfd is %d after getdtablesize()\n", maxfd));
-#endif 
+        maxfd = libclf_maxfd();
     }
     if (maxfd < 0)
 	maxfd = OPEN_MAX;
@@ -184,6 +206,12 @@ int libclf_closefrom(int fd0)
 }
 #endif
 
+int libclf_maxfd(int)
+{
+    struct rlimit lim;
+    getrlimit(RLIMIT_NOFILE, &lim);
+    return int(lim.rlim_cur);
+}
 
 #else /* TEST_CLOSEFROM */
 

src/utils/closefrom.h

@@ -20,4 +20,8 @@
 /* Close all descriptors >=  fd */
 extern int libclf_closefrom(int fd);
 
+/* Retrieve max open fd. This might be the actual max open one (not
+   thread-safe) or a system max value. */
+extern int libclf_maxfd(int flags=0);
+
 #endif /* _closefrom_h_included_ */

src/utils/execmd.cpp

@@ -15,7 +15,11 @@
  *   59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
  */
 #ifndef TEST_EXECMD
+#ifdef RECOLL_DATADIR
+#include "autoconfig.h"
+#else
 #include "config.h"
+#endif
 
 #include <stdio.h>
 #include <stdlib.h>
@@ -31,6 +35,16 @@
 
 #include <vector>
 #include <string>
+#ifdef HAVE_SPAWN_H
+#ifndef __USE_GNU
+#define __USE_GNU
+#define undef__USE_GNU
+#endif
+#include <spawn.h>
+#ifdef undef__USE_GNU
+#undef __USE_GNU
+#endif
+#endif
 
 #include "execmd.h"
 
@@ -150,7 +164,18 @@ bool ExecCmd::which(const string& cmd, string& exepath, const char* path)
     return false;
 }
 
-void  ExecCmd::putenv(const string &ea)
+void ExecCmd::useVfork(bool on)
+{
+    // Just in case: there are competent people who believe that the
+    // dynamic linker can sometimes deadlock if execve() is resolved
+    // inside the vfork/exec window. Make sure it's done now. If "/" is
+    // an executable file, we have a problem.
+    const char *argv[] = {"/", 0};
+    execve("/", (char *const *)argv, environ);
+    o_useVfork  = on;
+}
+
+void ExecCmd::putenv(const string &ea)
 {
     m_env.push_back(ea);
 }
@@ -256,10 +281,17 @@ inline void ExecCmd::dochild(const string &cmd, const char **argv,
     }
 
     // Restore SIGTERM to default. Really, signal handling should be
-    // specified when creating the execmd. Help Recoll get rid of its
-    // filter children though. To be fixed one day... Not sure that
-    // all of this is needed. But an ignored sigterm and the masks are
-    // normally inherited.
+    // specified when creating the execmd, there might be other
+    // signals to reset. Resetting SIGTERM helps Recoll get rid of its
+    // filter children for now though. To be fixed one day...
+    // Note that resetting to SIG_DFL is a portable use of
+    // signal(). No need for sigaction() here.
+
+    // There is supposedely a risk of problems if another thread was
+    // calling a signal-affecting function when vfork was called. This
+    // seems acceptable though as no self-respecting thread is going
+    // to mess with the global process signal disposition.
+
     if (signal(SIGTERM, SIG_DFL) == SIG_ERR) {
 	//LOGERR(("ExecCmd::DOCHILD: signal() failed, errno %d\n", errno));
     }
@@ -307,9 +339,9 @@ inline void ExecCmd::dochild(const string &cmd, const char **argv,
     libclf_closefrom(3);
 
     execve(cmd.c_str(), (char *const*)argv, (char *const*)envv);
-    // Hu ho. This should never happened as we checked the existence of the
-    // executable before calling dochild... Until we did this, this was 
-    // the chief cause of LOG mutex deadlock
+    // Hu ho. This should never have happened as we checked the
+    // existence of the executable before calling dochild... Until we
+    // did this check, this was the chief cause of LOG mutex deadlock
     LOGERR(("ExecCmd::DOCHILD: execve(%s) failed. errno %d\n", cmd.c_str(),
 	    errno));
     _exit(127);
@@ -392,11 +424,85 @@ int ExecCmd::startExec(const string &cmd, const vector<string>& args,
         free(envv);
         return -1;
     }
-////////////////////////////////
+//////////////////////////////// End vfork child prepare section.
 
+#if HAVE_POSIX_SPAWN && USE_POSIX_SPAWN
+    {
+        posix_spawnattr_t attrs;
+        posix_spawnattr_init (&attrs);
+        short flags;
+        posix_spawnattr_getflags(&attrs, &flags);
+
+        flags |=  POSIX_SPAWN_USEVFORK;
+
+        posix_spawnattr_setpgroup(&attrs, 0);
+        flags |= POSIX_SPAWN_SETPGROUP;
+
+        sigset_t sset;
+        sigemptyset(&sset);
+        posix_spawnattr_setsigmask (&attrs, &sset);
+        flags |= POSIX_SPAWN_SETSIGMASK;
+
+        sigemptyset(&sset);
+        sigaddset(&sset, SIGTERM);
+        posix_spawnattr_setsigdefault(&attrs, &sset);
+        flags |= POSIX_SPAWN_SETSIGDEF;
+
+        posix_spawnattr_setflags(&attrs, flags);
+
+        posix_spawn_file_actions_t facts;
+        posix_spawn_file_actions_init(&facts);
+
+        if (has_input) {
+            posix_spawn_file_actions_addclose(&facts, m_pipein[1]);
+            if (m_pipein[0] != 0) {
+                posix_spawn_file_actions_adddup2(&facts, m_pipein[0], 0);
+                posix_spawn_file_actions_addclose(&facts, m_pipein[0]);
+            }
+        }
+        if (has_output) {
+            posix_spawn_file_actions_addclose(&facts, m_pipeout[0]);
+            if (m_pipeout[1] != 1) {
+                posix_spawn_file_actions_adddup2(&facts, m_pipeout[1], 1);
+                posix_spawn_file_actions_addclose(&facts, m_pipeout[1]);
+            }
+        }
+
+        // Do we need to redirect stderr ?
+        if (!m_stderrFile.empty()) {
+            int oflags = O_WRONLY|O_CREAT;
+#ifdef O_APPEND
+            oflags |= O_APPEND;
+#endif
+            posix_spawn_file_actions_addopen(&facts, 2, m_stderrFile.c_str(), 
+                                             oflags, 0600);
+        }
+        LOGDEB1(("using SPAWN\n"));
+
+        // posix_spawn() does not have any standard way to ask for
+        // calling closefrom(). Afaik there is a solaris extension for this,
+        // but let's just add all fds
+        for (int i = 3; i < libclf_maxfd(); i++) {
+            posix_spawn_file_actions_addclose(&facts, i);
+        }
+
+        int ret = posix_spawn(&m_pid, exe.c_str(), &facts, &attrs, 
+                              (char *const *)argv, (char *const *)envv);
+        posix_spawnattr_destroy(&attrs);
+        posix_spawn_file_actions_destroy(&facts);
+        if (ret) {
+            LOGERR(("ExecCmd::startExec: posix_spawn() failed. errno %d\n", 
+                    ret));
+            return -1;
+        }
+    }
+
+#else
     if (o_useVfork) {
+        LOGDEB1(("using VFORK\n"));
 	m_pid = vfork();
     } else {
+        LOGDEB1(("using FORK\n"));
 	m_pid = fork();
     }
     if (m_pid < 0) {
@@ -411,6 +517,7 @@ int ExecCmd::startExec(const string &cmd, const vector<string>& args,
 	// dochild does not return. Just in case...
 	_exit(1);
     }
+#endif
 
     // Father process
 

src/utils/execmd.h

@@ -70,12 +70,10 @@ class ExecCmdProvide {
  */
 class ExecCmd {
  public:
-    // Use vfork instead of fork. This must not be called in a multithreaded 
-    // program.
-    static void useVfork(bool on)
-    {
-	o_useVfork  = on;
-    }
+    // Use vfork instead of fork. Our vfork usage is multithread-compatible as
+    // far as I can see, but just in case...
+    static void useVfork(bool on);
+
     /** 
      * Add/replace environment variable before executing command. This must
      * be called before doexec() to have an effect (possibly multiple