Bug 435741

Created attachment 296641 [details]
patch to fix htdig to give proper error message when external_parsers fails

I think better will be flush buffers before fork(2) and this should be
sufficient, isn't?

Well, flushing the buffers before the fork call would prevent the output from
being fed back to the parent process, but that would just be a partial solution.
 The point is that a child process, after a fork call, should _never_ call the
regular exit(3) function.  It _must_ call the _exit(2) call that doesn't touch
any stdio buffers, because you don't really know what files may or may not be
open at that time.  The exit() call in the child process was just plain wrong,
and should be fixed regardless of any fflush() calls you add before the fork().
 Secondly, the other problem was the lack of error message if the execv() call
fails.  My patch addresses both of these problems.  Adding some fflush() calls
before the fork() wouldn't hurt, but doing that alone is definitely not
sufficient, and not better than putting the proper fallback calls after the execv.

It's a shame the Linux man pages for exit(3) and _exit(2) don't make the
distinction more clearly, because there seems to be a lot of confusion about the
proper way to use these.  When the stdio functions were first introduced to the
UNIX C library (back in the days of 7th Edition UNIX, on which I first cut my
teeth), the man pages stressed the importance of this distinction better than
current man pages do.  The point is that after a fork(), the child process
shouldn't make too many assumptions about what it inherits from the parent in
its copy of the parent's memory image, and so it should avoid tampering with
that copy as much as possible, especially the I/O buffers.

A good on-line example is available here:
http://www.cs.uleth.ca/~holzmann/C/system/pipeforkexec.html

Hm, generally I think _exit should not be used and every file descriptor which
should not be duplicated to child should have FD_CLOEXEC flag set. And
std{io,out} should be fflush(3)-ed so _exit is not needed. But in this case it
looks that upstream source is quite broken so I'm going to use your patch.
Thanks for it.

Adam, you are quite right that *generally* the _exit system call should not be
used, because *generally* you *want* the atexit()/on_exit() processing (such as
output buffer flushing) to take place when the process exits.  But, this is not
a general case.  This is a very, very exceptional case, where you do not, under
any circumstances, want this processing to take place.

Allow me to spell out explicitly why this is such an exceptional case, requiring
exceptional handling, since you seem to have a lot of reluctance to accept the
exceptional nature of it.  We have a situation where a process does a fork()
call, which creates a second process (the child process) running the exact same
code as the original, parent process, and receiving an exact duplicate of the
parent process's data memory.  The only difference is the return code that
fork() returns to the parent and child processes.  The child process is created
for one purpose only: to set up file descriptors, signal handling, and
environment to run a new executable program, which it attempts to run via the
execv() system call.  If this execv() call fails, then the child process's only
reason for being created in the first place has ceased to exist, and so the
child process *must* try to go away causing as little disruption as possible. 
The best way to do this is to put out a simple error message to stderr (or its
file descriptor), and go away without touching any memory structures that don't
rightly belong to it and that it isn't coded to know about.  All these memory
structures, including unflushed output buffers, are just copies inherited from
the parent process, and it's the parent process that will end up doing the
appropriate atexit()/on_exit() processing on its copy when it exits.  It would
be quite inappropriate for the child process to attempt any such processing on
its copy of these -- they should remain untouched until the OS reclaims the memory.

Now, if the execv() had succeeded, that's pretty much exactly what would happen
to the child process's memory.  It would have been entirely replaced with the
code and data images from the executable that the OS would have found and
loaded, and the duplicated memory image from the parent would have disappeared,
*without* flushing copied buffers or any other atexit()/on_exit() processing. 
If that's what happens when execv() succeeds, it only stands to reason that the
same thing should happen when execv() fails.  It's exactly for exceptional cases
like this that the _exit() system call is available to programmers.  This is
probably the one sort of case where a programmer can, and *should*, use _exit().

There are probably many things you could do in the parent process, before
calling fork(), that would make things safer in the child process, and certainly
flushing buffers before the fork() is not a bad thing to do.  But doing only
that and not doing the *right thing* in the child process is just bad
programming.  It requires that you keep track of *everything* that's going on in
the parent process so that you can flush, close, reset or whatever else needs to
be set up ahead of time so that the child process's *inappropriate* use of
atexit()/on_exit() processing will not cause any trouble for you.  What's more,
any future changes to the htdig code which would introduce a new output stream
or any other new atexit() processing would require a corresponding fix to this
code before the fork() to prevent any new problems from coming up.  That's not
defensive programming.  And to what end?  All to avoid using the _exit() system
call in exactly the sort of situation where it's appropriate to use it.

If that's still not enough to convince you, consider this too: my patch was
written by someone who spent a lot of time squashing bugs in htdig (I was a
developer for ht://Dig for several years), it's been tested, and it works as
advertised.  It's also gone into the ht://Dig CVS repository, but I sent you the
patch because there doesn't seem to be an imminent release of a new upstream
version any time soon.  (Current developers seem to be busy with version 4 which
will be a fairly major rewrite.)

I hope this helps.

Yes, I know you are member of upstream. I didn't have reluctance with that
patch, I only pointed if _exit is really needed. From your detailed description
it is really clear for me now that _exit is better than exit. Btw I know you
have far more knowledge about htdig than me :) Patch is currently applied in F9
and rawhide branches. Again, thanks for it :)

This request was evaluated by Red Hat Product Management for
inclusion, but this component is not scheduled to be updated in
the current Red Hat Enterprise Linux release. If you would like
this request to be reviewed for the next minor release, ask your
support representative to set the next rhel-x.y flag to "?".

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2009-0291.html