[master] f88b47951 Fail fetch retries when uncached request body has been released

[Dridi Boukelmoune]

commit f88b47951415fc212c8b4bb929b5a0db2c0835c0
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.

diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h
index 836be9c4e..a8f021645 100644
--- a/bin/varnishd/cache/cache.h
+++ b/bin/varnishd/cache/cache.h
@@ -389,6 +389,7 @@ struct busyobj {
 	 * is recycled.
 	 */
 	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 808655f0b..7a2e57b37 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/bin/varnishd/cache/cache_fetch.c
@@ -238,6 +238,7 @@ vbf_stp_mkbereq(struct worker *wrk, struct busyobj *bo)
 	bo->ws_bo = WS_Snapshot(bo->ws);
 	HTTP_Clone(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);
@@ -264,6 +265,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