Re: [Nmh-workers] outstanding patches

[Robert Elz]

    Date:        Thu, 26 May 2005 17:47:02 +0200
    From:        Harald Geyer <address@hidden>
    Message-ID:  <address@hidden>

  | Ok, then this should be changed too, in case anybody wants to keep
  | vfork().

Yes, that ought be changed, though in practice the one in question
should not really matter a lot, as it only happens if the exec fails,
which should ordinarily not happen.

But, aside from the fprintf, ...

  | But I still think it's better to switch to fork() - nmh doesn't
  | obey the rules necessary to use vfork() even if it isn't that bad as
  | it seemed to me in the beginning.

I saw nothing in replsbr.c's use of vfork() (the bug referred to in the
Debian bug report) that was incorrect - all that happens, assuming the
exec succeeds, is that file descriptors are dup'd and closed.   That
kind of thing is exactly what is normally planned to happen between
vfork() and exec (and it is precisely because it is quite hard to
specify exactly what should be done, in any way other than C code, that
makes spawn() kind of difficult).

On the other hand, I also saw nothing there that would actually print
an error message on file system full (the trigger condition for the
bug report).   There's certainly an fflush() that could fail, and perhaps
cause errno to get set, which the code in the vfork() case could
then alter - but if nmh is expecting errno to remain unchanged from
that fflush() (just before the vfork()) to some undisclosed later
place, presumably where a ferror() test is done, and an error printed),
then nmh is entirely deluded.   Aside from the vfork() the parent code
also calls pidXwait() (which is pitstatus(pidwait(...)) and fseek()
after the fflush before replfilter() ends (and the error message must
occur somewhere later).   pidwait() does waitpid() (which might alter errno).
and sometimes, signal() (which also might alter errno), pidstatus()
can call fprintf() ...    After all of that, expecting errno to be
unchanged is just plain wrong.

Changing vfork() into fork() may just happen to remove one cause of
errno being altered altered, but if none other happens, that's pure fluke.

The real bug here is in using errno (directly, or via perror()) in
an environment where errno's value cannot be predicted, which is
what seems to be happening.   It is also a very common programming bug.

That is what should really be fixed.

Changing vfork() into fork() as a general crusade against vfork() is
OK too, just don't pretend that is being done as a solution to the
reported bug, as it certainly is not.

kre