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

[Tom Gillespie <tgbugs@gmail.com>]

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