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

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

[Tom Gillespie]

If the primary subprocess created by make-process fails early then the
stderr process is not cleaned up and running kill-buffer on any stderr
buffer attached to the stderr process will prompt the user.

Two early exits that can cause the issue are

1. in make-process if the command is not found
report_file_error ("Searching for program", program);

2. in create_process if vfork fails
report_file_errno (CHILD_SETUP_ERROR_DESC, Qnil, vfork_errno);

I'm sure there are other failure modes that would trigger the issue.

I know of at least two ways to trigger the behavior that correspond to
those two lines. One is to provide a :command that does not exist, the
other is to call an executable file with a format that Emacs does not
understand (e.g. a valid sh file that is missing a shebang line).

The following example shows the behavior for a non-existent command on
master at 556c304007fbea1a552c65529fa86c0a5637b27b. When running it
the program will stop at a prompt to kill the " rc stderr" buffer.
#+begin_src bash
read -r -d '' example <<'EOF'
(let ((stderr-buffer (generate-new-buffer " rc stderr")))
  (unwind-protect
      (let ((process
             (make-process
              :name "process that never actually starts"
              :stderr stderr-buffer
              :command '("i_fail_before_there_can_be_a_return_code")))))
    (kill-buffer stderr-buffer)))
EOF
src/emacs -Q -batch -eval "${example}"
#+end_src

One potential fix for the issue would be to decouple :noquery for the
primary process from the stderr process and make the stderr process
query_on_exit always false. The user has no knowledge that the stderr
process exists and also has no way to reference it from their code. If
they want :noquery nil behavior an advanced user could always
construct the stderr process themselves.

If that is done then it seems that the stderr process will
automatically clean itself up once the stderr buffer is killed. This
seems easier than trying to catch all early exit cases that would
leave the stderr process alive during an early exit.

bug#56002: update with an additional example

[Tom Gillespie]

I have been reading the docs on accepting output again,
and I see that there might be some subtlety here, but in
fact following the examples in the docs should reveal
another way the bug can express itself.

Specifically, in the original example I did not call
accept-process-output on the stderr process as is
suggested by the docs. Unfortunately, the docs are
misleading in this case because I am doing things
inside an unwind-protect cleanup clause and
accepting output from the stderr process causes
the example (below) to hang forever.

I think this happens because the stderr process is not
cleaned up correctly, OR possibly because of some
unexpected interaction due to the use of unwind-protect.

Example:
#+begin_src bash
read -r -d '' example <<'EOF'
(let ((stderr-buffer (generate-new-buffer " rc stderr")))
  (unwind-protect
      (let ((process
             (make-process
              :name "process that never actually starts"
              :stderr stderr-buffer
              :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)))
    (while (accept-process-output (get-buffer-process stderr-buffer)))
    (kill-buffer stderr-buffer)))
EOF
emacs -Q -batch -eval "${example}"
#+end_src

Docs in question:
https://www.gnu.org/software/emacs/manual/html_node/elisp/Accepting-Output.html

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

[Eli Zaretskii]

> From: Tom Gillespie <tgbugs@gmail.com>
> Date: Wed, 15 Jun 2022 15:38:05 -0700
> 
> If the primary subprocess created by make-process fails early then the
> stderr process is not cleaned up and running kill-buffer on any stderr
> buffer attached to the stderr process will prompt the user.

Can you elaborate on what do you mean by "clean up the stderr
process"?  Do you see the code which does that in the "normal" cases?

> Two early exits that can cause the issue are
> 
> 1. in make-process if the command is not found
> report_file_error ("Searching for program", program);
> 
> 2. in create_process if vfork fails
> report_file_errno (CHILD_SETUP_ERROR_DESC, Qnil, vfork_errno);
> 
> I'm sure there are other failure modes that would trigger the issue.

Sounds like we lack some unwind-protect call somewhere?

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

[Tom Gillespie]

> Can you elaborate on what do you mean by "clean up the stderr
> process"?  Do you see the code which does that in the "normal" cases?

I'm not entirely sure as I am unfamiliar with the life cycle for processes in
the runtime. I think that the "normal" cleanup happens when the parent
process exits with a return code. The stderr process is a pipe process so
I assume that under normal circumstances the pipe process would receive
a signal from the primary process via the os and exit allowing calls to
accept-process-output to complete and then presumably the gc would
take care of the rest? I have no idea if this is remotely accurate.

It does look like remote_process is called during other failures though.

> Sounds like we lack some unwind-protect call somewhere?

It does look like there are a number of calls to record_unwind_protect
(remove_process, proc); in the code. Maybe there is a missing
record_unwind_protect (remove_process, stderrproc); in make-process?

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

[Tom Gillespie]

[-- 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

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

[Tom Gillespie]

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

Here is an update to the 0002 patch to account for recent changes.

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

From f8830045187d28c90ee6167eafc79a0019c41ab7 Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Sun, 7 Aug 2022 16:43:13 -0700
Subject: [PATCH] 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)

Now with changes to account for intervening changes to pty code.
---
 src/process.c | 45 ++++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/src/process.c b/src/process.c
index 23479c0619..11f85c5d1d 100644
--- a/src/process.c
+++ b/src/process.c
@@ -1762,6 +1762,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
@@ -1837,8 +1842,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))
@@ -1848,16 +1851,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);
@@ -1870,7 +1864,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
@@ -1889,9 +1883,6 @@ DEFUN ("make-process", Fmake_process, Smake_process, 0, MANY, 0,
 	is_pty_from_symbol (tem);
     }
 
-  if (!NILP (stderrproc))
-    pset_stderrproc (XPROCESS (proc), stderrproc);
-
 #ifdef HAVE_GNUTLS
   /* AKA GNUTLS_INITSTAGE(proc).  */
   verify (GNUTLS_STAGE_EMPTY == 0);
@@ -2057,10 +2048,29 @@ 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);
+
       create_process (proc, new_argv, current_dir);
     }
   else
-    create_pty (proc);
+    {
+      if (!NILP (stderrproc))
+	pset_stderrproc (XPROCESS (proc), stderrproc);
+
+      create_pty (proc);
+    }
 
   return SAFE_FREE_UNBIND_TO (count, proc);
 }
@@ -8609,6 +8619,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

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

[Lars Ingebrigtsen]

Tom Gillespie <tgbugs@gmail.com> writes:

> * 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)

I think this makes sense...  Eli, any comments?

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  56002@debbugs.gnu.org
> Date: Mon, 08 Aug 2022 13:36:58 +0200
> 
> Tom Gillespie <tgbugs@gmail.com> writes:
> 
> > * 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)
> 
> I think this makes sense...  Eli, any comments?

TBH, I don't understand the rationale well enough.  What does it mean
we "leak stderr process"? isn't the stderr process recycled if
make-process fails?  Is it just a matter of some simple change in the
control flow, to make sure we always go through the cleanup code
before we return?

In general, I'd prefer not to change timing of what we do in
make-process unless it's really unavoidable.  There's a lot of
system-dependent stuff there, and who knows what will that cause on
some platform?

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

[Tom Gillespie]

> TBH, I don't understand the rationale well enough.  What does it mean
> we "leak stderr process"? isn't the stderr process recycled if
> make-process fails?

When the stderrproc is created internally there is no way that it can be
reused because nothing else has a reference to it. The changes here only
deal with cases where a stderrproc is not provided by the caller.

> Is it just a matter of some simple change in the
> control flow, to make sure we always go through the cleanup code
> before we return?

There is no way easy way to modify the control flow because there are
so many possible points where process creation can fail unexpectedly
that must be handled if the stderrproc is created too early.

It is very hard to determine exactly which statements inside of make
process can produce unexpected exits, I have encountered at least
two, but I'm sure there are others. Thus the safest thing to do was to
move creation of stderrproc after all the points where process creation
can potentially fail (e.g. failures during vfork).

> In general, I'd prefer not to change timing of what we do in
> make-process unless it's really unavoidable.  There's a lot of
> system-dependent stuff there, and who knows what will that cause on
> some platform?

I'm fairly certain that it is impossible for there to be any interaction between
the primary process and the stderr proc at any point in time prior to where
I moved the creation of the stderrproc because the first reference to
p->stderrproc after it is created is in create_process.

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

[Eli Zaretskii]

> From: Tom Gillespie <tgbugs@gmail.com>
> Date: Mon, 8 Aug 2022 11:54:09 -0700
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, 56002@debbugs.gnu.org
> 
> > TBH, I don't understand the rationale well enough.  What does it mean
> > we "leak stderr process"? isn't the stderr process recycled if
> > make-process fails?
> 
> When the stderrproc is created internally there is no way that it can be
> reused because nothing else has a reference to it. The changes here only
> deal with cases where a stderrproc is not provided by the caller.

This is a misunderstanding: I meant "recycled" as in
"garbage-collected".  GC in Emacs is supposed to prevent leaks of
memory and resources.  You seem to be saying that this somehow doesn't
work in this case.  Can you explain why it doesn't work, and which
resources specifically appear to be leaking?

> > Is it just a matter of some simple change in the
> > control flow, to make sure we always go through the cleanup code
> > before we return?
> 
> There is no way easy way to modify the control flow because there are
> so many possible points where process creation can fail unexpectedly
> that must be handled if the stderrproc is created too early.

But we have code that errors out in the middle of processing all over
the place, and that is safe in Emacs, because any unused Lisp objects
will be GC'ed soon.  Why is this case special?

> It is very hard to determine exactly which statements inside of make
> process can produce unexpected exits, I have encountered at least
> two, but I'm sure there are others. Thus the safest thing to do was to
> move creation of stderrproc after all the points where process creation
> can potentially fail (e.g. failures during vfork).
> 
> > In general, I'd prefer not to change timing of what we do in
> > make-process unless it's really unavoidable.  There's a lot of
> > system-dependent stuff there, and who knows what will that cause on
> > some platform?
> 
> I'm fairly certain that it is impossible for there to be any interaction between
> the primary process and the stderr proc at any point in time prior to where
> I moved the creation of the stderrproc because the first reference to
> p->stderrproc after it is created is in create_process.

I meant the potential interactions that are not explicitly visible by
reading the code, but instead stem from system-dependent stuff that is
related to how subprocesses are created on different systems.

So I'd still be happier if we could deal with the problem without
moving chunks of code around.

Thanks.

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

[Tom Gillespie]

> This is a misunderstanding: I meant "recycled" as in
> "garbage-collected".  GC in Emacs is supposed to prevent leaks of
> memory and resources.  You seem to be saying that this somehow doesn't
> work in this case.  Can you explain why it doesn't work, and which
> resources specifically appear to be leaking?

Ah. It doesn't work because in this failure mode stderrproc is never gced
because it is still running and attached to a buffer. This is because it is in
a bad state where it cannot exit because it cannot receive a signal from
the non-existent primary process. See the example below where you will
be prompted to kill stderr-buffer after sleeping and gc.

#+begin_src bash
read -r -d '' example <<'EOF'
(progn
  (setq stderr-buffer (generate-new-buffer " rc stderr"))
  (condition-case nil
      (make-process
       :name "process that never actually starts"
       :stderr stderr-buffer
       :query-stderr t
       :command '("i_fail_before_there_can_be_a_return_code"))
    (error t))
  (message "%S" (get-buffer-process stderr-buffer))
  (sleep-for 1)
  (garbage-collect)
  (message "%S" (get-buffer-process stderr-buffer))
  (kill-buffer stderr-buffer))
EOF
emacs -Q -batch -eval "${example}"
#+end_src

> But we have code that errors out in the middle of processing all over
> the place, and that is safe in Emacs, because any unused Lisp objects
> will be GC'ed soon.  Why is this case special?

There are a few reasons why this is special.

0. An internally created stderrproc will never be gced if the parent process
   fails to be created for the reasons listed below.
1. A vfork failure or being passed a command that does not exist causes
   a make-process to fail in a way that a) leaves stderrproc attached to the
   stderr buffer and b) leaves stderrproc in a bad state where it cannot receive
   a signal from the primary process to exit because the primary process
   never makes it into existence
2. Once created, stderrproc is attached to a buffer and that buffer is
    not guaranteed to be gced if the caller e.g. passes in a buffer they
    want to persist.
3. Elisp code can and does interact with stderr buffers that have
    this zombie stderrproc as their buffer process.

> I meant the potential interactions that are not explicitly visible by
> reading the code, but instead stem from system-dependent stuff that is
> related to how subprocesses are created on different systems.

My reading of make-process is that it is impossible for callers in
the elisp universe to see an internally created stderrproc until after
create-process returns so implicit interactions on the elisp side
never happen.

On the C side there are already implicit interactions that exist
and can fail because the original code as written did not account
for them, such as failures during vfork or a command that does
not exist, and this patch prevents those.

To the best of my knowledge the point to which I moved the call
to create stderrproc minimize all possible implicit interactions.

It is a pipe process that nothing else cares about or depends on
until create_process gets stderrproc's stdout file descriptor and
passes it to emacs_spawn as fd_error.

> So I'd still be happier if we could deal with the problem without
> moving chunks of code around.

The alternative is to add code to clean up the stderrproc for any
possible failure during make-process after it has been created,
though I'm not sure that is actually possible.

It is vastly easier, requires much less code, and leaves no doubt
as to whether we missed a call that could fail, to simply move
the creation of stderrproc to a point after all those potential
failure conditions but before it is ever accessed or needed by
other parts of the system.

To quote my internal notes while debugging the issue:

>> there is literally no way to automatically clean up the stderr buffer
>> no matter what you do if there is an error during process creation

Of course the caller could add a call to get-buffer-process and kill the
thing, but there shouldn't even be a process attached to the stderr buffer
because the call to make-process failed.

Without this patch make-process is fundamentally broken and every
caller that wants to clean up the stderr buffer on error but also prompt
on exit would have to know about this issue and call get-buffer-process
on the stderr buffer and then kill that process manually because it is in
a bad state and will not terminate by itself. All this assuming that they
could even figure out what was happening and find this bug report.

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

[Eli Zaretskii]

> From: Tom Gillespie <tgbugs@gmail.com>
> Date: Tue, 9 Aug 2022 11:59:19 -0700
> Cc: larsi@gnus.org, 56002@debbugs.gnu.org
> 
> > This is a misunderstanding: I meant "recycled" as in
> > "garbage-collected".  GC in Emacs is supposed to prevent leaks of
> > memory and resources.  You seem to be saying that this somehow doesn't
> > work in this case.  Can you explain why it doesn't work, and which
> > resources specifically appear to be leaking?
> 
> Ah. It doesn't work because in this failure mode stderrproc is never gced
> because it is still running and attached to a buffer. This is because it is in
> a bad state where it cannot exit because it cannot receive a signal from
> the non-existent primary process. See the example below where you will
> be prompted to kill stderr-buffer after sleeping and gc.

Sorry, I don't understand: stderrproc in this case is not a real
process, it's just a process object.  So why does it need to receive a
signal?

To clean it up, make-process "just" needs to make sure this "process"
is killed and its resources released before it returns unsuccessfully.
Right?

> > I meant the potential interactions that are not explicitly visible by
> > reading the code, but instead stem from system-dependent stuff that is
> > related to how subprocesses are created on different systems.
> 
> My reading of make-process is that it is impossible for callers in
> the elisp universe to see an internally created stderrproc until after
> create-process returns so implicit interactions on the elisp side
> never happen.

That's not what I meant.  I meant the hidden dependencies on the
timing and the order of doing things.

For example, you are talking about vfork all the time, so I presume
you didn't analyze what happens in a build that uses posix_spawn
instead (see emacs_spawn), or when we launch subprocesses on
MS-Windows.  They use different system calls in different orders, and
I worry that we could introduce subtle bugs by rocking this delicate
boat.

> The alternative is to add code to clean up the stderrproc for any
> possible failure during make-process after it has been created,
> though I'm not sure that is actually possible.

Maybe I'm misunderstand something here, but the usual way of doing
that is to use record_unwind_protect immediately after creating the
stderr process, with a suitable unwind function that would perform the
necessary cleanup.  This ensures that however we exit make-process,
the cleanup is never missed, and we don't leak resources.

Why cannot we do this here?  What am I missing?

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

[Tom Gillespie]

So I realized that the current patch only fixes half the problems
but it does fix them completely.

Your concerns are well founded but are only relevant to the half
of the problems that this patch does not fix (more detail below).

Thus, this patch can be merged as is and a separate patch
that uses unwind protect is needed to deal with failures that
happen inside create_process. An additional test is needed
for that as well since the current test only triggers a failure
in Fmake_process prior to the call to create process.

Ideally the existing patches would be merged and I will submit
another one with the fix for issues in create_process. The function
used in unwind protectis going to be more complex and will need
more review so we should not hold up the existing patch to wait
for that one.

> Sorry, I don't understand: stderrproc in this case is not a real
> process, it's just a process object.  So why does it need to receive a
> signal?

Sorry, terminology mixup here. I'm pretty sure that it is the kernel that
is closing the stderrproc file descriptor, which I assume status_notify
or something similar detects and then reaps the stderrproc.

Because the stderrproc file descriptor never makes it to the kernel
that fd can never be closed when the primary process exits so
status_notify (or similar) will see process-status as open forever
unless the user intervenes.

> To clean it up, make-process "just" needs to make sure this "process"
> is killed and its resources released before it returns unsuccessfully.
> Right?

Yes, if you create stderrproc too early. No, if it is never created at a
point where it would have to be cleaned up.

We can avoid any errors caused by failures in Fmake_process
before the call to create_process by moving the creation of
stderrproc as I have done here. That change is safe from
any implicit order dependencies as it is all internal to emacs.

We still need the cleanup code to handle failures inside
the call to create_process. However those should come in
another patch, because the changes are not as simple.

> For example, you are talking about vfork all the time, so I presume
> you didn't analyze what happens in a build that uses posix_spawn
> instead (see emacs_spawn), or when we launch subprocesses on
> MS-Windows.  They use different system calls in different orders, and
> I worry that we could introduce subtle bugs by rocking this delicate
> boat.

As I just realized, this patch only mitigates cases where the
cause of the error was a failure inside Fmake_process. We
need another patch to deal with failures inside create_process
as you suggest.

For the current patch there are no cases where code in Fmake_process
interacts with the operating system in ways that are of concern for
the implicit order dependencies because all implicit os stuff happens
inside create_process (thus the second set of patches).

> Maybe I'm misunderstand something here, but the usual way of doing
> that is to use record_unwind_protect immediately after creating the
> stderr process, with a suitable unwind function that would perform the
> necessary cleanup.  This ensures that however we exit make-process,
> the cleanup is never missed, and we don't leak resources.
>
> Why cannot we do this here?  What am I missing?

We could but do not need to for the issues inside Fmake_process
since we can avoid writing any new code and move the call to
Fmake_pipe_process to immediately before the call to create_process.
There is no risk of unexpected interactions with os conventions
prior to the call to create_process.

For any error happening in create_process before a successful return
from emacs_spawn we do need to use record unwind protect. The
function needed to do that cleanup safely is not as simple and
should be in an independent patch that can be reviewed separately.

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

[Eli Zaretskii]

> From: Tom Gillespie <tgbugs@gmail.com>
> Date: Wed, 10 Aug 2022 19:33:22 -0700
> Cc: larsi@gnus.org, 56002@debbugs.gnu.org
> 
> So I realized that the current patch only fixes half the problems
> but it does fix them completely.
> 
> Your concerns are well founded but are only relevant to the half
> of the problems that this patch does not fix (more detail below).
> 
> Thus, this patch can be merged as is and a separate patch
> that uses unwind protect is needed to deal with failures that
> happen inside create_process. An additional test is needed
> for that as well since the current test only triggers a failure
> in Fmake_process prior to the call to create process.

I'd prefer to have all the cleanup in an unwind function, if that's
possible, because that would allow us not to rearrange the existing
code, and thus would be free of the potential risks I'm worried about.
We could have more than a single unwind function if that is required.

> Ideally the existing patches would be merged and I will submit
> another one with the fix for issues in create_process. The function
> used in unwind protectis going to be more complex and will need
> more review so we should not hold up the existing patch to wait
> for that one.

I'm not bothered with reviewing complex unwind functions, because
their code is local: we need to review that code without thinking
about anything that's not in it.

> > Maybe I'm misunderstand something here, but the usual way of doing
> > that is to use record_unwind_protect immediately after creating the
> > stderr process, with a suitable unwind function that would perform the
> > necessary cleanup.  This ensures that however we exit make-process,
> > the cleanup is never missed, and we don't leak resources.
> >
> > Why cannot we do this here?  What am I missing?
> 
> We could but do not need to for the issues inside Fmake_process
> since we can avoid writing any new code and move the call to
> Fmake_pipe_process to immediately before the call to create_process.

As I explained earlier, I'd prefer not to move the code we already
have.  Thus, basing the cleanups on unwind function is from my POV
preferable.

> There is no risk of unexpected interactions with os conventions
> prior to the call to create_process.

Opening file descriptors and creating Lisp objects in a different
sequence also affects the OS interactions, either indirectly or
directly.

> For any error happening in create_process before a successful return
> from emacs_spawn we do need to use record unwind protect. The
> function needed to do that cleanup safely is not as simple and
> should be in an independent patch that can be reviewed separately.

How about two separate patches, but both based on unwind function(s)
without moving any existing code?  You could make the unwind function
for the first patch simple, and then complicate it in the second
patch.

Thanks.