[6.0] a45e1f4e4 Fail fetch retries when uncached request body has been released

Reza Naghibi reza at naghibi.com
Wed Aug 19 13:12:07 UTC 2020


commit a45e1f4e49dbd6949659e9bb9ddfd2cdf9f6a479
Author: Martin Blix Grydeland <martin at varnish-software.com>
Date:   Thu Oct 10 14:30:18 2019 +0200

    Fail fetch retries when uncached request body has been released
    
    Currently we allow fetch retries with body even after we have released the
    request that initiated the fetch, and the request body with it. The
    attached test case demonstrates this, where s2 on the retry attempt gets
    stuck waiting for 3 bytes of body data that is never sent.
    
    Fix this by keeping track of what the initial request body status was, and
    failing the retry attempt if the request was already released
    (BOS_REQ_DONE) and the request body was not cached.
    
    Conflicts:
        bin/varnishd/cache/cache.h

diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h
index 9e47c7215..cb4f1ba79 100644
--- a/bin/varnishd/cache/cache.h
+++ b/bin/varnishd/cache/cache.h
@@ -390,7 +390,8 @@ struct busyobj {
 	 * All fields from retries and down are zeroed when the busyobj
 	 * is recycled.
 	 */
-	int			retries;
+	unsigned		retries;
+	enum req_body_state_e	initial_req_body_status;
 	struct req		*req;
 	struct sess		*sp;
 	struct worker		*wrk;
diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index 0d794c836..9962e1c9e 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/bin/varnishd/cache/cache_fetch.c
@@ -242,6 +242,7 @@ vbf_stp_mkbereq(struct worker *wrk, struct busyobj *bo)
 	bo->ws_bo = WS_Snapshot(bo->ws);
 	HTTP_Copy(bo->bereq, bo->bereq0);
 
+	bo->initial_req_body_status = bo->req->req_body_status;
 	if (bo->req->req_body_status == REQ_BODY_NONE) {
 		bo->req = NULL;
 		ObjSetState(bo->wrk, bo->fetch_objcore, BOS_REQ_DONE);
@@ -262,6 +263,15 @@ vbf_stp_retry(struct worker *wrk, struct busyobj *bo)
 
 	assert(bo->fetch_objcore->boc->state <= BOS_REQ_DONE);
 
+	if (bo->fetch_objcore->boc->state == BOS_REQ_DONE &&
+	    bo->initial_req_body_status != REQ_BODY_NONE &&
+	    bo->bereq_body == NULL) {
+		/* We have already released the req and there was a
+		 * request body that was not cached. Too late to retry. */
+		VSLb(bo->vsl, SLT_Error, "req.body already consumed");
+		return (F_STP_FAIL);
+	}
+
 	VSLb_ts_busyobj(bo, "Retry", W_TIM_real(wrk));
 
 	/* VDI_Finish (via vbf_cleanup) must have been called before */
diff --git a/bin/varnishtest/tests/r03093.vtc b/bin/varnishtest/tests/r03093.vtc
new file mode 100644
index 000000000..f9aaa2a7c
--- /dev/null
+++ b/bin/varnishtest/tests/r03093.vtc
@@ -0,0 +1,45 @@
+varnishtest "r03093 - fail retry on missing req body"
+
+barrier b1 sock 2
+
+server s1 {
+	rxreq
+	expect req.method == POST
+	expect req.body == foo
+	txresp -nolen -hdr "Content-Length: 3"
+	barrier b1 sync
+} -start
+
+# In this test s2 should not be called. The attempt to retry should fail because
+# the request was already released from the fetch thread in the first attempt.
+server s2 {
+	rxreq
+	expect req.method == POST
+	expect req.body == foo
+	txresp -body bar
+} -start
+
+varnish v1 -arg "-p debug=+syncvsl" -vcl+backend {
+	import vtc;
+	sub vcl_backend_fetch {
+		set bereq.http.retries = bereq.retries;
+		if (bereq.retries == 1) {
+			set bereq.backend = s2;
+		}
+	}
+	sub vcl_backend_response {
+		if (bereq.http.retries == "0") {
+			vtc.barrier_sync("${b1_sock}");
+		}
+		set beresp.do_stream = false;
+	}
+	sub vcl_backend_error {
+		return (retry);
+	}
+} -start
+
+client c1 {
+	txreq -req POST -body foo
+	rxresp
+	expect resp.status == 503
+} -run


More information about the varnish-commit mailing list