fix between bytes timeout vs total timeout

On the client side, we impose a total timeout, yet on the client side we
use between_bytes_timeout and do not care about the total (<- we might
want to reconsider this).

Yet HTC_RxStuff only implemented a total timeout, for which we
effectively used first_byte_timeout + between_bytes_timeout. Yet if
first_byte_timeout was not used up, the effective timeout between bytes
could be substantially longer than between_bytes_timeout (initially
analyzed by @daghf).

We now add a duration argument td to HTC_RxStuff which will be used in
addition to (or instead of) the existing total timeout tn. Either td or
tn must be given.

Testcase originally by @fgsch, slightly modified to avoid an assertion
failure in vtc_server due to the connection being closed by varnish.

Fixes #2395

bin/varnishd/cache/cache_session.c

@@ -247,12 +247,14 @@ HTC_RxPipeline(struct http_conn *htc, void *p)
  * *t1 becomes time of first non-idle rx
  * *t2 becomes time of complete rx
  * ti is when we return IDLE if nothing has arrived
- * tn is when we timeout on non-complete
+ * tn is when we timeout on non-complete (total timeout)
+ * td is max timeout between reads
  */
 
 enum htc_status_e
 HTC_RxStuff(struct http_conn *htc, htc_complete_f *func,
-    vtim_real *t1, vtim_real *t2, vtim_real ti, vtim_real tn, int maxbytes)
+    vtim_real *t1, vtim_real *t2, vtim_real ti, vtim_real tn, vtim_dur td,
+    int maxbytes)
 {
 	vtim_dur tmo;
 	vtim_real now;
@@ -267,7 +269,7 @@ HTC_RxStuff(struct http_conn *htc, htc_complete_f *func,
 	assert(htc->rxbuf_b <= htc->rxbuf_e);
 	assert(htc->rxbuf_e <= htc->ws->r);
 
-	AZ(isnan(tn));
+	AZ(isnan(tn) && isnan(td));
 	if (t1 != NULL)
 		assert(isnan(*t1));
 
@@ -309,9 +311,18 @@ HTC_RxStuff(struct http_conn *htc, htc_complete_f *func,
 		else
 			WRONG("htc_status_e");
 
-		tmo = tn - now;
-		if (!isnan(ti) && ti < tn && hs == HTC_S_EMPTY)
+		if (hs == HTC_S_EMPTY && !isnan(ti) && (isnan(tn) || ti < tn))
 			tmo = ti - now;
+		else if (isnan(tn))
+			tmo = td;
+		else if (isnan(td))
+			tmo = tn - now;
+		else if (td < tn - now)
+			tmo = td;
+		else
+			tmo = tn - now;
+
+		AZ(isnan(tmo));
 		z = maxbytes - (htc->rxbuf_e - htc->rxbuf_b);
 		if (z <= 0) {
 			/* maxbytes reached but not HTC_S_COMPLETE. Return
@@ -328,14 +339,11 @@ HTC_RxStuff(struct http_conn *htc, htc_complete_f *func,
 		} else if (z > 0)
 			htc->rxbuf_e += z;
 		else if (z == -2) {
-			if (hs == HTC_S_EMPTY && ti <= now) {
-				WS_ReleaseP(htc->ws, htc->rxbuf_b);
+			WS_ReleaseP(htc->ws, htc->rxbuf_b);
+			if (hs == HTC_S_EMPTY)
 				return (HTC_S_IDLE);
-			}
-			if (tn <= now) {
-				WS_ReleaseP(htc->ws, htc->rxbuf_b);
+			else
 				return (HTC_S_TIMEOUT);
-			}
 		}
 	}
 }

bin/varnishd/cache/cache_varnishd.h

@@ -342,7 +342,8 @@ const char * HTC_Status(enum htc_status_e);
 void HTC_RxInit(struct http_conn *htc, struct ws *ws);
 void HTC_RxPipeline(struct http_conn *htc, void *);
 enum htc_status_e HTC_RxStuff(struct http_conn *, htc_complete_f *,
-    vtim_real *t1, vtim_real *t2, vtim_real ti, vtim_real tn, int maxbytes);
+    vtim_real *t1, vtim_real *t2, vtim_real ti, vtim_real tn, vtim_dur td,
+    int maxbytes);
 
 #define SESS_ATTR(UP, low, typ, len)					\
 	int SES_Set_##low(const struct sess *sp, const typ *src);	\

bin/varnishd/http1/cache_http1_fetch.c

@@ -178,7 +178,7 @@ V1F_FetchRespHdr(struct busyobj *bo)
 
 	t = VTIM_real() + htc->first_byte_timeout;
 	hs = HTC_RxStuff(htc, HTTP1_Complete, NULL, NULL,
-	    t, t + htc->between_bytes_timeout, cache_param->http_resp_size);
+	    t, NAN, htc->between_bytes_timeout, cache_param->http_resp_size);
 	if (hs != HTC_S_COMPLETE) {
 		bo->acct.beresp_hdrbytes +=
 		    htc->rxbuf_e - htc->rxbuf_b;

bin/varnishd/http1/cache_http1_fsm.c

@@ -332,6 +332,7 @@ HTTP1_Session(struct worker *wrk, struct req *req)
 			    &req->t_first, &req->t_req,
 			    sp->t_idle + cache_param->timeout_linger,
 			    sp->t_idle + cache_param->timeout_idle,
+			    NAN,
 			    cache_param->http_req_size);
 			AZ(req->htc->ws->r);
 			if (hs < HTC_S_EMPTY) {

bin/varnishd/http2/cache_http2_proto.c

@@ -1080,7 +1080,7 @@ h2_rxframe(struct worker *wrk, struct h2_sess *h2)
 	h2->sess->t_idle = VTIM_real();
 	hs = HTC_RxStuff(h2->htc, h2_frame_complete,
 	    NULL, NULL, NAN,
-	    h2->sess->t_idle + cache_param->timeout_idle,
+	    h2->sess->t_idle + cache_param->timeout_idle, NAN,
 	    h2->local_settings.max_frame_size + 9);
 	switch (hs) {
 	case HTC_S_COMPLETE:

bin/varnishd/http2/cache_http2_session.c

@@ -291,7 +291,7 @@ h2_ou_session(struct worker *wrk, struct h2_sess *h2,
 
 	/* Wait for PRISM response */
 	hs = HTC_RxStuff(h2->htc, H2_prism_complete,
-	    NULL, NULL, NAN, h2->sess->t_idle + cache_param->timeout_idle,
+	    NULL, NULL, NAN, h2->sess->t_idle + cache_param->timeout_idle, NAN,
 	    sizeof H2_prism);
 	if (hs != HTC_S_COMPLETE) {
 		VSLb(h2->vsl, SLT_Debug, "H2: No/Bad OU PRISM (hs=%d)", hs);

bin/varnishd/proxy/cache_proxy_proto.c

@@ -554,7 +554,7 @@ vpx_new_session(struct worker *wrk, void *arg)
 
 	HTC_RxInit(req->htc, req->ws);
 	hs = HTC_RxStuff(req->htc, vpx_complete,
-	    NULL, NULL, NAN, sp->t_idle + cache_param->timeout_idle,
+	    NULL, NULL, NAN, sp->t_idle + cache_param->timeout_idle, NAN,
 	    1024);			// XXX ?
 	if (hs != HTC_S_COMPLETE) {
 		Req_Release(req);

bin/varnishtest/tests/r02395.vtc

+varnishtest "Test between_bytes_timeout works fetching headers"
+
+server s1 {
+        rxreq
+        send "HTTP/1.0 "
+        delay 2
+} -start
+
+varnish v1 -vcl+backend {
+        sub vcl_recv { return (pass); }
+        sub vcl_backend_fetch { set bereq.between_bytes_timeout = 1s; }
+} -start
+
+varnish v1 -cliok "param.set debug +syncvsl"
+
+client c1 {
+        txreq
+        rxresp
+        expect resp.status == 503
+} -run
+
+server s1 -wait