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.