bug#56002: src/process.c; make-process fails to clean up stderr process on early exit

[Tom Gillespie <tgbugs@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2436 bytes --]

After digging deep into this I think I have a solution. It is attached
as two patches, one that adds tests and one that updates Fmake_proc.
For the record, there was not a missing unwind-protect.

There are two changes to the implementation that are complementary.

The first is to change the internal structure of Fmake_process so that
stderrproc is created immediately before the call to create_process.
This avoids the need to call remove_process on stderrproc in the event
of any expected or unexpected exits prior to the call to create process.

This resolves the issue as originally described.

The second change is to decouple the query-on-exit behavior of
stderrproc from that of proc by adding a new keyword argument
:query-stderr which defaults to nil. The coupling of query-on-exit for
proc and stderrproc in the currently implementation forces users to
always add additional boilerplate specifically when they don't care
about stderr, which is the opposite of what would want (forcing
callers who don't care about something to handle it, rather than the
other way around).

In addition to the patches, I have a question about an inconsistency
in the documentation, and a suggestion about how it might be resolved.
I can open this as a new issue if that makes more sense.

One thing I don't understand is the interaction between :command
'(nil) and :stderr a-pipe-process. It seems that the behaviors
described by their documentation should be mutually exclusive, but
make-process creates a pty without complaining.

I suggest that either an error be raised if :command '(nil) and
:stderr a-pipe-process are provided at the same time, because in all
other cases for :stderr there is a call to CHECK_STRING that will
cause an error, and it is not clear why the pipe process variant should
be exempt from this check since it does not appear that the stderr pipe
process is ever hooked up to the pty.

Any insights here would be appreciated.

At the very least the documentation for :connection-type needs to be
updated to correct the following, since it does not match the current
implemented behavior

> if a non-nil value is specified for the :stderr parameter ... the type will always be pipe

As it stands I think I have preserved existing behavior with some
slight code duplication that reflects the confusion about what should
happens when a stderr process is provided and create_pty is called
instead of create_process.

[-- Attachment #2: 0001-test-src-process-tests.el-test-make-process-stderr-q.patch --]
[-- Type: text/x-patch, Size: 4080 bytes --]

From 2b0d3d2aa886a482bb0e5546ec27fef47f9d34b0 Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Wed, 29 Jun 2022 13:26:06 -0700
Subject: [PATCH 1/2] test/src/process-tests.el: test make-process stderr
 query/cleanup

* test/src/process-tests.el (make-process/query-stderr): Added to
ensure :query-stderr behaves as expected.

* test/src/process-tests.el (make-process/cleanup-stderrproc): Added
to ensure that make-process correctly cleans up/never creates
stderrproc in the event of an early exit/error.
---
 test/src/process-tests.el | 65 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/test/src/process-tests.el b/test/src/process-tests.el
index 824c6da119..50159ab46b 100644
--- a/test/src/process-tests.el
+++ b/test/src/process-tests.el
@@ -233,6 +233,71 @@ make-process/noquery-stderr
               (should-not (process-query-on-exit-flag process))))
         (kill-process process))))))
 
+(ert-deftest make-process/query-stderr ()
+  (skip-unless (executable-find "bash"))
+  (let ((stderr-buffer (generate-new-buffer " rc stderr")))
+    (unwind-protect
+        (let ((process
+               (make-process
+                :name "process that never actually starts"
+                :stderr stderr-buffer
+                :command '("bash" "-c" "echo stdout; echo stderr 1>&2;"))))
+          (while (accept-process-output (get-buffer-process (process-buffer process)))))
+      (let ((stderrproc (get-buffer-process stderr-buffer)))
+        (should stderrproc)
+        (should-not (process-query-on-exit-flag stderrproc)))
+      (should (get-buffer-process stderr-buffer))
+      (kill-buffer stderr-buffer))
+    (should-not (buffer-live-p stderr-buffer)))
+
+  (let ((stderr-buffer (generate-new-buffer " rc stderr")))
+    (unwind-protect
+        (let ((process
+               (make-process
+                :name "process that never actually starts"
+                :stderr stderr-buffer
+                :query-stderr t
+                :command '("bash" "-c" "echo stdout; echo stderr 1>&2;"))))
+          (while (accept-process-output (get-buffer-process (process-buffer process)))))
+      (let ((stderrproc (get-buffer-process stderr-buffer)))
+        (should stderrproc)
+        (should (process-query-on-exit-flag stderrproc)))
+      (while (accept-process-output (get-buffer-process stderr-buffer)))
+      (should-not (get-buffer-process stderr-buffer))
+      (kill-buffer stderr-buffer))
+    (should-not (buffer-live-p stderr-buffer))))
+
+(ert-deftest make-process/cleanup-stderrproc ()
+  "ensure stderrproc is cleaned up/never created if `make-process' fails"
+  (should-error
+   (let ((stderr-buffer (generate-new-buffer " rc stderr")))
+     (unwind-protect
+         (let ((process
+                (make-process
+                 :name "process that never actually starts"
+                 :stderr stderr-buffer
+                 :query-stderr t
+                 :command '("i_fail_before_there_can_be_a_return_code")))))
+       (should-not (get-buffer-process stderr-buffer))
+       (kill-buffer stderr-buffer))
+     (should-not (buffer-live-p stderr-buffer))))
+
+  (should-error
+   (let ((stderr-buffer (generate-new-buffer " rc stderr")))
+     (unwind-protect
+         (let ((process
+                (make-process
+                 :name "process that never actually starts"
+                 :stderr stderr-buffer
+                 :query-stderr t
+                 :command '("i_fail_before_there_can_be_a_return_code"))))
+           (message "this will never print because we never get here")
+           (while (accept-process-output process)))
+       (should-not (get-buffer-process stderr-buffer))
+       (while (accept-process-output (get-buffer-process stderr-buffer)))
+       (kill-buffer stderr-buffer))
+     (should-not (buffer-live-p stderr-buffer)))))
+
 ;; Return t if OUTPUT could have been generated by merging the INPUTS somehow.
 (defun process-tests--mixable (output &rest inputs)
   (while (and output (let ((ins inputs))
-- 
2.35.1


[-- Attachment #3: 0002-make-process-stderrproc-cleanup-on-failure-decouple-.patch --]
[-- Type: text/x-patch, Size: 4567 bytes --]

From 07a1fa34edf067eabf726b9b44ef08bf67771c60 Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Wed, 29 Jun 2022 13:48:09 -0700
Subject: [PATCH 2/2] make-process stderrproc cleanup on failure decouple
 stderr query

* src/process.c (Fmake_process): Move the call to create the stderr
process as late as possible to avoid having to clean up stderrproc in
the event of an error prior to the call to create_process. This change
is needed to ensure that when called with :query-stderr t (aka the
default behavior prior to the addition of :query-stderr) make-process
will not leak the stderr process if a call to make-process fails.
Also adds a new keyword argument :query-stderr to control whether to
query on exit the stderr process (if one is created). (bug#56002)
---
 src/process.c | 57 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 21 deletions(-)

diff --git a/src/process.c b/src/process.c
index af402c8edb..c08dd6995a 100644
--- a/src/process.c
+++ b/src/process.c
@@ -1732,6 +1732,11 @@ DEFUN ("make-process", Fmake_process, Smake_process, 0, MANY, 0,
 :noquery BOOL -- When exiting Emacs, query the user if BOOL is nil and
 the process is running.  If BOOL is not given, query before exiting.
 
+:query-stderr BOOL -- When exiting Emacs, query the user if BOOL is
+non-nil and the stderr process is running. If BOOL is not given, do
+not query before exiting. This has no effect if the value passed to
+:stderr is a process.
+
 :stop BOOL -- BOOL must be nil.  The `:stop' key is ignored otherwise
 and is retained for compatibility with other process types such as
 pipe processes.  Asynchronous subprocesses never start in the
@@ -1804,8 +1809,6 @@ DEFUN ("make-process", Fmake_process, Smake_process, 0, MANY, 0,
   if (!NILP (program))
     CHECK_STRING (program);
 
-  bool query_on_exit = NILP (plist_get (contact, QCnoquery));
-
   stderrproc = Qnil;
   xstderr = plist_get (contact, QCstderr);
   if (PROCESSP (xstderr))
@@ -1815,16 +1818,7 @@ DEFUN ("make-process", Fmake_process, Smake_process, 0, MANY, 0,
       stderrproc = xstderr;
     }
   else if (!NILP (xstderr))
-    {
-      CHECK_STRING (program);
-      stderrproc = CALLN (Fmake_pipe_process,
-			  QCname,
-			  concat2 (name, build_string (" stderr")),
-			  QCbuffer,
-			  Fget_buffer_create (xstderr, Qnil),
-			  QCnoquery,
-			  query_on_exit ? Qnil : Qt);
-    }
+    CHECK_STRING (program);
 
   proc = make_process (name);
   record_unwind_protect (start_process_unwind, proc);
@@ -1837,7 +1831,7 @@ DEFUN ("make-process", Fmake_process, Smake_process, 0, MANY, 0,
   pset_filter (XPROCESS (proc), plist_get (contact, QCfilter));
   pset_command (XPROCESS (proc), Fcopy_sequence (command));
 
-  if (!query_on_exit)
+  if (!NILP (plist_get (contact, QCnoquery)))
     XPROCESS (proc)->kill_without_query = 1;
   tem = plist_get (contact, QCstop);
   /* Normal processes can't be started in a stopped state, see
@@ -1854,13 +1848,6 @@ DEFUN ("make-process", Fmake_process, Smake_process, 0, MANY, 0,
   else
     report_file_error ("Unknown connection type", tem);
 
-  if (!NILP (stderrproc))
-    {
-      pset_stderrproc (XPROCESS (proc), stderrproc);
-
-      XPROCESS (proc)->pty_flag = false;
-    }
-
 #ifdef HAVE_GNUTLS
   /* AKA GNUTLS_INITSTAGE(proc).  */
   verify (GNUTLS_STAGE_EMPTY == 0);
@@ -2026,10 +2013,37 @@ DEFUN ("make-process", Fmake_process, Smake_process, 0, MANY, 0,
 	  tem = XCDR (tem);
 	}
 
+      if (!PROCESSP (xstderr) && !NILP (xstderr))
+	{
+	  stderrproc = CALLN (Fmake_pipe_process,
+			      QCname,
+			      concat2 (name, build_string (" stderr")),
+			      QCbuffer,
+			      Fget_buffer_create (xstderr, Qnil),
+			      QCnoquery,
+			      NILP (plist_get (contact, QCquery_stderr)) ? Qt : Qnil);
+	}
+
+      if (!NILP (stderrproc))
+	{
+	  pset_stderrproc (XPROCESS (proc), stderrproc);
+
+	  XPROCESS (proc)->pty_flag = false;
+	}
+
       create_process (proc, new_argv, current_dir);
     }
   else
-    create_pty (proc);
+    {
+      if (!NILP (stderrproc))
+	{
+	  pset_stderrproc (XPROCESS (proc), stderrproc);
+
+	  XPROCESS (proc)->pty_flag = false;
+	}
+
+      create_pty (proc);
+    }
 
   return SAFE_FREE_UNBIND_TO (count, proc);
 }
@@ -8543,6 +8557,7 @@ syms_of_process (void)
   DEFSYM (Qnsm_verify_connection, "nsm-verify-connection");
   DEFSYM (QClog, ":log");
   DEFSYM (QCnoquery, ":noquery");
+  DEFSYM (QCquery_stderr, ":query-stderr");
   DEFSYM (QCstop, ":stop");
   DEFSYM (QCplist, ":plist");
   DEFSYM (QCcommand, ":command");
-- 
2.35.1