[master] 6f0e274 Inspired by #1356: Make handling of req.body much more robust and RFC-compliant.
Poul-Henning Kamp
phk at varnish-cache.org
Thu Oct 24 11:54:27 CEST 2013
commit 6f0e274716b8eac91936fab63b98cb798dd56e72
Author: Poul-Henning Kamp <phk at FreeBSD.org>
Date: Thu Oct 24 09:53:39 2013 +0000
Inspired by #1356: Make handling of req.body much more robust and
RFC-compliant.
Fixes #1356
diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h
index 1ce24fa..b51354d 100644
--- a/bin/varnishd/cache/cache.h
+++ b/bin/varnishd/cache/cache.h
@@ -652,7 +652,6 @@ struct req {
struct {
ssize_t bytes_done;
ssize_t bytes_yet;
- enum {CL, CHUNKED} mode;
} h1; /* HTTP1 specific */
/* The busy objhead we sleep on */
diff --git a/bin/varnishd/cache/cache_http1_fetch.c b/bin/varnishd/cache/cache_http1_fetch.c
index e0b35d8..50c421a 100644
--- a/bin/varnishd/cache/cache_http1_fetch.c
+++ b/bin/varnishd/cache/cache_http1_fetch.c
@@ -240,6 +240,13 @@ V1F_fetch_hdr(struct worker *wrk, struct busyobj *bo, struct req *req)
i = HTTP1_IterateReqBody(req, vbf_iter_req_body, wrk);
if (req->req_body_status == REQ_BODY_DONE)
retry = -1;
+ if (req->req_body_status == REQ_BODY_FAIL) {
+ VSLb(bo->vsl, SLT_FetchError,
+ "req.body read error: %d (%s)",
+ errno, strerror(errno));
+ req->doclose = SC_RX_BODY;
+ retry = -1;
+ }
}
if (WRW_FlushRelease(wrk) || i != 0) {
diff --git a/bin/varnishd/cache/cache_http1_fsm.c b/bin/varnishd/cache/cache_http1_fsm.c
index 85e3022..fcdbebb 100644
--- a/bin/varnishd/cache/cache_http1_fsm.c
+++ b/bin/varnishd/cache/cache_http1_fsm.c
@@ -255,16 +255,11 @@ http1_req_body_status(struct req *req)
return (REQ_BODY_FAIL);
if (req->req_bodybytes == 0)
return (REQ_BODY_NONE);
- req->h1.mode = CL;
req->h1.bytes_yet = req->req_bodybytes - req->h1.bytes_done;
return (REQ_BODY_PRESENT);
}
-
- if (http_GetHdr(req->http, H_Transfer_Encoding, NULL)) {
- req->h1.mode = CHUNKED;
- VSLb(req->vsl, SLT_Debug, "Transfer-Encoding in request");
+ if (http_GetHdr(req->http, H_Transfer_Encoding, NULL))
return (REQ_BODY_FAIL);
- }
return (REQ_BODY_NONE);
}
@@ -274,7 +269,8 @@ http1_req_body_status(struct req *req)
static enum req_fsm_nxt
http1_dissect(struct worker *wrk, struct req *req)
{
- const char *r = "HTTP/1.1 100 Continue\r\n\r\n";
+ const char *r_100 = "HTTP/1.1 100 Continue\r\n\r\n";
+ const char *r_411 = "HTTP/1.1 411 Length Required\r\n\r\n";
char *p;
CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
@@ -308,6 +304,19 @@ http1_dissect(struct worker *wrk, struct req *req)
SES_Close(req->sp, SC_RX_JUNK);
return (REQ_FSM_DONE);
}
+
+ if (req->req_body_status == REQ_BODY_INIT)
+ req->req_body_status = http1_req_body_status(req);
+ else
+ assert(req->req_body_status == REQ_BODY_NONE); // ESI
+
+ if (req->req_body_status == REQ_BODY_FAIL) {
+ wrk->stats.client_req_411++;
+ (void)write(req->sp->fd, r_411, strlen(r_411));
+ SES_Close(req->sp, SC_RX_JUNK);
+ return (REQ_FSM_DONE);
+ }
+
req->acct_req.req++;
req->ws_req = WS_Snapshot(req->ws);
@@ -318,7 +327,8 @@ http1_dissect(struct worker *wrk, struct req *req)
if (strcasecmp(p, "100-continue")) {
wrk->stats.client_req_417++;
req->err_code = 417;
- } else if (strlen(r) != write(req->sp->fd, r, strlen(r))) {
+ } else if (strlen(r_100) !=
+ write(req->sp->fd, r_100, strlen(r_100))) {
SES_Close(req->sp, SC_REM_CLOSE);
return (REQ_FSM_DONE);
}
@@ -328,10 +338,6 @@ http1_dissect(struct worker *wrk, struct req *req)
wrk->stats.client_req++;
http_Unset(req->http, H_Expect);
- if (req->req_body_status == REQ_BODY_INIT)
- req->req_body_status = http1_req_body_status(req);
- else
- assert(req->req_body_status == REQ_BODY_NONE);
assert(req->req_body_status != REQ_BODY_INIT);
@@ -437,26 +443,23 @@ http1_iter_req_body(struct req *req, void *buf, ssize_t len)
{
CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
- if (req->h1.mode == CL) {
- AN(req->req_bodybytes);
- AN(len);
- AN(buf);
- if (len > req->req_bodybytes - req->h1.bytes_done)
- len = req->req_bodybytes - req->h1.bytes_done;
- if (len == 0) {
- req->req_body_status = REQ_BODY_DONE;
- return (0);
- }
- len = HTTP1_Read(req->htc, buf, len);
- if (len <= 0) {
- req->req_body_status = REQ_BODY_FAIL;
- return (-1);
- }
- req->h1.bytes_done += len;
- req->h1.bytes_yet = req->req_bodybytes - req->h1.bytes_done;
- return (len);
+ AN(req->req_bodybytes);
+ AN(len);
+ AN(buf);
+ if (len > req->req_bodybytes - req->h1.bytes_done)
+ len = req->req_bodybytes - req->h1.bytes_done;
+ if (len == 0) {
+ req->req_body_status = REQ_BODY_DONE;
+ return (0);
}
- INCOMPL();
+ len = HTTP1_Read(req->htc, buf, len);
+ if (len <= 0) {
+ req->req_body_status = REQ_BODY_FAIL;
+ return (-1);
+ }
+ req->h1.bytes_done += len;
+ req->h1.bytes_yet = req->req_bodybytes - req->h1.bytes_done;
+ return (len);
}
/*----------------------------------------------------------------------
@@ -513,6 +516,7 @@ HTTP1_IterateReqBody(struct req *req, req_body_iter_f *func, void *priv)
do {
l = http1_iter_req_body(req, buf, sizeof buf);
if (l < 0) {
+ req->doclose = SC_RX_BODY;
return (l);
}
if (l > 0) {
@@ -548,6 +552,8 @@ HTTP1_DiscardReqBody(struct req *req)
if (req->req_body_status == REQ_BODY_DONE)
return(0);
+ if (req->req_body_status == REQ_BODY_FAIL)
+ return(0);
if (req->req_body_status == REQ_BODY_TAKEN)
return(0);
return(HTTP1_IterateReqBody(req, httpq_req_body_discard, NULL));
@@ -568,8 +574,11 @@ HTTP1_CacheReqBody(struct req *req, ssize_t maxsize)
CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
+ assert (req->req_step == R_STP_RECV);
switch(req->req_body_status) {
case REQ_BODY_CACHED:
+ case REQ_BODY_FAIL:
+ return (-1);
case REQ_BODY_NONE:
return (0);
case REQ_BODY_PRESENT:
@@ -599,8 +608,10 @@ HTTP1_CacheReqBody(struct req *req, ssize_t maxsize)
l = st->space - st->len;
l = http1_iter_req_body(req, st->ptr + st->len, l);
- if (l < 0)
+ if (l < 0) {
+ req->doclose = SC_RX_BODY;
return (l);
+ }
if (req->req_bodybytes > maxsize) {
req->req_body_status = REQ_BODY_FAIL;
return (-1);
diff --git a/bin/varnishd/cache/cache_req_fsm.c b/bin/varnishd/cache/cache_req_fsm.c
index c20aa83..fe089cd 100644
--- a/bin/varnishd/cache/cache_req_fsm.c
+++ b/bin/varnishd/cache/cache_req_fsm.c
@@ -693,6 +693,11 @@ cnt_recv(struct worker *wrk, struct req *req)
http_CollectHdr(req->http, H_Cache_Control);
VCL_recv_method(req->vcl, wrk, req, NULL, req->http->ws);
+
+ /* Attempts to cache req.body may fail */
+ if (req->req_body_status == REQ_BODY_FAIL) {
+ return (REQ_FSM_DONE);
+ }
recv_handling = wrk->handling;
if (cache_param->http_gzip_support &&
diff --git a/bin/varnishtest/tests/c00055.vtc b/bin/varnishtest/tests/c00055.vtc
index ff13f66..4ecbe35 100644
--- a/bin/varnishtest/tests/c00055.vtc
+++ b/bin/varnishtest/tests/c00055.vtc
@@ -30,3 +30,19 @@ client c1 {
expect resp.http.Foo == "Foo"
expect resp.bodylen == 2
} -run
+
+client c1 {
+ txreq -req POST -nolen -hdr "Content-Length: 52"
+ delay .3
+} -run
+
+server s1 {
+ rxreq
+ txresp
+} -start
+
+client c1 {
+ txreq -url "/is_varnish_still_running"
+ rxresp
+ expect resp.status == 200
+} -run
diff --git a/bin/varnishtest/tests/r01356.vtc b/bin/varnishtest/tests/r01356.vtc
new file mode 100644
index 0000000..f0c00b2
--- /dev/null
+++ b/bin/varnishtest/tests/r01356.vtc
@@ -0,0 +1,37 @@
+varnishtest "#1356, req.body failure"
+
+server s1 {
+ rxhdrs
+ expect_close
+} -start
+
+varnish v1 -vcl+backend { } -start
+
+client c1 {
+ txreq -req POST -nolen -hdr "Transfer-Encoding: carrier-pigeon"
+ rxresp
+ expect resp.status == 411
+} -run
+
+client c1 {
+ txreq -req POST -nolen -hdr "Content-Length: carrier-pigeon"
+ rxresp
+ expect resp.status == 411
+} -run
+
+client c1 {
+ txreq -req POST -nolen -hdr "Content-Length: 56"
+} -run
+
+# Check that varnishd still runs
+
+server s1 {
+ rxreq
+ txresp
+} -start
+
+client c1 {
+ txreq
+ rxresp
+ expect resp.status == 200
+} -run
diff --git a/include/tbl/sess_close.h b/include/tbl/sess_close.h
index 6246a51..b132b39 100644
--- a/include/tbl/sess_close.h
+++ b/include/tbl/sess_close.h
@@ -32,6 +32,7 @@
SESS_CLOSE(REM_CLOSE, "Client Closed")
SESS_CLOSE(REQ_CLOSE, "Client requested close")
SESS_CLOSE(REQ_HTTP10, "proto < HTTP.1.1")
+SESS_CLOSE(RX_BODY, "Failure receiving req.body")
SESS_CLOSE(RX_JUNK, "Received junk data")
SESS_CLOSE(RX_OVERFLOW, "Received buffer overflow")
SESS_CLOSE(RX_TIMEOUT, "Receive timeout")
diff --git a/include/tbl/vsc_f_main.h b/include/tbl/vsc_f_main.h
index 51ad0f6..825d73e 100644
--- a/include/tbl/vsc_f_main.h
+++ b/include/tbl/vsc_f_main.h
@@ -101,6 +101,11 @@ VSC_F(client_req_400, uint64_t, 1, 'a', info,
" malformed in some drastic way."
)
+VSC_F(client_req_411, uint64_t, 1, 'a', info,
+ "Client requests received, subject to 411 errors",
+ "411 means the client did not send a Content-Lenght for the req.body."
+)
+
VSC_F(client_req_413, uint64_t, 1, 'a', info,
"Client requests received, subject to 413 errors",
"413 means that HTTP headers execeeded length or count limits."
More information about the varnish-commit
mailing list