Re: [fetchmail-devel] [patch][RFC] proper kill management of the MDA process

[Matthias A. <mat...@gm...>]

Am 06.09.2012 14:12, schrieb Fabio Rossi:
>>> Am 09.03.2012 23:02, schrieb Fabio Rossi:
>>>
>>> Has anyone tried the patch? Is the implementation correct? As it solves a 
>>> problem with the current fetchmail release I think it's a starting point to 
> fix 
>>> the above described problem.
>>
>> Fabio,
>>
>> grazie mille.
>>
>> I've been meaning to look at the patch, but didn't get around to do much
>> fetchmail work in the past months; I have three issues pending
>> work/review for 6.3.22: (a) a change in OpenSSL compatibility options
>> (mostly done, in Git), (b) your MDA kill management (pending review),
>> (c) a fix for --plugin and "-f -" from a Debian user (in the Debian BTS,
>> pending review).
>>
>> Also, some translation updates have accumulated that will be included in
>> the next release.
> 
> Matthias, 
> I have just seen the latest fetchmail 6.3.22/7.0.0 releases but unfortunately 
> the patch hasn't been included. Is there any plan for the next release?

Fabio,

I have been looking at it today. Your approach is the right one, the
devil, however, hides in the details, as your original changelog hinted.

For instance, we lose file descriptors (from mda_pipe) and do not kill
the child if fdopen() or anything else fails during writing.  This would
hurt in daemon mode because we'd then run out of file descriptors sooner
or later if the machine is under load.

Another problem, the exit status is not properly fetched from the child.
It would seem that that the SIGCHLD handler runs asynchronously *before*
we wait_for_child(mda_pid), and with its "while (waitpid(-1, &status,
WONOHANG) > 0) continue;" steals the exit code. Try:

fetchmail --all --fetchlimit 3 --keep --mda "cat >/dev/null && exit 75"

- this "exit 75" goes unnoticed in my testing (it should postpone
messages -- check with the debugger; I have added --keep to protect the
innocent, which will force leaving the messages on the server).

I tend to believe that if fetchmail needs to use a SIGCHLD handler to
reap random (grand-)children, that there are bugs (possibly with
plugin/plugout) where the waitpid() was missing.  The global mda_pid may
be insufficient here, and we may need to pay more attention when we call
waitpid(-1, ...).

It would seem fine to call this at the end of a poll, in daemon-mode.

Also, I find using 0 as "unset" for mda_pid unfortunate, because
waitpid(0, ...) is valid and means "wait for children from our process
group".  Either we need a separate BOOL (flag) to state that mda_pid is
valid, or (what I've currently been experimenting with) use mda_pid =
MDA_PID_UNSET; with enum { MDA_PID_UNSET = 1 }; on the assumption that
we will never wait for the init(8) process (which is special in that it
always has PID #1).

This needs more attention, and I am not sure if I want to integrate this
into 6.3.

Best regards,
Matthias