[Emacs-diffs] trunk r113425: * callproc.c (Fcall_process_region): Fix minor race and tune.

[Paul Eggert]

------------------------------------------------------------
revno: 113425
revision-id: address@hidden
parent: address@hidden
committer: Paul Eggert <address@hidden>
branch nick: trunk
timestamp: Sun 2013-07-14 19:56:17 -0700
message:
  * callproc.c (Fcall_process_region): Fix minor race and tune.
  
  (create_temp_file): New function, with the temp-file-creation part
  of the old Fcall_process_region.  Use Fcopy_sequence to create the
  temp file name, rather than alloca + build_string, for simplicity.
  Don't bother to block input around the temp file creation;
  shouldn't be needed.  Simplify use of mktemp.  Use
  record_unwind_protect immediately after creating the temp file;
  this closes an unlikely race where the temp file was not removed.
  Use memcpy rather than an open-coded loop.
  (Fcall_process_region): Use the new function.  If the input is
  empty, redirect from /dev/null rather than from a newly created
  empty temp file; this avoids unnecessary file system traffic.
modified:
  src/ChangeLog                  changelog-20091113204419-o5vbwnq5f7feedwu-1438
  src/callproc.c                 callproc.c-20091113204419-o5vbwnq5f7feedwu-248

=== modified file 'src/ChangeLog'
--- a/src/ChangeLog     2013-07-14 23:12:42 +0000
+++ b/src/ChangeLog     2013-07-15 02:56:17 +0000
@@ -1,3 +1,18 @@
+2013-07-15  Paul Eggert  <address@hidden>
+
+       * callproc.c (Fcall_process_region): Fix minor race and tune.
+       (create_temp_file): New function, with the temp-file-creation part
+       of the old Fcall_process_region.  Use Fcopy_sequence to create the
+       temp file name, rather than alloca + build_string, for simplicity.
+       Don't bother to block input around the temp file creation;
+       shouldn't be needed.  Simplify use of mktemp.  Use
+       record_unwind_protect immediately after creating the temp file;
+       this closes an unlikely race where the temp file was not removed.
+       Use memcpy rather than an open-coded loop.
+       (Fcall_process_region): Use the new function.  If the input is
+       empty, redirect from /dev/null rather than from a newly created
+       empty temp file; this avoids unnecessary file system traffic.
+
 2013-07-14  Paul Eggert  <address@hidden>
 
        * filelock.c (create_lock_file) [!HAVE_MKOSTEMP && !HAVE_MKSTEMP]:

=== modified file 'src/callproc.c'
--- a/src/callproc.c    2013-07-14 00:30:45 +0000
+++ b/src/callproc.c    2013-07-15 02:56:17 +0000
@@ -960,41 +960,16 @@
   return Qnil;
 }
 
-DEFUN ("call-process-region", Fcall_process_region, Scall_process_region,
-       3, MANY, 0,
-       doc: /* Send text from START to END to a synchronous process running 
PROGRAM.
-The remaining arguments are optional.
-Delete the text if fourth arg DELETE is non-nil.
-
-Insert output in BUFFER before point; t means current buffer; nil for
- BUFFER means discard it; 0 means discard and don't wait; and `(:file
- FILE)', where FILE is a file name string, means that it should be
- written to that file (if the file already exists it is overwritten).
-BUFFER can also have the form (REAL-BUFFER STDERR-FILE); in that case,
-REAL-BUFFER says what to do with standard output, as above,
-while STDERR-FILE says what to do with standard error in the child.
-STDERR-FILE may be nil (discard standard error output),
-t (mix it with ordinary output), or a file name string.
-
-Sixth arg DISPLAY non-nil means redisplay buffer as output is inserted.
-Remaining args are passed to PROGRAM at startup as command args.
-
-If BUFFER is 0, `call-process-region' returns immediately with value nil.
-Otherwise it waits for PROGRAM to terminate
-and returns a numeric exit status or a signal description string.
-If you quit, the process is killed with SIGINT, or SIGKILL if you quit again.
-
-usage: (call-process-region START END PROGRAM &optional DELETE BUFFER DISPLAY 
&rest ARGS)  */)
-  (ptrdiff_t nargs, Lisp_Object *args)
+/* Create a temporary file suitable for storing the input data of
+   call-process-region.  NARGS and ARGS are the same as for
+   call-process-region.  */
+
+static Lisp_Object
+create_temp_file (ptrdiff_t nargs, Lisp_Object *args)
 {
   struct gcpro gcpro1;
   Lisp_Object filename_string;
-  register Lisp_Object start, end;
-  ptrdiff_t count = SPECPDL_INDEX ();
-  /* Qt denotes we have not yet called Ffind_operation_coding_system.  */
-  Lisp_Object coding_systems;
-  Lisp_Object val, *args2;
-  ptrdiff_t i;
+  Lisp_Object val, start, end;
   Lisp_Object tmpdir;
 
   if (STRINGP (Vtemporary_file_directory))
@@ -1016,9 +991,7 @@
     }
 
   {
-    USE_SAFE_ALLOCA;
     Lisp_Object pattern = Fexpand_file_name (Vtemp_file_name_pattern, tmpdir);
-    Lisp_Object encoded_tem;
     char *tempfile;
 
 #ifdef WINDOWSNT
@@ -1036,39 +1009,30 @@
       }
 #endif
 
-    encoded_tem = ENCODE_FILE (pattern);
-    tempfile = SAFE_ALLOCA (SBYTES (encoded_tem) + 1);
-    memcpy (tempfile, SDATA (encoded_tem), SBYTES (encoded_tem) + 1);
-    coding_systems = Qt;
+    filename_string = Fcopy_sequence (ENCODE_FILE (pattern));
+    GCPRO1 (filename_string);
+    tempfile = SSDATA (filename_string);
 
-#if defined HAVE_MKOSTEMP || defined HAVE_MKSTEMP
     {
-      int fd, open_errno;
+      int fd;
 
-      block_input ();
-# ifdef HAVE_MKOSTEMP
+#ifdef HAVE_MKOSTEMP
       fd = mkostemp (tempfile, O_CLOEXEC);
-# else
+#elif defined HAVE_MKSTEMP
       fd = mkstemp (tempfile);
-# endif
-      open_errno = errno;
-      unblock_input ();
+#else
+      errno = EEXIST;
+      mktemp (tempfile);
+      /* INT_MAX denotes success, because close (INT_MAX) does nothing.  */
+      fd = *tempfile ? INT_MAX : -1;
+#endif
       if (fd < 0)
-       report_file_errno ("Failed to open temporary file",
-                          Fcons (build_string (tempfile), Qnil), open_errno);
+       report_file_error ("Failed to open temporary file using pattern",
+                          Fcons (pattern, Qnil));
       emacs_close (fd);
     }
-#else
-    errno = EEXIST;
-    mktemp (tempfile);
-    if (!*tempfile)
-      report_file_error ("Failed to open temporary file using pattern",
-                        Fcons (pattern, Qnil));
-#endif
 
-    filename_string = build_string (tempfile);
-    GCPRO1 (filename_string);
-    SAFE_FREE ();
+    record_unwind_protect (delete_temp_file, filename_string);
   }
 
   start = args[0];
@@ -1080,10 +1044,12 @@
     val = Qraw_text;
   else
     {
+      Lisp_Object coding_systems;
+      Lisp_Object *args2;
       USE_SAFE_ALLOCA;
       SAFE_NALLOCA (args2, 1, nargs + 1);
       args2[0] = Qcall_process_region;
-      for (i = 0; i < nargs; i++) args2[i + 1] = args[i];
+      memcpy (args2 + 1, args, nargs * sizeof *args);
       coding_systems = Ffind_operation_coding_system (nargs + 1, args2);
       val = CONSP (coding_systems) ? XCDR (coding_systems) : Qnil;
       SAFE_FREE ();
@@ -1105,7 +1071,59 @@
   /* Note that Fcall_process takes care of binding
      coding-system-for-read.  */
 
-  record_unwind_protect (delete_temp_file, filename_string);
+  RETURN_UNGCPRO (filename_string);
+}
+
+DEFUN ("call-process-region", Fcall_process_region, Scall_process_region,
+       3, MANY, 0,
+       doc: /* Send text from START to END to a synchronous process running 
PROGRAM.
+The remaining arguments are optional.
+Delete the text if fourth arg DELETE is non-nil.
+
+Insert output in BUFFER before point; t means current buffer; nil for
+ BUFFER means discard it; 0 means discard and don't wait; and `(:file
+ FILE)', where FILE is a file name string, means that it should be
+ written to that file (if the file already exists it is overwritten).
+BUFFER can also have the form (REAL-BUFFER STDERR-FILE); in that case,
+REAL-BUFFER says what to do with standard output, as above,
+while STDERR-FILE says what to do with standard error in the child.
+STDERR-FILE may be nil (discard standard error output),
+t (mix it with ordinary output), or a file name string.
+
+Sixth arg DISPLAY non-nil means redisplay buffer as output is inserted.
+Remaining args are passed to PROGRAM at startup as command args.
+
+If BUFFER is 0, `call-process-region' returns immediately with value nil.
+Otherwise it waits for PROGRAM to terminate
+and returns a numeric exit status or a signal description string.
+If you quit, the process is killed with SIGINT, or SIGKILL if you quit again.
+
+usage: (call-process-region START END PROGRAM &optional DELETE BUFFER DISPLAY 
&rest ARGS)  */)
+  (ptrdiff_t nargs, Lisp_Object *args)
+{
+  struct gcpro gcpro1;
+  Lisp_Object filename_string;
+  ptrdiff_t count = SPECPDL_INDEX ();
+  Lisp_Object start = args[0];
+  Lisp_Object end = args[1];
+  bool empty_input;
+
+  if (STRINGP (start))
+    empty_input = SCHARS (start) == 0;
+  else if (NILP (start))
+    empty_input = BEG == Z;
+  else
+    {
+      validate_region (&args[0], &args[1]);
+      start = args[0];
+      end = args[1];
+      empty_input = XINT (start) == XINT (end);
+    }
+
+  filename_string = (empty_input
+                    ? build_string (NULL_DEVICE)
+                    : create_temp_file (nargs, args));
+  GCPRO1 (filename_string);
 
   if (nargs > 3 && !NILP (args[3]))
     Fdelete_region (start, end);