Streaming NNTP bug (redux)

[Russ Allbery]

Julien ÉLIE <julien at trigofacile.com> writes:

> 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.)

Good catch!  That code indeed looks very suspect.

> 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;

Rather than this, I think the rest of the body of this function should be
conditional on i > 0 and not do any of the rest of this if i <= 0.

> 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;
> +}

That would also work, yeah.

-- 
Russ Allbery (rra at stanford.edu)             <http://www.eyrie.org/~eagle/>

    Please send questions to the list rather than mailing me directly.
     <http://www.eyrie.org/~eagle/faqs/questions.html> explains why.