r917 - trunk/varnish-cache/bin/varnishd

[phk at projects.linpro.no]

Author: phk
Date: 2006-09-06 09:37:34 +0200 (Wed, 06 Sep 2006)
New Revision: 917

Modified:
   trunk/varnish-cache/bin/varnishd/cache_fetch.c
Log:
Fix a bug in chunked fetching:

With very short chunks, in this case 3 characters, our buffer may
contain not only the chunk length and the chunk data, but also the
next chunk length.

If the short chunk is the last chunk before the zero length chunk
at the end, unconditionally trying to fill the buffer before parsing
the length may hang because we already have everything there is to
have in the buffer.

The fix is to always try to parse the buffer before adding to it.

While here, tighten up and improve error checks of the code.

Reported by: Xing Li <xing at litespeedtech.com>



Modified: trunk/varnish-cache/bin/varnishd/cache_fetch.c
===================================================================
--- trunk/varnish-cache/bin/varnishd/cache_fetch.c	2006-09-06 06:39:52 UTC (rev 916)
+++ trunk/varnish-cache/bin/varnishd/cache_fetch.c	2006-09-06 07:37:34 UTC (rev 917)
@@ -67,86 +67,94 @@
 {
 	int i;
 	char *q;
-	unsigned char *p;
 	struct storage *st;
 	unsigned u, v;
-	char buf[20];
+	char buf[20];		/* XXX: arbitrary */
 	char *bp, *be;
 
-	i = fcntl(fd, F_GETFL);		/* XXX ? */
-	i &= ~O_NONBLOCK;
-	i = fcntl(fd, F_SETFL, i);
-
-	be = buf + sizeof buf;
+	be = buf + sizeof buf - 1;
 	bp = buf;
 	st = NULL;
+	u = 0;
 	while (1) {
-		i = http_Read(hp, fd, bp, be - bp);
-		xxxassert(i >= 0);
-		bp += i;
+		/* Try to parse buf as a chunk length */
 		*bp = '\0';
 		u = strtoul(buf, &q, 16);
-		if (q == NULL || q == buf)
+
+		/* Skip trailing whitespace */
+		if (q != NULL && q > buf) {
+			while (*q == '\t' || *q == ' ')
+				q++;
+			if (*q == '\r')
+				q++;
+		}
+
+		/* If we didn't succeed, add to buffer, try again */
+		if (q == NULL || q == buf || *q != '\n') {
+			xxxassert(be > bp);
+			i = http_Read(hp, fd, bp, be - bp);
+			xxxassert(i >= 0);
+			bp += i;
 			continue;
-		xxxassert(isspace(*q));
-		while (*q == '\t' || *q == ' ')
-			q++;
-		if (*q == '\r')
-			q++;
-		xxxassert(*q == '\n');
+		}
+
+		/* Skip NL */
 		q++;
+
+		/* Last chunk is zero bytes long */
 		if (u == 0)
 			break;
-		sp->obj->len += u;
 
 		while (u > 0) {
-			if (st != NULL && st->len < st->space) {
-				p = st->ptr + st->len;
-			} else {
-				st = stevedore->alloc(stevedore,
-				    stevedore->trim == NULL ? u : CHUNK_PREALLOC);
+
+			/* Get some storage if we don't have any */
+			if (st == NULL || st->len == st->space) {
+				v = u;
+				if (u < CHUNK_PREALLOC && 
+				    stevedore->trim != NULL)
+					v = CHUNK_PREALLOC;
+				st = stevedore->alloc(stevedore, v);
 				XXXAN(st->stevedore);
 				TAILQ_INSERT_TAIL(&sp->obj->store, st, list);
-				p = st->ptr;
 			}
 			v = st->space - st->len;
 			if (v > u)
 				v = u;
 
+			/* Handle anything left in our buffer first */
 			i = bp - q;
 			assert(i >= 0);
-			if (i == 0) {
-			} else if (v > i) {
-				memcpy(p, q, i);
-				p += i;
+			if (i > v)
+				i = v;
+			if (i != 0) {
+				memcpy(st->ptr + st->len, q, i);
 				st->len += i;
+				sp->obj->len += i;
 				u -= i;
 				v -= i;
-				q = bp = buf;
-			} else if (i >= v) {
-				memcpy(p, q, v);
-				p += v;
-				st->len += v;
-				q += v;
-				u -= v;
-				v -= v;
-				if (u == 0 && bp > q) {
-					memcpy(buf, q, bp - q);
-					q = bp = buf + (bp - q);
-				}
+				q += i;
 			}
 			if (u == 0)
 				break;
 			if (v == 0)
 				continue;
+
+			/* Pick up the rest of this chunk */
 			while (v > 0) {
-				i = http_Read(hp, fd, p, v);
+				i = http_Read(hp, fd, st->ptr + st->len, v);
 				st->len += i;
+				sp->obj->len += i;
+				u -= i;
 				v -= i;
-				u -= i;
-				p += i;
 			}
 		}
+		assert(u == 0);
+
+		/* We might still have stuff in our buffer */
+		v = bp - q;
+		if (v > 0)
+			memcpy(buf, q, v);
+		q = bp = buf + v;
 	}
 
 	if (st != NULL && stevedore->trim != NULL)