h2: Don't panic if we reembark with a request body

This moves the body status decision to h2_end_headers, to make sure that
it is only done once per request.

We also move the Cookie header concatenation to h2_end_headers, to avoid
stepping on any changes that may have taken place in VCL.

Fixes: #2305

bin/varnishd/http2/cache_http2_proto.c

@@ -480,26 +480,11 @@ h2_do_req(struct worker *wrk, void *priv)
 {
 	struct req *req;
 	struct h2_req *r2;
-	const char *b;
 
 	CAST_OBJ_NOTNULL(req, priv, REQ_MAGIC);
 	CAST_OBJ_NOTNULL(r2, req->transport_priv, H2_REQ_MAGIC);
 	THR_SetRequest(req);
 
-	// XXX: Smarter to do this already at HPACK time into tail end of
-	// XXX: WS, then copy back once all headers received.
-	// XXX: Have I mentioned H/2 Is hodge-podge ?
-	http_CollectHdrSep(req->http, H_Cookie, "; ");	// rfc7540,l,3114,3120
-
-	if (req->req_body_status == REQ_BODY_INIT) {
-		if (!http_GetHdr(req->http, H_Content_Length, &b))
-			req->req_body_status = REQ_BODY_WITHOUT_LEN;
-		else
-			req->req_body_status = REQ_BODY_WITH_LEN;
-	} else {
-		assert (req->req_body_status == REQ_BODY_NONE);
-	}
-
 	req->http->conds = 1;
 	if (CNT_Request(wrk, req) != REQ_FSM_DISEMBARK) {
 		AZ(req->ws->r);
@@ -516,6 +501,7 @@ h2_end_headers(struct worker *wrk, struct h2_sess *h2,
     struct req *req, struct h2_req *r2)
 {
 	h2_error h2e;
+	const char *b;
 
 	ASSERT_RXTHR(h2);
 	assert(r2->state == H2_S_OPEN);
@@ -533,6 +519,20 @@ h2_end_headers(struct worker *wrk, struct h2_sess *h2,
 	}
 	VSLb_ts_req(req, "Req", req->t_req);
 
+	// XXX: Smarter to do this already at HPACK time into tail end of
+	// XXX: WS, then copy back once all headers received.
+	// XXX: Have I mentioned H/2 Is hodge-podge ?
+	http_CollectHdrSep(req->http, H_Cookie, "; ");	// rfc7540,l,3114,3120
+
+	if (req->req_body_status == REQ_BODY_INIT) {
+		if (!http_GetHdr(req->http, H_Content_Length, &b))
+			req->req_body_status = REQ_BODY_WITHOUT_LEN;
+		else
+			req->req_body_status = REQ_BODY_WITH_LEN;
+	} else {
+		assert (req->req_body_status == REQ_BODY_NONE);
+	}
+
 	req->req_step = R_STP_TRANSPORT;
 	req->task.func = h2_do_req;
 	req->task.priv = req;

bin/varnishtest/tests/r02305.vtc

+varnishtest "#2305: h/2 reembark with a request body"
+
+barrier b1 cond 2
+barrier b2 sock 2
+
+server s1 {
+	rxreq
+	expect req.url == "/"
+	barrier b1 sync
+	delay 2
+	txresp
+} -start
+
+varnish v1 -cliok "param.set feature +http2"
+varnish v1 -cliok "param.set debug +syncvsl"
+varnish v1 -cliok "param.set debug +waitinglist"
+
+varnish v1 -vcl+backend {
+	import vtc;
+	sub vcl_recv {
+		return (hash);
+	}
+	sub vcl_deliver {
+		if (req.http.sync) {
+			vtc.barrier_sync("${b2_sock}");
+		}
+	}
+} -start
+
+client c1 {
+	stream 1 {
+		txreq
+		rxresp
+		expect resp.status == 200
+	} -start
+	stream 3 {
+		barrier b1 sync
+		txreq -req POST -hdr sync 1 -body "foo"
+		rxwinup
+		# barrier b2 is here to make HEADERS vs WINDOW_UPDATE
+		# less racy
+		barrier b2 sync
+		rxresp
+		expect resp.status == 200
+	} -run
+} -run
+
+varnish v1 -vsl_catchup
+varnish v1 -expect MEMPOOL.req0.live == 0
+varnish v1 -expect MEMPOOL.sess0.live == 0
+varnish v1 -expect MEMPOOL.req1.live == 0
+varnish v1 -expect MEMPOOL.sess1.live == 0