From d0e96897e8e0dd46fe47e2a1c294bfa4c8afada1 Mon Sep 17 00:00:00 2001 From: Benoit Sigoure 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