Page MenuHome GnuPG
Create Task
Maniphest T7716

gpgrt:w32: Synchronous spawning detached process, with standard input and standard error
Open, WishlistPublic
Actions

Description

Currently, spawning a detached process, it assumes no input/output from/to its parent process.

To support synchronous invocation of gpg-agent for GnuPG, when input from detached process can be available, parent process could check if gpg-agent is ready for serving with the socket.

(1) GnuPG spawns gpg-agent process with pipe input+stderr
(2) gpg-agent can emit errors to stderr, emit "OK\n" when it's ready, or "ERR <ERRCODE>\n" to its stdout
(3) GnuPG checks pipe input to see gpg-agent is ready
(4) gpg-agent closes its stdout and stderr
(5) GnuPG closes pipe input

For now, I don't know if the scenario above works, let me try.

Related Objects
Search...

Event Timeline

• gniibe claimed this task.Wed, Jul 9, 6:43 AM
• gniibe triaged this task as Wishlist priority.
• gniibe created this task.
• gniibe added a project: gpgrt.
• gniibe updated the task description. (Show Details)Wed, Jul 9, 9:07 AM
• gniibe renamed this task from gpgrt:w32: Spawn detached process, with standard input and standard error to gpgrt:w32: Synchronous spawning detached process, with standard input and standard error.Fri, Jul 11, 10:39 AM
• gniibe added a comment.Fri, Jul 11, 10:50 AM

Here is an experimental change to support the feature.

It works for me.

diff --git a/src/spawn-w32.c b/src/spawn-w32.c
index 8ae589b..b190726 100644
--- a/src/spawn-w32.c
+++ b/src/spawn-w32.c
@@ -85,7 +85,8 @@ struct gpgrt_spawn_actions {
  * one for Unices.  */
 struct gpgrt_process {
   const char *pgmname;
-  unsigned int terminated:1;  /* or detached */
+  unsigned int terminated:1;
+  unsigned int detached:1;
   unsigned int flags;
   HANDLE hProcess;
   HANDLE hd_in;
@@ -213,15 +214,26 @@ create_inheritable_pipe (HANDLE filedes[2], int flags)
 static HANDLE
 w32_open_null (int for_write)
 {
+#if 0
   HANDLE hfile;
+  SECURITY_ATTRIBUTES sec_attr;
+
+  /* Prepare security attributes.  */
+  memset (&sec_attr, 0, sizeof sec_attr );
+  sec_attr.nLength = sizeof sec_attr;
+  sec_attr.bInheritHandle = TRUE;
 
   hfile = CreateFileW (L"nul",
                        for_write? GENERIC_WRITE : GENERIC_READ,
                        FILE_SHARE_READ | FILE_SHARE_WRITE,
-                       NULL, OPEN_EXISTING, 0, NULL);
+                       &sec_attr, OPEN_EXISTING, 0, NULL);
   if (hfile == INVALID_HANDLE_VALUE)
     _gpgrt_log_debug ("can't open 'nul': ec=%d\n", (int)GetLastError ());
   return hfile;
+#else
+  (void)for_write;
+  return INVALID_HANDLE_VALUE;
+#endif
 }
 
 
@@ -475,180 +487,6 @@ prepare_env_block (char **r_env, const char *const *envchange)
   return ec;
 }
 
-static gpg_err_code_t
-spawn_detached (const char *pgmname, char *cmdline, gpgrt_spawn_actions_t act)
-{
-  SECURITY_ATTRIBUTES sec_attr;
-  PROCESS_INFORMATION pi = { NULL, 0, 0, 0 };
-  STARTUPINFOEXW si;
-  int cr_flags;
-  wchar_t *wcmdline = NULL;
-  wchar_t *wpgmname = NULL;
-  gpg_err_code_t ec;
-  int ret;
-  BOOL ask_inherit = FALSE;
-  int i;
-  char *env = NULL;
-
-  ec = _gpgrt_access (pgmname, X_OK);
-  if (ec)
-    {
-      xfree (cmdline);
-      return ec;
-    }
-
-  memset (&si, 0, sizeof si);
-
-  i = 0;
-  if (act->hd[0] != INVALID_HANDLE_VALUE)
-    i++;
-  if (act->hd[1] != INVALID_HANDLE_VALUE)
-    i++;
-  if (act->hd[2] != INVALID_HANDLE_VALUE)
-    i++;
-
-  if (i != 0 || act->inherit_hds)
-    {
-      SIZE_T attr_list_size = 0;
-      HANDLE hd[32];
-      HANDLE *hd_p = act->inherit_hds;
-      int j = 0;
-
-      if (act->hd[0] != INVALID_HANDLE_VALUE)
-        hd[j++] = act->hd[0];
-      if (act->hd[1] != INVALID_HANDLE_VALUE)
-        hd[j++] = act->hd[1];
-      if (act->hd[2] != INVALID_HANDLE_VALUE)
-        hd[j++] = act->hd[2];
-      if (hd_p)
-        {
-          while (*hd_p != INVALID_HANDLE_VALUE)
-            if (j < DIM (hd))
-              hd[j++] = *hd_p++;
-            else
-              {
-                _gpgrt_log_info ("gpgrt_spawn_detached: too many handles\n");
-                break;
-              }
-        }
-
-      if (j)
-        {
-          if (check_windows_version ())
-            {
-              InitializeProcThreadAttributeList (NULL, 1, 0, &attr_list_size);
-              si.lpAttributeList = xtrymalloc (attr_list_size);
-              if (si.lpAttributeList == NULL)
-                {
-                  xfree (cmdline);
-                  return _gpg_err_code_from_syserror ();
-                }
-              InitializeProcThreadAttributeList (si.lpAttributeList, 1, 0,
-                                                 &attr_list_size);
-              UpdateProcThreadAttribute (si.lpAttributeList, 0,
-                                         PROC_THREAD_ATTRIBUTE_HANDLE_LIST,
-                                         hd, sizeof (HANDLE) * j, NULL, NULL);
-            }
-          ask_inherit = TRUE;
-        }
-    }
-
-  /* Prepare security attributes.  */
-  memset (&sec_attr, 0, sizeof sec_attr );
-  sec_attr.nLength = sizeof sec_attr;
-  sec_attr.bInheritHandle = FALSE;
-
-  /* Start the process.  */
-  si.StartupInfo.cb = sizeof (si);
-  si.StartupInfo.dwFlags = ((i > 0 ? STARTF_USESTDHANDLES : 0)
-                            | STARTF_USESHOWWINDOW);
-  si.StartupInfo.wShowWindow = DEBUG_W32_SPAWN? SW_SHOW : SW_MINIMIZE;
-  si.StartupInfo.hStdInput  = act->hd[0];
-  si.StartupInfo.hStdOutput = act->hd[1];
-  si.StartupInfo.hStdError  = act->hd[2];
-
-  cr_flags = (CREATE_DEFAULT_ERROR_MODE
-              | GetPriorityClass (GetCurrentProcess ())
-              | CREATE_NEW_PROCESS_GROUP
-              | DETACHED_PROCESS);
-
-  if (act->env)
-    {
-      /* Either ENV or ENVCHANGE can be specified, not both.  */
-      if (act->envchange)
-        {
-          xfree (cmdline);
-          return GPG_ERR_INV_ARG;
-        }
-
-      env = act->env;
-    }
-  else if (act->envchange)
-    {
-      ec = prepare_env_block (&env, act->envchange);
-      if (ec)
-        {
-          xfree (cmdline);
-          return ec;
-        }
-
-      cr_flags |= CREATE_UNICODE_ENVIRONMENT;
-    }
-
-  /* Take care: CreateProcessW may modify wpgmname */
-  if (!(wpgmname = _gpgrt_utf8_to_wchar (pgmname)))
-    ret = 0;
-  else if (!(wcmdline = _gpgrt_utf8_to_wchar (cmdline)))
-    ret = 0;
-  else
-    ret = CreateProcessW (wpgmname,      /* Program to start.  */
-                          wcmdline,      /* Command line arguments.  */
-                          &sec_attr,     /* Process security attributes.  */
-                          &sec_attr,     /* Thread security attributes.  */
-                          ask_inherit,   /* Inherit handles.  */
-                          cr_flags,      /* Creation flags.  */
-                          env,           /* Environment.  */
-                          NULL,          /* Use current drive/directory.  */
-                          (STARTUPINFOW *)&si,    /* Startup information. */
-                          &pi            /* Returns process information.  */
-                          );
-  if (act->envchange)
-    xfree (env);
-  env = NULL;
-  if (si.lpAttributeList)
-    DeleteProcThreadAttributeList (si.lpAttributeList);
-  if (!ret)
-    {
-      if (!wpgmname || !wcmdline)
-        _gpgrt_log_info ("gpgrt_spawn_detached: "
-                         "CreateProcess failed (utf8_to_wchar): %s\n",
-                          strerror (errno));
-      else
-        _gpgrt_log_info ("gpgrt_spawn_detached: "
-                         "CreateProcess(detached) failed: %d\n",
-                          (int)GetLastError ());
-      _gpgrt_free_wchar (wpgmname);
-      _gpgrt_free_wchar (wcmdline);
-      xfree (cmdline);
-      return GPG_ERR_GENERAL;
-    }
-  _gpgrt_free_wchar (wpgmname);
-  _gpgrt_free_wchar (wcmdline);
-  xfree (cmdline);
-
-  /* log_debug ("CreateProcess(detached) ready: hProcess=%p hThread=%p" */
-  /*           " dwProcessID=%d dwThreadId=%d\n", */
-  /*           pi.hProcess, pi.hThread, */
-  /*          (int) pi.dwProcessId, (int) pi.dwThreadId); */
-
-  /* Note: AllowSetForegroundWindow doesn't make sense for background
-     process.  */
-
-  CloseHandle (pi.hThread);
-  CloseHandle (pi.hProcess);
-  return 0;
-}
-
 
 gpg_err_code_t
 _gpgrt_spawn_actions_new (gpgrt_spawn_actions_t *r_act)
@@ -747,24 +585,6 @@ _gpgrt_process_spawn (const char *pgmname, const char *argv[],
   if (ec)
     return ec;
 
-  if ((flags & GPGRT_PROCESS_DETACHED))
-    {
-      if ((flags & GPGRT_PROCESS_STDFDS_SETTING))
-        {
-          xfree (cmdline);
-          return GPG_ERR_INV_FLAG;
-        }
-
-      /* In detached case, it must be no R_PROCESS.  */
-      if (r_process || pgmname == NULL)
-        {
-          xfree (cmdline);
-          return GPG_ERR_INV_ARG;
-        }
-
-      return spawn_detached (pgmname, cmdline, act);
-    }
-
   if (r_process)
     *r_process = NULL;
 
@@ -783,6 +603,20 @@ _gpgrt_process_spawn (const char *pgmname, const char *argv[],
 
   process->pgmname = pgmname;
   process->flags = flags;
+  process->detached = 0;
+
+  if ((flags & GPGRT_PROCESS_DETACHED))
+    {
+      process->detached = 1;
+      ec = _gpgrt_access (pgmname, X_OK);
+      if (ec)
+        {
+          _gpgrt_log_info ("gpgrt_process_spawn: access failed %s\n",
+                           _gpg_strerror (ec));
+          xfree (cmdline);
+          return ec;
+        }
+    }
 
   if ((flags & GPGRT_PROCESS_STDINOUT_SOCKETPAIR))
     {
@@ -954,16 +788,20 @@ _gpgrt_process_spawn (const char *pgmname, const char *argv[],
   si.StartupInfo.cb = sizeof (si);
   si.StartupInfo.dwFlags = ((i > 0 ? STARTF_USESTDHANDLES : 0)
                             | STARTF_USESHOWWINDOW);
-  si.StartupInfo.wShowWindow = DEBUG_W32_SPAWN? SW_SHOW : SW_HIDE;
+  si.StartupInfo.wShowWindow = DEBUG_W32_SPAWN? SW_SHOW :
+    (process->detached? SW_MINIMIZE : SW_HIDE);
   si.StartupInfo.hStdInput  = act->hd[0];
   si.StartupInfo.hStdOutput = act->hd[1];
   si.StartupInfo.hStdError  = act->hd[2];
 
   /* log_debug ("CreateProcess, path='%s' cmdline='%s'\n", pgmname, cmdline); */
-  cr_flags = (CREATE_DEFAULT_ERROR_MODE
-              | ((flags & GPGRT_PROCESS_NO_CONSOLE) ? DETACHED_PROCESS : 0)
-              | GetPriorityClass (GetCurrentProcess ())
-              | CREATE_SUSPENDED);
+  cr_flags = (CREATE_DEFAULT_ERROR_MODE | EXTENDED_STARTUPINFO_PRESENT
+              | GetPriorityClass (GetCurrentProcess ()));
+  if (process->detached)
+    cr_flags |= (CREATE_NEW_PROCESS_GROUP | DETACHED_PROCESS);
+  else
+    cr_flags |= (((flags & GPGRT_PROCESS_NO_CONSOLE) ? DETACHED_PROCESS : 0)
+                 | CREATE_SUSPENDED);
 
   if (act->env)
     {
@@ -990,6 +828,7 @@ _gpgrt_process_spawn (const char *pgmname, const char *argv[],
       cr_flags |= CREATE_UNICODE_ENVIRONMENT;
     }
 
+  /* Take care: CreateProcessW may modify wpgmname */
   if (!(wpgmname = _gpgrt_utf8_to_wchar (pgmname)))
     ret = 0;
   else if (!(wcmdline = _gpgrt_utf8_to_wchar (cmdline)))
@@ -1070,11 +909,16 @@ _gpgrt_process_spawn (const char *pgmname, const char *argv[],
                          (int)GetLastError ());
     }
 
-  /* Process has been created suspended; resume it now. */
-  _gpgrt_pre_syscall ();
-  ResumeThread (pi.hThread);
-  CloseHandle (pi.hThread);
-  _gpgrt_post_syscall ();
+  if (process->detached)
+    CloseHandle (pi.hThread);
+  else
+    {
+      /* Process has been created suspended; resume it now. */
+      _gpgrt_pre_syscall ();
+      ResumeThread (pi.hThread);
+      CloseHandle (pi.hThread);
+      _gpgrt_post_syscall ();
+    }
 
   process->hProcess = pi.hProcess;
   process->hd_in = hd_in[1];
@@ -1085,9 +929,17 @@ _gpgrt_process_spawn (const char *pgmname, const char *argv[],
 
   if (r_process == NULL)
     {
-      ec = _gpgrt_process_wait (process, 1);
-      _gpgrt_process_release (process);
-      return ec;
+      if (process->detached)
+        {
+          _gpgrt_process_release (process);
+          return 0;
+        }
+      else
+        {
+          ec = _gpgrt_process_wait (process, 1);
+          _gpgrt_process_release (process);
+          return ec;
+        }
     }
 
   *r_process = process;
@@ -1326,12 +1178,19 @@ _gpgrt_process_release (gpgrt_process_t process)
   if (!process)
     return;
 
-  if (!process->terminated)
+  if (!process->terminated && !process->detached)
     {
       _gpgrt_process_terminate (process);
       _gpgrt_process_wait (process, 1);
     }
 
+  if (process->hd_in != INVALID_HANDLE_VALUE)
+    CloseHandle (process->hd_in);
+  if (process->hd_out != INVALID_HANDLE_VALUE)
+    CloseHandle (process->hd_out);
+  if (process->hd_err != INVALID_HANDLE_VALUE)
+    CloseHandle (process->hd_err);
+
   CloseHandle (process->hProcess);
   xfree (process);
 }
@@ -1344,7 +1203,7 @@ _gpgrt_process_wait_list (gpgrt_process_t *process_list, int count, int hang)
 
   for (i = 0; i < count; i++)
     {
-      if (process_list[i]->terminated)
+      if (process_list[i]->terminated || process_list[i]->detached)
         continue;
 
       ec = _gpgrt_process_wait (process_list[i], hang);