pass_chunked()

[Dag-Erling Smørgrav]

"Poul-Henning Kamp" <phk at phk.freebsd.dk> writes:
> Dag-Erling Smørgrav <des at linpro.no> writes:
> > 100% reproducable.  I've found several bugs in pass_chunked().
> I'm not surprised :-)

OK, here's what I found:

> static int
> pass_chunked(struct sess *sp, int fd, struct http *hp)
> {
>         int i, j;
>         char *b, *q, *e;
>         char *p;
>         unsigned u;
>         char buf[PASS_BUFSIZ];
>         char *bp, *be;
>
>         i = fcntl(fd, F_GETFL);         /* XXX ? */
>         i &= ~O_NONBLOCK;
>         i = fcntl(fd, F_SETFL, i);
>
>         bp = buf;
>         be = buf + sizeof buf;
>         p = buf;
>         while (1) {
>                 i = http_Read(hp, fd, bp, be - bp);
>                 i = read(fd, bp, be - bp);

duplicate read, and we discard the data from the first read

>                 assert(i > 0);

wrong - we may still have data in our buffer which needs processing -
in fact, this is almost always the case at the end of the document

>                 bp += i;
>                 /* buffer valid from p to bp */
>
>                 u = strtoul(p, &q, 16);
>                 if (q == NULL || (*q != '\n' && *q != '\r')) {
>                         INCOMPL();
>                         /* XXX: move bp to buf start, get more */

easy enough to implement - memmove(), adjust pointers, continue

>                 }
>                 if (*q == '\r')
>                         q++;
>                 assert(*q == '\n');
>                 q++;
>                 if (u == 0)
>                         break;
>
>                 sp->wrk->acct.bodybytes += WRK_Write(sp->wrk, p, q - p);
>
>                 p = q;
>
>                 while (u > 0) {
>                         j = u;
>                         if (bp == p) {
>                                 bp = p = buf;
>                                 break;
>                         }
>                         if (bp - p < j)
>                                 j = bp - p;
>                         sp->wrk->acct.bodybytes += WRK_Write(sp->wrk, p, j);
>                         p += j;
>                         u -= j;
>                 }

OK so far...

>                 while (u > 0) {
>                         if (http_GetTail(hp, u, &b, &e)) {
>                                 j = e - b;
>                                 sp->wrk->acct.bodybytes +=
>                                     WRK_Write(sp->wrk, q, j);
>                                 u -= j;
>                         } else
>                                 break;
>                 }

Can't see the point of this loop

>                 if (WRK_Flush(sp->wrk))
>                         vca_close_session(sp, "remote closed");
>                 while (u > 0) {
>                         j = u;
>                         if (j > sizeof buf)
>                                 j = sizeof buf;
>                         i = read(fd, buf, j);

should be http_Read()

>                         assert(i > 0);
>                         sp->wrk->acct.bodybytes += WRK_Write(sp->wrk, buf, i);
>                         u -= i;
>                         if (WRK_Flush(sp->wrk))
>                                 vca_close_session(sp, "remote closed");
>                 }

this loop should be merged with the previous one.

>         }
>         return (0);
> }

DES
-- 
Dag-Erling Smørgrav
Senior Software Developer
Linpro AS - www.linpro.no