Merge branch 'win32-spawn-escaping' into 'master'

gspawn, win32: qouted args - escape end backslash

See merge request GNOME/glib!419

glib/gspawn-win32-helper.c

@@ -92,22 +92,17 @@ protect_wargv (gint       argc,
       wchar_t *p = wargv[i];
       wchar_t *q;
       gint len = 0;
+      gint pre_bslash = 0;
       gboolean need_dblquotes = FALSE;
       while (*p)
 	{
 	  if (*p == ' ' || *p == '\t')
 	    need_dblquotes = TRUE;
-	  else if (*p == '"')
+	  /* estimate max len, assuming that all escapable chracters will be escaped */
-	    len++;
+	  if (*p == '"' || *p == '\\')
-	  else if (*p == '\\')
+	    len += 2;
-	    {
+	  else
-	      wchar_t *pp = p;
+	    len += 1;
-	      while (*pp && *pp == '\\')
-		pp++;
-	      if (*pp == '"')
-		len++;
-	    }
-	  len++;
 	  p++;
 	}
 
@@ -117,24 +112,40 @@ protect_wargv (gint       argc,
       if (need_dblquotes)
 	*q++ = '"';
 
+      /* Only quotes and backslashes preceeding quotes are escaped:
+       * see "Parsing C Command-Line Arguments" at
+       * https://docs.microsoft.com/en-us/cpp/c-language/parsing-c-command-line-arguments
+       */
       while (*p)
 	{
 	  if (*p == '"')
-	    *q++ = '\\';
-	  else if (*p == '\\')
 	    {
-	      wchar_t *pp = p;
+	      /* Add backslash for escaping quote itself */
-	      while (*pp && *pp == '\\')
+	      *q++ = '\\';
-		pp++;
+	      /* Add backslash for every preceeding backslash for escaping it */
-	      if (*pp == '"')
+	      for (;pre_bslash > 0; --pre_bslash)
 		*q++ = '\\';
 	    }
+
+	  /* Count length of continuous sequence of preceeding backslashes. */
+	  if (*p == '\\')
+	    ++pre_bslash;
+	  else
+	    pre_bslash = 0;
+
 	  *q++ = *p;
 	  p++;
 	}
 
       if (need_dblquotes)
-	*q++ = '"';
+	{
+	  /* Add backslash for every preceeding backslash for escaping it,
+	   * do NOT escape quote itself.
+	   */
+	  for (;pre_bslash > 0; --pre_bslash)
+	    *q++ = '\\';
+	  *q++ = '"';
+	}
       *q++ = '\0';
     }
   (*new_wargv)[argc] = NULL;
@@ -179,6 +190,7 @@ main (int ignored_argc, char **ignored_argv)
 {
   int child_err_report_fd = -1;
   int helper_sync_fd = -1;
+  int saved_stderr_fd = -1;
   int i;
   int fd;
   int mode;
@@ -280,6 +292,7 @@ main (int ignored_argc, char **ignored_argv)
 	}
     }
 
+  saved_stderr_fd = reopen_noninherited (dup (2), _O_WRONLY);
   if (argv[ARG_STDERR][0] == '-')
     ; /* Nothing */
   else if (argv[ARG_STDERR][0] == 'z')
@@ -315,15 +328,15 @@ main (int ignored_argc, char **ignored_argv)
    */
   if (argv[ARG_CLOSE_DESCRIPTORS][0] == 'y')
     for (i = 3; i < 1000; i++)	/* FIXME real limit? */
-      if (i != child_err_report_fd && i != helper_sync_fd)
+      if (i != child_err_report_fd && i != helper_sync_fd && i != saved_stderr_fd)
         if (_get_osfhandle (i) != -1)
           close (i);
 
   /* We don't want our child to inherit the error report and
    * helper sync fds.
    */
-  child_err_report_fd = dup_noninherited (child_err_report_fd, _O_WRONLY);
+  child_err_report_fd = reopen_noninherited (child_err_report_fd, _O_WRONLY);
-  helper_sync_fd = dup_noninherited (helper_sync_fd, _O_RDONLY);
+  helper_sync_fd = reopen_noninherited (helper_sync_fd, _O_RDONLY);
 
   /* argv[ARG_WAIT] is "w" to wait for the program to exit */
   if (argv[ARG_WAIT][0] == 'w')
@@ -351,6 +364,11 @@ main (int ignored_argc, char **ignored_argv)
 
   saved_errno = errno;
 
+  /* Some coverage warnings may be printed on stderr during this process exit.
+   * Remove redirection so that they would go to original stderr
+   * instead of being treated as part of stderr of child process.
+   */
+  dup2 (saved_stderr_fd, 2);
   if (handle == -1 && saved_errno != 0)
     {
       int ec = (saved_errno == ENOENT)

glib/gspawn-win32.c

@@ -105,8 +105,8 @@ enum {
 };
 
 static int
-dup_noninherited (int fd,
+reopen_noninherited (int fd,
-		  int mode)
+		     int mode)
 {
   HANDLE filehandle;
 
@@ -125,28 +125,24 @@ dup_noninherited (int fd,
 #define HELPER_PROCESS "gspawn-win32-helper"
 #endif
 
+/* This logic has a copy for wchar_t in gspawn-win32-helper.c, protect_wargv() */
 static gchar *
 protect_argv_string (const gchar *string)
 {
   const gchar *p = string;
   gchar *retval, *q;
   gint len = 0;
+  gint pre_bslash = 0;
   gboolean need_dblquotes = FALSE;
   while (*p)
     {
       if (*p == ' ' || *p == '\t')
 	need_dblquotes = TRUE;
-      else if (*p == '"')
+      /* estimate max len, assuming that all escapable chracters will be escaped */
-	len++;
+      if (*p == '"' || *p == '\\')
-      else if (*p == '\\')
+	len += 2;
-	{
+      else
-	  const gchar *pp = p;
+	len += 1;
-	  while (*pp && *pp == '\\')
-	    pp++;
-	  if (*pp == '"')
-	    len++;
-	}
-      len++;
       p++;
     }
   
@@ -155,25 +151,40 @@ protect_argv_string (const gchar *string)
 
   if (need_dblquotes)
     *q++ = '"';
-  
+  /* Only quotes and backslashes preceeding quotes are escaped:
+   * see "Parsing C Command-Line Arguments" at
+   * https://docs.microsoft.com/en-us/cpp/c-language/parsing-c-command-line-arguments
+   */
   while (*p)
     {
       if (*p == '"')
-	*q++ = '\\';
-      else if (*p == '\\')
 	{
-	  const gchar *pp = p;
+	  /* Add backslash for escaping quote itself */
-	  while (*pp && *pp == '\\')
+	  *q++ = '\\';
-	    pp++;
+	  /* Add backslash for every preceeding backslash for escaping it */
-	  if (*pp == '"')
+	  for (;pre_bslash > 0; --pre_bslash)
 	    *q++ = '\\';
 	}
+
+      /* Count length of continuous sequence of preceeding backslashes. */
+      if (*p == '\\')
+	++pre_bslash;
+      else
+	pre_bslash = 0;
+
       *q++ = *p;
       p++;
     }
   
   if (need_dblquotes)
-    *q++ = '"';
+    {
+      /* Add backslash for every preceeding backslash for escaping it,
+       * do NOT escape quote itself.
+       */
+      for (;pre_bslash > 0; --pre_bslash)
+	*q++ = '\\';
+      *q++ = '"';
+    }
   *q++ = '\0';
 
   return retval;
@@ -606,7 +617,7 @@ do_spawn_with_fds (gint                 *exit_status,
    * helper process, and the started actual user process. As such that
    * shouldn't harm, but it is unnecessary.
    */
-  child_err_report_pipe[0] = dup_noninherited (child_err_report_pipe[0], _O_RDONLY);
+  child_err_report_pipe[0] = reopen_noninherited (child_err_report_pipe[0], _O_RDONLY);
 
   if (flags & G_SPAWN_FILE_AND_ARGV_ZERO)
     {
@@ -625,7 +636,7 @@ do_spawn_with_fds (gint                 *exit_status,
    * process won't read but won't get any EOF either, as it has the
    * write end open itself.
    */
-  helper_sync_pipe[1] = dup_noninherited (helper_sync_pipe[1], _O_WRONLY);
+  helper_sync_pipe[1] = reopen_noninherited (helper_sync_pipe[1], _O_WRONLY);
 
   if (stdin_fd != -1)
     {
@@ -657,7 +668,7 @@ do_spawn_with_fds (gint                 *exit_status,
       new_argv[ARG_STDOUT] = "-";
     }
   
-  if (stdout_fd != -1)
+  if (stderr_fd != -1)
     {
       _g_sprintf (args[ARG_STDERR], "%d", stderr_fd);
       new_argv[ARG_STDERR] = args[ARG_STDERR];

glib/tests/spawn-singlethread.c

@@ -312,24 +312,40 @@ test_spawn_sync (void)
 {
   int tnum = 1;
   GError *error = NULL;
-  GPtrArray *argv;
+  char *arg = g_strdup_printf ("thread %d", tnum);
-  char *arg;
+  /* Include arguments with special symbols to test that they are correctly passed to child.
+   * This is tested on all platforms, but the most prone to failure is win32,
+   * where args are specially escaped during spawning.
+   */
+  const char * const argv[] = {
+    echo_prog_path,
+    arg,
+    "doublequotes\\\"after\\\\\"\"backslashes", /* this would be special escaped on win32 */
+    "\\\"\"doublequotes spaced after backslashes\\\\\"", /* this would be special escaped on win32 */
+    "even$$dollars",
+    "even%%percents",
+    "even\"\"doublequotes",
+    "even''singlequotes",
+    "even\\\\backslashes",
+    "even//slashes",
+    "$odd spaced$dollars$",
+    "%odd spaced%spercents%",
+    "\"odd spaced\"doublequotes\"",
+    "'odd spaced'singlequotes'",
+    "\\odd spaced\\backslashes\\", /* this wasn't handled correctly on win32 in glib <=2.58 */
+    "/odd spaced/slashes/",
+    NULL
+  };
+  char *joined_args_str = g_strjoinv ("", (char**)argv + 1);
   char *stdout_str;
   int estatus;
 
-  arg = g_strdup_printf ("thread %d", tnum);
+  g_spawn_sync (NULL, (char**)argv, NULL, 0, NULL, NULL, &stdout_str, NULL, &estatus, &error);
-
-  argv = g_ptr_array_new ();
-  g_ptr_array_add (argv, echo_prog_path);
-  g_ptr_array_add (argv, arg);
-  g_ptr_array_add (argv, NULL);
-
-  g_spawn_sync (NULL, (char**)argv->pdata, NULL, 0, NULL, NULL, &stdout_str, NULL, &estatus, &error);
   g_assert_no_error (error);
-  g_assert_cmpstr (arg, ==, stdout_str);
+  g_assert_cmpstr (joined_args_str, ==, stdout_str);
   g_free (arg);
   g_free (stdout_str);
-  g_ptr_array_free (argv, TRUE);
+  g_free (joined_args_str);
 }
 
 /* Like test_spawn_sync but uses spawn flags that trigger the optimized

tests/meson.build

@@ -38,6 +38,7 @@ tests = {
     'dependencies' : [libgthread_dep],
   },
   'sources' : {},
+  'spawn-test' : {},
   'thread-test' : {},
   'threadpool-test' : {'suite' : ['slow']},
   'type-test' : {},
@@ -68,9 +69,12 @@ test_extra_programs = {
 if host_machine.system() != 'windows'
   tests += {
     'timeloop' : {},
-    'spawn-test' : {},
     'iochannel-test' : {},
   }
+else
+  test_extra_programs += {
+    'spawn-test-win32-gui' : {'gui_app' : true}
+  }
 endif
 
 if installed_tests_enabled
@@ -139,5 +143,6 @@ foreach program_name, extra_args : test_extra_programs
       dependencies : common_deps + extra_args.get('dependencies', []),
       install_dir : installed_tests_execdir,
       install : install,
+      gui_app : extra_args.get('gui_app', false),
   )
 endforeach

tests/spawn-test-win32-gui.c

@@ -1,4 +1,5 @@
 #include <windows.h>
+#include <io.h>
 #include <errno.h>
 #include <stdio.h>
 #include <string.h>
@@ -19,24 +20,15 @@ WinMain (struct HINSTANCE__ *hInstance,
 	char *lpszCmdLine,
 	int   nCmdShow)
 {
-  char buf[100];
+  if (__argc >= 2 && strcmp (__argv[1], "print_argv0") == 0)
-
-  if (__argc >= 2 && strcmp (__argv[1], "nop") == 0)
     {
-      sprintf (buf, "spawn-test-win32-gui: argv[0]=\"%s\"", __argv[0]);
+      printf ("%s", __argv[0]);
-      MessageBox (NULL, buf, lpszCmdLine, MB_ICONINFORMATION|MB_SYSTEMMODAL);
     }
   else if (__argc <= 2)
     {
-      MessageBox (NULL, "spawn-test-win32-gui: Will write to stdout",
-		  lpszCmdLine, MB_ICONINFORMATION|MB_SYSTEMMODAL);
-      
       printf ("This is stdout\n");
       fflush (stdout);
       
-      MessageBox (NULL, "spawn-test-win32-gui: Will write to stderr",
-		  lpszCmdLine, MB_ICONINFORMATION|MB_SYSTEMMODAL);
-      
       fprintf (stderr, "This is stderr\n");
       fflush (stderr);
     }
@@ -45,70 +37,52 @@ WinMain (struct HINSTANCE__ *hInstance,
       int infd = atoi (__argv[2]);
       int outfd = atoi (__argv[3]);
       int k, n;
+      char buf[100] = {0};
 
       if (infd < 0 || outfd < 0)
 	{
-	  MessageBox (NULL, "spawn-test-win32-gui: illegal fds on command line",
+	  printf ("spawn-test-win32-gui: illegal fds on command line %s",
-		      lpszCmdLine, MB_ICONERROR|MB_SYSTEMMODAL);
+		      lpszCmdLine);
 	  exit (1);
 	}
 
-      MessageBox (NULL, "spawn-test-win32-gui: Will write to parent",
-		  lpszCmdLine, MB_ICONINFORMATION|MB_SYSTEMMODAL);
-
       n = strlen ("Hello there");
       if (write (outfd, &n, sizeof (n)) == -1 ||
 	  write (outfd, "Hello there", n) == -1)
 	{
 	  int errsv = errno;
-	  sprintf (buf, "spawn-test-win32-gui: Write: %s", strerror (errsv));
+	  printf ("spawn-test-win32-gui: Write error: %s", strerror (errsv));
-	  MessageBox (NULL, buf, lpszCmdLine, MB_ICONERROR|MB_SYSTEMMODAL);
 	  exit (1);
 	}
 
-      MessageBox (NULL, "spawn-test-win32-gui: Will read from parent",
-		  lpszCmdLine, MB_ICONINFORMATION|MB_SYSTEMMODAL);
-
       if ((k = read (infd, &n, sizeof (n))) != sizeof (n))
 	{
-	  sprintf (buf, "spawn-test-win32-gui: Got only %d bytes, wanted %d",
+	  printf ("spawn-test-win32-gui: Got only %d bytes, wanted %d",
-		   k, sizeof (n));
+		   k, (int)sizeof (n));
-	  MessageBox (NULL, buf, lpszCmdLine, MB_ICONERROR|MB_SYSTEMMODAL);
 	  exit (1);
 	}
 
-      sprintf (buf, "spawn-test-win32-gui: Parent says %d bytes to read", n);
+      printf ("spawn-test-win32-gui: Parent says %d bytes to read", n);
-      MessageBox (NULL, buf, lpszCmdLine, MB_ICONINFORMATION|MB_SYSTEMMODAL);
 
       if ((k = read (infd, buf, n)) != n)
 	{
 	  int errsv = errno;
 	  if (k == -1)
-	    sprintf (buf, "spawn-test-win32-gui: Read: %s", strerror (errsv));
+	    printf ("spawn-test-win32-gui: Read error: %s", strerror (errsv));
 	  else
-	    sprintf (buf, "spawn-test-win32-gui: Got only %d bytes", k);
+	    printf ("spawn-test-win32-gui: Got only %d bytes", k);
-	  MessageBox (NULL, buf, lpszCmdLine, MB_ICONERROR|MB_SYSTEMMODAL);
 	  exit (1);
 	}
 
-      MessageBox (NULL, "spawn-test-win32-gui: Will write more to parent",
-		  lpszCmdLine, MB_ICONINFORMATION|MB_SYSTEMMODAL);
-
       n = strlen ("See ya");
       if (write (outfd, &n, sizeof (n)) == -1 ||
 	  write (outfd, "See ya", n) == -1)
 	{
 	  int errsv = errno;
-	  sprintf (buf, "spawn-test-win32-gui: Write: %s", strerror (errsv));
+	  printf ("spawn-test-win32-gui: Write error: %s", strerror (errsv));
-	  MessageBox (NULL, buf, lpszCmdLine, MB_ICONERROR|MB_SYSTEMMODAL);
 	  exit (1);
 	}
     }
 
-  Sleep (2000);
-  
-  MessageBox (NULL, "spawn-test-win32-gui: Done, exiting.",
-	      lpszCmdLine, MB_ICONINFORMATION|MB_SYSTEMMODAL);
-
   return 0;
 }

tests/spawn-test.c

@@ -26,6 +26,7 @@
 #undef G_LOG_DOMAIN
 
 #include <glib.h>
+#include <glib/gstdio.h>
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
@@ -38,15 +39,20 @@
 
 
 static void
-run_tests (void)
+run_tests (const gchar* argv0)
 {
-  GError *err;
+  GError *err = NULL;
   gchar *output = NULL;
-#ifdef G_OS_WIN32
   gchar *erroutput = NULL;
+  gchar *dirname = g_path_get_dirname (argv0);
+#ifdef G_OS_WIN32
   int pipedown[2], pipeup[2];
   gchar **argv = 0;
+  gchar spawn_binary[1000] = {0};
+  gchar full_cmdline[1000] = {0};
+  g_snprintf (spawn_binary, sizeof (spawn_binary), "%s\\spawn-test-win32-gui.exe", dirname);
 #endif
+  g_free (dirname);
   
   err = NULL;
   if (!g_spawn_command_line_sync ("nonexistent_application foo 'bar baz' blah blah",
@@ -96,12 +102,15 @@ run_tests (void)
         }
 
       g_free (output);
+      output = NULL;
     }
-#else
+#endif
-#ifdef G_OS_WIN32
+  /* Running sort synchronously, collecting its output. 'sort' command is selected
-  printf ("Running netstat synchronously, collecting its output\n");
+   * because it is non-builtin command on both unix and win32 with well-defined stdout behaviour.
-
+   */
-  if (!g_spawn_command_line_sync ("netstat -n",
+  g_file_set_contents ("spawn-test-created-file.txt", "line first\nline 2\nline last\n", -1, &err);
+  g_assert_no_error(err);
+  if (!g_spawn_command_line_sync ("sort spawn-test-created-file.txt",
                                   &output, &erroutput, NULL,
                                   &err))
     {
@@ -114,9 +123,9 @@ run_tests (void)
       g_assert (output != NULL);
       g_assert (erroutput != NULL);
       
-      if (strstr (output, "Active Connections") == 0)
+      if (strstr (output, "\nline first") == 0)
         {
-          printf ("output was '%s', should have contained 'Active Connections'\n",
+          printf ("output was '%s', should have contained 'line first' in second line\n",
                   output);
 
           exit (1);
@@ -132,13 +141,36 @@ run_tests (void)
       output = NULL;
       g_free (erroutput);
       erroutput = NULL;
+      g_unlink ("spawn-test-created-file.txt");
     }
 
-  printf ("Running spawn-test-win32-gui in various ways. Click on the OK buttons.\n");
+  if (!g_spawn_command_line_sync ("sort non-existing-file.txt",
+                                  NULL, &erroutput, NULL,
+                                  &err))
+    {
+      fprintf (stderr, "Error: %s\n", err->message);
+      g_error_free (err);
+      exit (1);
+    }
+  else
+    {
+      g_assert (erroutput != NULL);
+
+      if (erroutput[0] == '\0')
+        {
+          printf ("erroutput was empty, expected contain error message about non-existing-file.txt\n");
+          exit (1);
+        }
+      g_free (erroutput);
+      erroutput = NULL;
+    }
+
+#ifdef G_OS_WIN32
+  printf ("Running spawn-test-win32-gui in various ways.\n");
 
   printf ("First asynchronously (without wait).\n");
-
+  g_snprintf (full_cmdline, sizeof (full_cmdline), "'%s' 1", spawn_binary);
-  if (!g_spawn_command_line_async ("'.\\spawn-test-win32-gui.exe' 1", &err))
+  if (!g_spawn_command_line_async (full_cmdline, &err))
     {
       fprintf (stderr, "Error: %s\n", err->message);
       g_error_free (err);
@@ -146,7 +178,8 @@ run_tests (void)
     }
 
   printf ("Now synchronously, collecting its output.\n");
-  if (!g_spawn_command_line_sync ("'.\\spawn-test-win32-gui.exe' 2",
+  g_snprintf (full_cmdline, sizeof (full_cmdline), "'%s' 2", spawn_binary);
+  if (!g_spawn_command_line_sync (full_cmdline,
 				  &output, &erroutput, NULL,
 				  &err))
     {
@@ -174,27 +207,39 @@ run_tests (void)
 	}
 
       g_free (output);
+      output = NULL;
       g_free (erroutput);
+      erroutput = NULL;
     }
 
   printf ("Now with G_SPAWN_FILE_AND_ARGV_ZERO.\n");
-
+  g_snprintf (full_cmdline, sizeof (full_cmdline), "'%s' this-should-be-argv-zero print_argv0", spawn_binary);
-  if (!g_shell_parse_argv ("'.\\spawn-test-win32-gui.exe' this-should-be-argv-zero nop", NULL, &argv, &err))
+  if (!g_shell_parse_argv (full_cmdline, NULL, &argv, &err))
     {
       fprintf (stderr, "Error parsing command line? %s\n", err->message);
       g_error_free (err);
       exit (1);
     }
 
-  if (!g_spawn_async (NULL, argv, NULL,
+  if (!g_spawn_sync (NULL, argv, NULL,
 		      G_SPAWN_FILE_AND_ARGV_ZERO,
-		      NULL, NULL, NULL,
+		      NULL, NULL, &output, NULL, NULL,
 		      &err))
     {
       fprintf (stderr, "Error: %s\n", err->message);
       g_error_free (err);
       exit (1);
     }
+  else
+    {
+      if (strcmp (output, "this-should-be-argv-zero") != 0)
+	{
+	  printf ("output was '%s', should have been 'this-should-be-argv-zero'\n", output);
+	  exit (1);
+	}
+      g_free (output);
+      output = NULL;
+    }
 
   printf ("Now talking to it through pipes.\n");
 
@@ -205,8 +250,8 @@ run_tests (void)
       exit (1);
     }
 
-  if (!g_shell_parse_argv (g_strdup_printf ("'.\\spawn-test-win32-gui.exe' pipes %d %d",
+  g_snprintf (full_cmdline, sizeof (full_cmdline), "'%s' pipes %d %d", spawn_binary, pipedown[0], pipeup[1]);
-					    pipedown[0], pipeup[1]),
+  if (!g_shell_parse_argv (full_cmdline,
                            NULL, &argv,
                            &err))
     {
@@ -237,7 +282,7 @@ run_tests (void)
 	    fprintf (stderr, "Read error: %s\n", g_strerror (errsv));
 	  else
 	    fprintf (stderr, "Wanted to read %d bytes, got %d\n",
-		     sizeof (n), k);
+		     (int)sizeof (n), k);
 	  exit (1);
 	}
 
@@ -268,7 +313,12 @@ run_tests (void)
 	    fprintf (stderr, "Read error: %s\n", g_strerror (errsv));
 	  else
 	    fprintf (stderr, "Wanted to read %d bytes, got %d\n",
-		     sizeof (n), k);
+		     (int)sizeof (n), k);
+	  exit (1);
+	}
+      if (n != strlen ("See ya"))
+	{
+	  printf ("child wrote %d bytes, expected %d", n, (int) strlen ("See ya"));
 	  exit (1);
 	}
 
@@ -282,16 +332,21 @@ run_tests (void)
 		     n, k);
 	  exit (1);
 	}
+      buf[n] = '\0';
+      if (strcmp (buf, "See ya") != 0)
+	{
+	  printf ("output was '%s', should have been 'See ya'\n", buf);
+	  exit (1);
+	}
     }
 #endif
-#endif
 }
 
 int
 main (int   argc,
       char *argv[])
 {
-  run_tests ();
+  run_tests (argv[0]);
   
   return 0;
 }