Bug Fixes for inn221

David Shrimpton shrimpto at its.uq.edu.au
Tue Oct 26 08:34:24 UTC 1999


Hi All, 

I'm writing to send in some bug reports and fixes for inn 2.2.1 
in case they haven't been incorporated in later releases. 



1. Bug in errmsg subroutine in pgpverify which causes pgpverifies 
to be syslogged with undefined facility.

Line 355 

      ($facility, $level) = ($syslog =~ /^(w+)\.(\w+)$/);

should be

      ($facility, $level) = ($syslog =~ /^(\w+)\.(\w+)$/);

2. Bug in innconfval.c which causes 
   "Reading config from /usr/local/news/etc/inn.conf" messages 
   to be syslogged with undefined facility.


In main in innconfval.c there is a call to openlog with facility
LOG_INN_PROG which is undefined.  Hence each time inn.conf is read,
and the message "Reading config from /usr/local/news/etc/inn.conf"
is logged to syslog then the message doen't log to the news facility
ie news.notice but logs as *.DEBUG to wherever that is logged by
syslog.conf.  (Turns up in our misc/misclog ) (inn.conf is read
when innconfval is called , eg by batcher. )

Offending code in innconfval.c is:

main(int ac, char **av)
{
    char        *p;
    char        *val;
    BOOL        File;
    int i;

    /* First thing, set up logging and our identity. */
    openlog("innconfval", L_OPENLOG_FLAGS | LOG_PID, LOG_INN_PROG);

...


This can be fixed by #include "config.h" in innconfval.c  which defines 
LOG_INN_PROG.

I have also added a closelog at the end of main to politely close
the syslogging before program exit.


3. Bug in batcher.c which causes  
   "Reading config from /usr/local/news/etc/inn.conf" messages
   to be syslogged with undefined facility.

This bug is at the start of main in batcher.c.
There is a call to ReadInnConf which trys to syslog a Reading conf file
message.  However on our system if there has been no openlog the syslog
call logs as *.DEBUG and ends up wherever this is directed by syslog.conf
and not to LOG_NEWS ( news.notice )

This may only apply to Digital Unix where I believe you need to 
do the openlog calls before any syslog call.


Fix is to openlog before ReadInnConf and closelog after eg.

...
    struct stat Sb;
    BOOL Token;
    QIOSTATE    *qp;

    /* Set defaults. */
    /* ReadInnConf does a syslog at LOG_DEBUG so need to
       have opened a log first or get syslog mesgs with no identiy
       "Reading ..." . Ctlinnd nnrpd will log these mesgs properly */
    (void)openlog("batcher", L_OPENLOG_FLAGS | LOG_PID, LOG_INN_PROG);
    if (ReadInnConf() < 0) exit(1);
    closelog();
    AltSpool = NULL;
    Redirect = TRUE;
    (void)umask(NEWSUMASK);

4.  This is a long story.  This dropped lines bug has been present 
    since inn172 at least and we have previously advised about it but it is 
    still present.  I've thrown in  a fix for the -a switch which sends 
    one less article than it should and some more precise calculations 
    for ending batches.  There is also a potential problem with
    one line input files.

The major bug is:

The batcher fails to process the last article line read from
the input file and also fails to requeue this input line when 
encountering an error condition and also when exiting if the 
maximum total number of articles or bytes for all batches combined is exceeded. 

This error occurs when the main loop is exited at various points 
by a break; statement without the current article having been sent.
The batcher at this stage has already read an input line and should 
be calling RequeueAndExit with that line rather than with a (char *)NULL.
This line is then never requeued and is lost.  (RequeueAndExit is correctly
called in some places but not others.)

The folowing are suggested fixes for these bugs:


4.1  I think BATCHstatus should be initialized to -1 in main.  Other
-wise If we RequeueAndExit before opening a batch, a one line input file will 
be discarded in the event that the undefined BATCHstatus tests to 0 in 
RequeueAndExit function.  I think BATCHcount could be initialized to 0 
here also. 

4.2 Replace RequeueAndExit(Cookie, (char *)NULL, 0L); with break ; at line 448

Replace:

        /* Have an open article, do we need to open a batch?  This code
         * is here (rather then up before the while loop) so that we
         * can avoid sending an empty batch.  The goto makes the code
         * a bit more clear. */
        if (F == NULL) {
            if (GotInterrupt) {
                if (Token)
                    QIOclose(qp);
                else
                    (void)close(artfd);
                RequeueAndExit(Cookie, (char *)NULL, 0L);
            }
            if ((F = BATCHstart()) == NULL) {

With:

        /* Have an open article, do we need to open a batch?  This code
         * is here (rather then up before the while loop) so that we
         * can avoid sending an empty batch.  The goto makes the code
         * a bit more clear. */
        if (F == NULL) {
            if (GotInterrupt) {
                if (Token)
                    QIOclose(qp);
                else
                    (void)close(artfd);

/*
                Need to pass line to RequeueAndExit so
                it can requeue the line we just read.  So break to
                end of while and RequeueAndExit with line and BytesInArt
----->          RequeueAndExit(Cookie, (char *)NULL, 0L);
*/
----->          break ;
            }
            if ((F = BATCHstart()) == NULL) {



A line has already been read from input file at this stage so if GotInterrupt
then calling RequeueAndExit with line (char *)NULL will result in loss
of this line.  It won't be requeued. Fix by break; to a RequeueAndExit 
at the end with line set to the line we have just read.  Line will be requeued



4.3  Fix so that -a arts sends (arts) and not (arts - 1) articles in batch .
Also so that a new batch is started if adding 
 ( the bytes in separator + bytes in article )  instead of ( bytes in article)
to current batch will cause it to exceed the batch bytes limit.  (This is being
a bit picky, I know. )


Replace:

        /* We're writing a batch, see if adding the current article
         * would exceed the limits. */
        if ((ArtsInBatch > 0 && ArtsInCB + 1 >= ArtsInBatch)
         || (BytesInBatch > 0 && BytesInCB + BytesInArt >= BytesInBatch)) {
            if ((BATCHstatus = BATCHclose(F)) != 0) {


With:

/*
        Fix so sends ArtsInBatch not ArtsInBatch - 1
        and starts a new batch only if writing BytesInCB + BytesInArt +
        bytes in the separator line (#! rnews) will exceed BytesInBatch

        if ((ArtsInBatch > 0 && ArtsInCB + 1 >= ArtsInBatch)
         || (BytesInBatch > 0 && BytesInCB + BytesInArt >= BytesInBatch)) {
*/
        if (Separator && *Separator) {
            (void)sprintf(buff, Separator, BytesInArt);
        }

        if ((ArtsInBatch > 0 && ArtsInCB >= ArtsInBatch)
         || (BytesInBatch > 0 && BytesInCB + strlen(buff) + 1 + BytesInArt > BytesInBatch
)) {
            if ((BATCHstatus = BATCHclose(F)) != 0) {





4.4 Add the separator bytes to the current article bytes to see if Max Bytes 
will be exceeded

Replace:

            /* See if we can start a new batch. */
            if ((MaxBatches > 0 && BATCHcount >= MaxBatches)
             || (MaxBytes > 0 && BytesWritten + BytesInArt >= MaxBytes)
             || (MaxArts > 0 && ArtsWritten + 1 >= MaxArts)) {
                if (Token)

With:

/*
            Don't start a new batch if another article
            will push us over MaxArts.
            Note MaxArts is only tested when starting new batches so
            could go over MaxArts if finishing the batch writes enough
            articles.  You'd need a test of MaxArts after each
            article write to change this.  MaxBytes can also get exceeded
            if you write enough articles after starting a new batch.

            if ((MaxBatches > 0 && BATCHcount >= MaxBatches)
             || (MaxBytes > 0 && BytesWritten + BytesInArt >= MaxBytes)
             || (MaxArts > 0 && ArtsWritten + 1 >= MaxArts)) {

*/
            /* See if we can start a new batch. */
            if ((MaxBatches > 0 && BATCHcount >= MaxBatches)
             || (MaxBytes > 0 && BytesWritten + strlen(buff) + 1 + BytesInArt > MaxBytes)
             || (MaxArts > 0 && ArtsWritten + 1 > MaxArts)) {
                if (Token)




4.5  Leave a correct RequeueAndExit call to the end of loop.

line 478

Replace :

            if (GotInterrupt) {
                if (Token)
                    QIOclose(qp);
                else
                    (void)close(artfd);
                RequeueAndExit(Cookie, line, BytesInArt);


With:

            if (GotInterrupt) {
                if (Token)
                    QIOclose(qp);
                else
                    (void)close(artfd);
/*
            break and RequeueAndExit at end
                RequeueAndExit(Cookie, line, BytesInArt);
*/
                break;




Line 545 Same again

Replace:

        if (GotInterrupt) {
            BATCHstatus = BATCHclose(F);
            RequeueAndExit(Cookie, line, BytesInArt);
        }

With:

        if (GotInterrupt) {
            BATCHstatus = BATCHclose(F);
/*
            RequeueAndExit(Cookie, line, BytesInArt);
*/
            break;
        }

line 551  Replace another correct RequeueAndExit with a break and
Fix the RequeueAndExit at the end of the file so that it is called
with line and not NULL.

Replace :

        if (GotInterrupt) {
            BATCHstatus = BATCHclose(F);
            RequeueAndExit(Cookie, line, BytesInArt);
        }
    }

    if (BATCHopen)
        BATCHstatus = BATCHclose(F);
    RequeueAndExit(Cookie, (char *)NULL, 0L);
    /* NOTREACHED */
    return 0;
}

With:

        if (GotInterrupt) {
            BATCHstatus = BATCHclose(F);
/*
            RequeueAndExit(Cookie, line, BytesInArt);
*/
            break;
        }
    }

    if (BATCHopen)
        BATCHstatus = BATCHclose(F);
/*
    Requeue with line so we don't lose line
    RequeueAndExit(Cookie, (char *)NULL, 0L);
*/
    RequeueAndExit(Cookie, line, BytesInArt);
    /* NOTREACHED */
    return 0;
}





Appendix.

Bugs relate to inn 221 compiled using DEC C V5.6-071 on 
Digital UNIX V4.0 (Rev. 878)  

We use traditional storage method.



Sorry to be so verbose.  The lost line bug in the batcher
is  easy to demo by having a small input file and running 
batcher with -B total_size or -A total_arts to a value so 
that they will be exceeded at some point and the remainder
of the input file requeued.  You always lose an article 
line. 



-- 
David Shrimpton                       Systems Programmer
Software Infrastructure, Information Technology Services   
University of Qld 4072            shrimpto at its.uq.edu.au
Brisbane Australia                 Phone  61 7 3365 4012




More information about the inn-bugs mailing list