Streaming NNTP bug (redux)

[Julien ÉLIE]

Hi River,

> Jan 11 22:58:39 INFO:     feeder: enfer-du-nord[87.98.157.95]:119: invalid response from command [.438<jektk2$7ja$1 at dont-email.me>]
> Jan 11 23:04:17 INFO:     feeder: tomockey[220.157.151.83]:119: invalid response from command [y438<DkM4jRgkX7fAHPAXlidGc.0.gpbBcspOKKMq7MNTw.AHtQ at spot.net>]
> 
> I don't have any more details, but if I can reproduce it on my own
> INN server again I'll investigate.

Thanks for the head up.

I wonder whether this bug could not be triggered by the following code
in innd/nc.c:

#define WCHANappend(cp, p, l)   buffer_append(&(cp)->Out, (p), (l))

cp->Out->data is a buffer containing the data to write
cp->Out->used is the position of the next byte in cp->Out->data to write
cp->Out->left is the number of bytes waiting to be sent

If the write() function fails, "i" is negative and we substract "i"
to cp->Out->left anyway.
So if for instance i=-1, we end up in having cp->Out->left increased
by one.  Which would, I believe, generate the bug you see:  an extra
character is added after a complete response.  (So it appears in fact
before a new response, just before 438 in this case.)



void
NCwritereply(CHANNEL *cp, const char *text)
{
    struct buffer *bp;
    int i;

    bp = &cp->Out;
    i = bp->left;
    WCHANappend(cp, text, strlen(text));        /* Text in buffer. */
    WCHANappend(cp, NCterm, strlen(NCterm));    /* Add CR LF to text. */

    if (i == 0) {       /* If only new data, then try to write directly. */
        i = write(cp->fd, &bp->data[bp->used], bp->left);
        if (Tracing || cp->Tracing)
            syslog(L_TRACE, "%s NCwritereply %d=write(%d, \"%.15s\", %lu)",
                CHANname(cp), i, cp->fd, &bp->data[bp->used],
                (unsigned long) bp->left);
        if (i > 0)
            bp->used += i;
        if (bp->used == bp->left) {
            /* All the data was written. */
            bp->used = bp->left = 0;
            NCwritedone(cp);
        } else {
            bp->left -= i;
            i = 0;
        }
    } else {
        i = 0;
    }
    if (i <= 0) {       /* Write failed, queue it for later. */
        WCHANadd(cp);
    if (Tracing || cp->Tracing)
        syslog(L_TRACE, "%s > %s", CHANname(cp), text);
}






Having a look at the history of innd/nc.c, the addition of the substraction
was in INN 2.4.3 to also fix random junk.
  http://inn.eyrie.org/trac/changeset/7418

I therefore think it safe to just add:

+if (i > 0) {
  bp->left -= i;
+}


% grep -r left * | grep -- '-='
shows bits of code in innd/site.c and innd/chan.c where this condition
about "i" is added.

As I do not have a clue about how to reliably test this change, please
tell me if you do think this change is wrong.  Otherwise, I will commit it.

-- 
Julien ÉLIE

« Le carré est une figure qui a un angle droit dans chaque coin. »
  (Jean-Charles)