Bug 435741 - htdig doesn't give a proper error message when failing to execute external parser
htdig doesn't give a proper error message when failing to execute external pa...
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 5
Classification: Red Hat
Component: htdig (Show other bugs)
5.1
All Linux
low Severity low
: rc
: ---
Assigned To: Adam Tkac
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-03 12:08 EST by Gilles Detillieux
Modified: 2013-04-30 19:38 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 678591 (view as bug list)
Environment:
Last Closed: 2009-03-11 12:11:44 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
patch to fix htdig to give proper error message when external_parsers fails (1.24 KB, patch)
2008-03-03 12:08 EST, Gilles Detillieux
no flags Details | Diff

  None (edit)
Description Gilles Detillieux 2008-03-03 12:08:36 EST
Description of problem:
When calling execv to run the parser configured in the external_parsers
attribute in htdig.cong, if the execv fails, htdig doesn't give a proper error
message and doesn't call the right exit function.  So, what happens is it tries
to "eat its own output", as the contents of the stdout buffer that's duplicated
in the child process gets fed back into the external parser input stream.

Version-Release number of selected component (if applicable):
3.2.0b6

How reproducible:
Always

Steps to Reproduce:
1. define external_parsers in htdig.conf, using a program path that doesn't exist
2. run htdig -vvvv
3. look at output
  
Actual results:
(output similar to this for any file types that are to be parsed by an external
parser)
Content-Type: application/pdf
Header line:
returnStatus = 0
Read 8192 from document
Read 8192 from document
Read 8192 from document
Read 8192 from document
Read 907 from document
Read a total of 361355 bytes
word: Read@0
word: 8192@4
word: from@9
word: document@13
word: Read@21
word: 8192@26
word: from@30
word: document@35
word: Read@43
word: 8192@47
word: from@52
word: document@56



Expected results:
Error message such as "Can't execute ..."

Additional info:
This was discussed many times on the htdig-general mailing list, but baffled
everyone.  I finally ran into the problem myself and tracked it down to this
problem.  The patch below is an updated version of the one I wrote in this
mailing list post:

http://sourceforge.net/mailarchive/forum.php?thread_name=200412162240.iBGMewmV015778%40cliff.scrc.umanitoba.ca&forum_name=htdig-general

This updated patch fixes a typo in the earlier patch, another typo in
ExternalParser.cc, as well as fixing a small bug in parsing the external parser
command string.  The patch is for htdig version 3.2.0b6, but the bug is in all
versions of htdig in all Red Hat and Fedora releases.
Comment 1 Gilles Detillieux 2008-03-03 12:08:36 EST
Created attachment 296641 [details]
patch to fix htdig to give proper error message when external_parsers fails
Comment 2 Adam Tkac 2008-04-02 08:34:27 EDT
I think better will be flush buffers before fork(2) and this should be
sufficient, isn't?
Comment 3 Gilles Detillieux 2008-04-02 13:28:05 EDT
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
Comment 4 Adam Tkac 2008-04-23 06:16:44 EDT
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.
Comment 5 Gilles Detillieux 2008-04-23 13:32:11 EDT
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.
Comment 6 Adam Tkac 2008-04-23 14:51:25 EDT
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 :)
Comment 7 RHEL Product and Program Management 2008-07-21 19:05:32 EDT
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 "?".
Comment 13 errata-xmlrpc 2009-03-11 12:11:44 EDT
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

Note You need to log in before you can comment on or make changes to this bug.