From d0e96897e8e0dd46fe47e2a1c294bfa4c8afada1 Mon Sep 17 00:00:00 2001
From: Benoit Sigoure <tsuna@stumbleupon.com>
Date: Wed, 18 Aug 2010 19:27:42 -0700
Subject: [PATCH] Properly handle requests with incomplete contents.

When a request specifies a Content-Length, Varnish naturally attempts to
read that many bytes after the end of the HTTP headers.  When some or all
of the bytes were missing, or when the client's connection was reset in
the mean time, Varnish would throw a 503 Service Unavailable Error, and
Varnish would erroneously log a "backend write error".  In addition, when
the problem was due to a malformed request where the Content-Length header
specified the wrong length, Varnish would log "backend write error: 0" as
errno was 0, which is really confusing.  This change makes Varnish handle
properly the various scenarios:
  - There's no more contents to read from the client (read() returned 0
    and errno is set to EAGAIN).  In this case Varnish will keep reading
    from the socket until all the request was successfully read.
  - There's no more contents to read from the client and errno indicates
    that there's nothing else to read anyway (e.g. the because the
    connection was reset or simply because that's the end of the request,
    period).  In this case Varnish now throws a 400 Bad Request Error.
  - We failed to read the contents from the client but it's our fault
    (bad file descriptor for some reason or whatever).  In this case
    we now send a 502 Bad Gateway Error.

In addition, we also check that the Content-Length head is well-formed.
A request with an invalid Content-Length will get a 411 Length Required
Error (to make troubleshooting easier).
---
 bin/varnishd/cache_center.c |    3 +-
 bin/varnishd/cache_fetch.c  |   50 +++++++++++++++++++++++++++++++++++++------
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/bin/varnishd/cache_center.c b/bin/varnishd/cache_center.c
index 2286570..abd078c 100644
--- a/bin/varnishd/cache_center.c
+++ b/bin/varnishd/cache_center.c
@@ -458,7 +458,8 @@ cnt_fetch(struct sess *sp)
 		sp->wrk->bereq = NULL;
 		sp->wrk->beresp = NULL;
 		sp->wrk->beresp1 = NULL;
-		sp->err_code = 503;
+		if (sp->err_code < 400)
+			sp->err_code = 503;
 		sp->step = STP_ERROR;
 		return (0);
 	}
diff --git a/bin/varnishd/cache_fetch.c b/bin/varnishd/cache_fetch.c
index d30f0fa..8dd6266 100644
--- a/bin/varnishd/cache_fetch.c
+++ b/bin/varnishd/cache_fetch.c
@@ -300,15 +300,45 @@ FetchReqBody(struct sess *sp)
 	if (http_GetHdr(sp->http, H_Content_Length, &ptr)) {
 
 		content_length = strtoul(ptr, &endp, 10);
-		/* XXX should check result of conversion */
+		if (ptr == endp  /* Didn't parse any valid digit. */
+		    || (*endp != '\r' && *endp != '\n' && *endp)) {  /* Garbage after last digit. */
+			/* The length is bad, so technically it should be a
+			 * 400 Bad Request Error, but to make debugging easier
+			 * let's serve a 411 (which is uncommon). */
+			sp->err_code = 411;  /* Length Required */
+			return (411);
+		}
 		while (content_length) {
 			if (content_length > sizeof buf)
 				rdcnt = sizeof buf;
 			else
 				rdcnt = content_length;
 			rdcnt = HTC_Read(sp->htc, buf, rdcnt);
-			if (rdcnt <= 0)
+                        if (rdcnt == 0) {
+				/* the last thing HTC_Read does is to call
+				 * read(2), so we assume errno was set by
+				 * read() on sp->htc->fd (the client's FD). */
+				switch (errno) {
+					case ECONNABORTED:
+					case ECONNRESET:
+					case 0:  /* Content-Length says we need to read more but we can't. */
+						sp->err_code = 400;  /* Bad Request */
+						return (400);
+					case EINTR:
+					case EAGAIN:  /* Let's try to read some more. */
+						continue;
+					case EBADF:
+					case EFAULT:
+					case EINVAL:
+					case EIO:
+						sp->err_code = 502;  /* Bad Gateway */
+						return (502);  /* Something bad happened. */
+				}
+			} else if (rdcnt < 0) {
+				WSP(sp, SLT_FetchError, "error FetchReqBody errno=%d rdcnt=%d / content_length=%lu fd=%d",
+				    errno, rdcnt, content_length, sp->htc->fd);
 				return (1);
+                        }
 			content_length -= rdcnt;
 			if (!sp->sendbody)
 				continue;
@@ -320,7 +350,7 @@ FetchReqBody(struct sess *sp)
 	if (http_GetHdr(sp->http, H_Transfer_Encoding, NULL)) {
 		/* XXX: Handle chunked encoding. */
 		WSL(sp->wrk, SLT_Debug, sp->fd, "Transfer-Encoding in request");
-		return (1);
+		return (3);
 	}
 	return (0);
 }
@@ -368,17 +398,23 @@ FetchHdr(struct sess *sp)
 		VBE_AddHostHeader(sp);
 
 	(void)TCP_blocking(vc->fd);	/* XXX: we should timeout instead */
+	/* XXX Shouldn't we do this after calling FetchReqBody? */
 	WRW_Reserve(w, &vc->fd);
 	(void)http_Write(w, hp, 0);	/* XXX: stats ? */
 
 	/* Deal with any message-body the request might have */
-	i = FetchReqBody(sp);
-	if (WRW_FlushRelease(w) || i > 0) {
-		WSP(sp, SLT_FetchError, "backend write error: %d", errno);
+	if ((i = FetchReqBody(sp)) > 0) {
+		WSP(sp, SLT_FetchError, "client read error: errno=%d code=%d", errno, i);
+		WRW_FlushRelease(w);
 		VBE_ClosedFd(sp);
 		/* XXX: other cleanup ? */
 		return (__LINE__);
-	}
+        } else if (i = WRW_FlushRelease(w)) {
+		WSP(sp, SLT_FetchError, "backend write error: errno=%d w->werr=%d", errno, i);
+		VBE_ClosedFd(sp);
+		/* XXX: other cleanup ? */
+		return (__LINE__);
+        }
 
 	/* Checkpoint the shmlog here */
 	WSL_Flush(w, 0);
-- 
1.5.4.3

