GNOME.org
 

[vte] spawn: Async-signal-safety fixes

commit ed78b9e2ec47675a9dccf045caca4d7a0b6c9fe8
Author: Christian Persch <chpe src gnome org>
Date:   Sat Jul 18 13:52:55 2020 +0200

    spawn: Async-signal-safety fixes
    
    Ported from https://gitlab.gnome.org/GNOME/glib/-/issues/2140
    with some adjustments.
    
    Fixes: https://gitlab.gnome.org/GNOME/vte/-/issues/263

 src/missing.cc  |  69 +++++++++++++++++++++++------
 src/spawn.cc    |  43 ++++++++++++++++--
 src/spawn.hh    |  11 ++---
 src/vtepty.cc   |   5 +--
 src/vtespawn.cc | 133 ++++++++++++++++++++++++++++----------------------------
 src/vtespawn.hh |   7 +--
 6 files changed, 173 insertions(+), 95 deletions(-)
---
diff --git a/src/missing.cc b/src/missing.cc
index 0ca1da6e..7729943d 100644
--- a/src/missing.cc
+++ b/src/missing.cc
@@ -51,6 +51,9 @@ struct linux_dirent64
   char           d_name[]; /* Filename (null-terminated) */
 };
 
+/* This function is called between fork and execve/_exit and so must be
+ * async-signal-safe; see man:signal-safety(7).
+ */
 static int
 filename_to_fd (const char *p)
 {
@@ -64,7 +67,7 @@ filename_to_fd (const char *p)
 
   while ((c = *p++) != '\0')
     {
-      if (!g_ascii_isdigit (c))
+      if (c < '0' || c > '9')
         return -1;
       c -= '0';
 
@@ -80,6 +83,56 @@ filename_to_fd (const char *p)
 
 #endif /* __linux__ */
 
+/* This function is called between fork and execve/_exit and so must be
+ * async-signal-safe; see man:signal-safety(7).
+ */
+static int
+getrlimit_NOFILE_max(void)
+{
+#ifdef HAVE_SYS_RESOURCE_H
+        struct rlimit rlim;
+
+#ifdef __linux__
+        if (prlimit(0 /* this PID */, RLIMIT_NOFILE, nullptr, &rlim) == 0 &&
+            rlim.rlim_max != RLIM_INFINITY)
+                return rlim.rlim_max;
+
+        /* fallback */
+#endif /* __linux__ */
+
+#ifdef __GLIBC__
+        /* Use getrlimit() function provided by the system if it is known to be
+         * async-signal safe.
+         *
+         * According to the glibc manual, getrlimit is AS-safe.
+         */
+        if (getrlimit(RLIMIT_NOFILE, &rlim) == 0 &&
+            rlim.rlim_max != RLIM_INFINITY)
+                return rlim.rlim_max;
+
+        /* fallback */
+#endif
+
+#endif /* HAVE_SYS_RESOURCE_H */
+
+#if defined(__FreeBSD__) || defined(__OpenBSD__)
+        /* Use sysconf() function provided by the system if it is known to be
+         * async-signal safe.
+         */
+        auto const r = sysconf(_SC_OPEN_MAX);
+        if (r != -1)
+                return r;
+
+        /* fallback */
+#endif
+
+        /* Hardcoded fallback: the default process hard limit in Linux as of 2020 */
+        return 4096;
+}
+
+/* This function is called between fork and execve/_exit and so must be
+ * async-signal-safe; see man:signal-safety(7).
+ */
 int
 fdwalk(int (*cb)(void *data, int fd),
        void *data)
@@ -88,14 +141,9 @@ fdwalk(int (*cb)(void *data, int fd),
    * may be slow on non-Linux operating systems, especially on systems allowing
    * very high number of open file descriptors.
    */
-  int open_max;
   int fd;
   int res = 0;
 
-#ifdef HAVE_SYS_RESOURCE_H
-  struct rlimit rl;
-#endif
-
 #ifdef __linux__
   /* Avoid use of opendir/closedir since these are not async-signal-safe. */
   int dir_fd = open ("/proc/self/fd", O_RDONLY | O_DIRECTORY | O_CLOEXEC);
@@ -129,14 +177,7 @@ fdwalk(int (*cb)(void *data, int fd),
 
 #endif
 
-#ifdef HAVE_SYS_RESOURCE_H
-
-  if (getrlimit(RLIMIT_NOFILE, &rl) == 0 && rl.rlim_max != RLIM_INFINITY)
-      open_max = rl.rlim_max;
-  else
-#endif
-      open_max = sysconf (_SC_OPEN_MAX);
-
+  auto const open_max = getrlimit_NOFILE_max();
   for (fd = 0; fd < open_max; fd++)
       if ((res = cb (data, fd)) != 0)
           break;
diff --git a/src/spawn.cc b/src/spawn.cc
index 354af8fe..34e7e332 100644
--- a/src/spawn.cc
+++ b/src/spawn.cc
@@ -278,8 +278,28 @@ SpawnContext::prepare_environ()
         m_envv = vte::glib::take_strv(merge_environ(m_envv.release(), m_cwd.get(), inherit_environ()));
 }
 
+char const*
+SpawnContext::search_path() const noexcept
+{
+        auto const path = m_search_path ? g_environ_getenv(environ(), "PATH") : nullptr;
+        return path ? : "/bin:/usr/bin:.";
+}
+
+size_t
+SpawnContext::workbuf_size() const noexcept
+{
+        auto const path = search_path();
+        return std::max(path ? strlen(path) + strlen(arg0()) + 2 /* leading '/' plus NUL terminator */ : 0,
+                        (g_strv_length(argv()) + 2) * sizeof(char*));
+}
+
+/* This function is called between fork and execve/_exit and so must be
+ * async-signal-safe; see man:signal-safety(7).
+ */
 SpawnContext::ExecError
-SpawnContext::exec(vte::libc::FD& child_report_error_pipe_write) noexcept
+SpawnContext::exec(vte::libc::FD& child_report_error_pipe_write,
+                   void* workbuf,
+                   size_t workbufsize) noexcept
 {
         /* NOTE! This function must not rely on smart pointers to
          * release their object, since the destructors are NOT run
@@ -449,7 +469,8 @@ SpawnContext::exec(vte::libc::FD& child_report_error_pipe_write) noexcept
                      argv(),
                      environ(),
                      search_path(),
-                     search_path_from_envp());
+                     workbuf,
+                     workbufsize);
 
         /* If we get here, exec failed */
         return ExecError::EXEC;
@@ -508,6 +529,18 @@ SpawnOperation::prepare(vte::glib::Error& error)
                        error))
                 return false;
 
+        /* allocate workbuf for SpawnContext::Exec() */
+        auto const workbufsize = context().workbuf_size();
+        auto workbuf = vte::glib::take_free_ptr(g_try_malloc(workbufsize));
+        if (!workbuf) {
+                auto errsv = vte::libc::ErrnoSaver{};
+                error.set(G_IO_ERROR,
+                          g_io_error_from_errno(errsv),
+                          "Failed to allocate workbuf: %s",
+                          g_strerror(errsv));
+                return false;
+        }
+
         /* Need to add the write end of the pipe to the FD map, so
          * that the FD re-arranging code knows it needs to preserve
          * the FD and not dup2 over it.
@@ -530,7 +563,11 @@ SpawnOperation::prepare(vte::glib::Error& error)
 
                 child_report_error_pipe_read.reset();
 
-                auto const err = context().exec(child_report_error_pipe_write);
+                auto const err = context().exec(child_report_error_pipe_write,
+                                                workbuf.get(), workbufsize);
+
+                /* Manually free the workbuf */
+                g_free(workbuf.release());
 
                 /* If we get here, exec failed. Write the error to the pipe and exit. */
                 _vte_write_err(child_report_error_pipe_write.get(), int(err));
diff --git a/src/spawn.hh b/src/spawn.hh
index 3e12af3b..5623050f 100644
--- a/src/spawn.hh
+++ b/src/spawn.hh
@@ -59,7 +59,6 @@ private:
         bool m_inherit_environ{true};
         bool m_systemd_scope{true};
         bool m_require_systemd_scope{false};
-        bool m_search_path_from_envp{false};
         bool m_search_path{false};
 
 public:
@@ -149,7 +148,6 @@ public:
         void set_no_systemd_scope()      noexcept { m_systemd_scope = false;        }
         void set_require_systemd_scope() noexcept { m_require_systemd_scope = true; }
         void set_search_path()           noexcept { m_search_path = true;           }
-        void set_search_path_from_envp() noexcept { m_search_path_from_envp = true; }
 
         auto arg0()         const noexcept { return m_arg0.get(); }
         auto argv()         const noexcept { return m_argv.get(); }
@@ -163,8 +161,9 @@ public:
         constexpr auto inherit_environ()       const noexcept { return m_inherit_environ;       }
         constexpr auto systemd_scope()         const noexcept { return m_systemd_scope;         }
         constexpr auto require_systemd_scope() const noexcept { return m_require_systemd_scope; }
-        constexpr auto search_path()           const noexcept { return m_search_path;           }
-        constexpr auto search_path_from_envp() const noexcept { return m_search_path_from_envp; }
+
+        char const* search_path() const noexcept;
+        size_t workbuf_size() const noexcept;
 
         void prepare_environ();
 
@@ -181,7 +180,9 @@ public:
                 UNSET_CLOEXEC,
         };
 
-        ExecError exec(vte::libc::FD& child_report_error_pipe_write) noexcept;
+        ExecError exec(vte::libc::FD& child_report_error_pipe_write,
+                       void* workbuf,
+                       size_t workbufsize) noexcept;
 
 }; // class SpawnContext
 
diff --git a/src/vtepty.cc b/src/vtepty.cc
index 230d5b63..4705ff9c 100644
--- a/src/vtepty.cc
+++ b/src/vtepty.cc
@@ -624,9 +624,8 @@ spawn_context_from_args(VtePty* pty,
         context.set_fallback_cwd(g_get_home_dir());
         context.set_child_setup(child_setup, child_setup_data, child_setup_data_destroy);
 
-        if (spawn_flags & G_SPAWN_SEARCH_PATH_FROM_ENVP)
-                context.set_search_path_from_envp();
-        if (spawn_flags & G_SPAWN_SEARCH_PATH)
+        if ((spawn_flags & G_SPAWN_SEARCH_PATH_FROM_ENVP) ||
+            (spawn_flags & G_SPAWN_SEARCH_PATH))
                 context.set_search_path();
 
         if (spawn_flags & G_SPAWN_FILE_AND_ARGV_ZERO)
diff --git a/src/vtespawn.cc b/src/vtespawn.cc
index cdedf0ac..9f01ac80 100644
--- a/src/vtespawn.cc
+++ b/src/vtespawn.cc
@@ -40,14 +40,19 @@
 
 #include "missing.hh"
 
-static gssize
-write_all (int fd, gconstpointer vbuf, gsize to_write)
+/* This function is called between fork and execve/_exit and so must be
+ * async-signal-safe; see man:signal-safety(7).
+ */
+static ssize_t
+write_all (int fd,
+           void const* vbuf,
+           size_t to_write) noexcept
 {
   char *buf = (char *) vbuf;
 
   while (to_write > 0)
     {
-      gssize count = write (fd, buf, to_write);
+      auto count = write(fd, buf, to_write);
       if (count < 0)
         {
           if (errno != EINTR)
@@ -63,9 +68,12 @@ write_all (int fd, gconstpointer vbuf, gsize to_write)
   return TRUE;
 }
 
+/* This function is called between fork and execve/_exit and so must be
+ * async-signal-safe; see man:signal-safety(7).
+ */
 void
 _vte_write_err (int fd,
-                int msg)
+                int msg) noexcept
 {
         int data[2] = {msg, errno};
 
@@ -74,46 +82,57 @@ _vte_write_err (int fd,
 
 /* Based on execvp from GNU C Library */
 
-static void
-script_execute (const char *file,
-                char      **argv,
-                char      **envp)
+/* This function is called between fork and execve/_exit and so must be
+ * async-signal-safe; see man:signal-safety(7).
+ */
+/* Returns false if failing before execv(e), or true if failing after it. */
+static bool
+script_execute (char const* file,
+                char** argv,
+                char** envp,
+                void* workbuf,
+                size_t workbufsize) noexcept
 {
   /* Count the arguments.  */
   int argc = 0;
   while (argv[argc])
     ++argc;
 
-  /* Construct an argument list for the shell.  */
-  {
-    char **new_argv;
-
-    new_argv = g_new0 (char*, argc + 2); /* /bin/sh and NULL */
+  auto argv_buffer = reinterpret_cast<char**>(workbuf);
+  auto argv_buffer_len = workbufsize / sizeof(char*);
+  /* Construct an argument list for the shell. */
+  if (size_t(argc + 2) > argv_buffer_len) {
+    errno = ENOMEM;
+    return false;
+  }
 
-    new_argv[0] = (char *) "/bin/sh";
-    new_argv[1] = (char *) file;
-    while (argc > 0)
-      {
-       new_argv[argc + 1] = argv[argc];
-       --argc;
-      }
+  argv_buffer[0] = (char *) "/bin/sh";
+  argv_buffer[1] = (char *) file;
+  while (argc > 0)
+    {
+      argv_buffer[argc + 1] = argv[argc];
+      --argc;
+    }
 
-    /* Execute the shell. */
-    if (envp)
-      execve (new_argv[0], new_argv, envp);
-    else
-      execv (new_argv[0], new_argv);
+  /* Execute the shell. */
+  if (envp)
+    execve (argv_buffer[0], argv_buffer, envp);
+  else
+    execv (argv_buffer[0], argv_buffer);
 
-    g_free (new_argv);
-  }
+  return true;
 }
 
+/* This function is called between fork and execve/_exit and so must be
+ * async-signal-safe; see man:signal-safety(7).
+ */
 int
 _vte_execute (const char *file,
               char      **argv,
               char      **envp,
-              bool        search_path,
-              bool        search_path_from_envp)
+              char const* search_path,
+              void* workbuf,
+              size_t workbufsize) noexcept
 {
   if (*file == '\0')
     {
@@ -122,7 +141,7 @@ _vte_execute (const char *file,
       return -1;
     }
 
-  if (!(search_path || search_path_from_envp) || strchr (file, '/') != NULL)
+  if (!search_path || strchr (file, '/') != nullptr)
     {
       /* Don't search when it contains a slash. */
       if (envp)
@@ -131,40 +150,23 @@ _vte_execute (const char *file,
         execv (file, argv);
 
       if (errno == ENOEXEC)
-       script_execute (file, argv, envp);
+         script_execute(file, argv, envp, workbuf, workbufsize);
     }
   else
     {
-      gboolean got_eacces = 0;
-      const char *path, *p;
-      char *name, *freeme;
-      gsize len;
-      gsize pathlen;
-
-      path = NULL;
-      if (search_path_from_envp)
-        path = g_environ_getenv (envp, "PATH");
-      if (search_path && path == NULL)
-        path = g_getenv ("PATH");
-
-      if (path == NULL)
-       {
-         /* There is no 'PATH' in the environment.  The default
-          * search path in libc is the current directory followed by
-          * the path 'confstr' returns for '_CS_PATH'.
-           */
+      auto got_eacces = false;
+      char const* path = search_path;
+      char const* p;
+      char* name = reinterpret_cast<char*>(workbuf);
 
-          /* In GLib we put . last, for security, and don't use the
-           * unportable confstr(); UNIX98 does not actually specify
-           * what to search if PATH is unset. POSIX may, dunno.
-           */
-
-          path = "/bin:/usr/bin:.";
-       }
+      auto const len = strlen(file) + 1;
+      auto const pathlen = strlen(path);
 
-      len = strlen (file) + 1;
-      pathlen = strlen (path);
-      freeme = name = (char*)g_malloc (pathlen + len + 1);
+      if (workbufsize < pathlen + len + 1)
+        {
+          errno = ENOMEM;
+          return -1;
+        }
 
       /* Copy the file name at the top, including '\0'  */
       memcpy (name + pathlen + 1, file, len);
@@ -186,7 +188,7 @@ _vte_execute (const char *file,
              */
            startp = name + 1;
          else
-            startp = (char*)memcpy (name - (p - path), path, p - path);
+            startp = reinterpret_cast<char*>(memcpy (name - (p - path), path, p - path));
 
          /* Try to execute this name.  If it works, execv will not return.  */
           if (envp)
@@ -194,8 +196,9 @@ _vte_execute (const char *file,
           else
             execv (startp, argv);
 
-         if (errno == ENOEXEC)
-           script_execute (startp, argv, envp);
+          if (errno == ENOEXEC &&
+              !script_execute(startp, argv, envp, workbuf, workbufsize))
+            return -1;
 
          switch (errno)
            {
@@ -206,8 +209,7 @@ _vte_execute (const char *file,
                */
              got_eacces = TRUE;
 
-              /* FALL THRU */
-
+              [[fallthrough]];
            case ENOENT:
 #ifdef ESTALE
            case ESTALE:
@@ -234,7 +236,6 @@ _vte_execute (const char *file,
                * something went wrong executing it; return the error to our
                * caller.
                */
-              g_free (freeme);
              return -1;
            }
        }
@@ -246,8 +247,6 @@ _vte_execute (const char *file,
          * error.
          */
         errno = EACCES;
-
-      g_free (freeme);
     }
 
   /* Return the error from the last attempt (probably ENOENT).  */
diff --git a/src/vtespawn.hh b/src/vtespawn.hh
index 481929cf..5398385d 100644
--- a/src/vtespawn.hh
+++ b/src/vtespawn.hh
@@ -25,8 +25,9 @@
 int _vte_execute(char const* file,
                  char** argv,
                  char** envp,
-                 bool search_path,
-                 bool search_path_from_envp);
+                 char const* path_env,
+                 void* workbuf,
+                 size_t workbufsize) noexcept;
 
 void _vte_write_err (int fd,
-                     int msg);
+                     int msg) noexcept;
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]