Allow PRIORITY frames on closed streams

Currently Varnish doesn't allow PRIORITY frames to be received on closed
streams: it treats it as a protocol violation and replies with a GOAWAY.

This is not spec compliant, rfc7540 states:

The PRIORITY frame can be sent for a stream in the "idle" or "closed"
state.
rfc7540,l,1947,1948

The PRIORITY frame can be sent on a stream in any state
rfc7540,l,1938,1938

https://tools.ietf.org/html/rfc7540#section-6.3

This behaviour can be triggered by real-world browsers: Chrome 69 has been
observed sending PRIORITY frames which are received by Varnish after a stream
has been closed (and cleaned by h2_sweep). When that happens the connection is
closed and Chrome aborts the loading of all the resources which started to
load on that connection.

This commit solves the issue by avoiding all the stream creation code and its
checks to be performed when a PRIORITY frame is received.
This moves all the stream creation logic inside h2_rx_headers, the only other
frame which is allowed on idle streams.

This also fixes the concurrent streams counter and highest_stream: they should
be updated only when a stream enters the "open" state (or "reserved" if
Varnish used served push) but currently a PRIORITY frame on an idle stream
affects them.

https://tools.ietf.org/html/rfc7540#section-5.1.1
rfc7540,l,1153,1156
rfc7540,l,1193,1198
rfc7540,l,1530,1533

Fixes: #2775

bin/varnishd/http2/cache_http2_proto.c

@@ -382,7 +382,7 @@ h2_rx_priority(struct worker *wrk, struct h2_sess *h2, struct h2_req *r2)
 
 	(void)wrk;
 	ASSERT_RXTHR(h2);
-	xxxassert(r2->stream & 1);
+	(void)r2;
 	return (0);
 }
 
@@ -616,7 +616,21 @@ h2_rx_headers(struct worker *wrk, struct h2_sess *h2, struct h2_req *r2)
 	size_t l;
 
 	ASSERT_RXTHR(h2);
+
+	if (r2 == NULL) {
+		if (h2->rxf_stream <= h2->highest_stream)
+			return (H2CE_PROTOCOL_ERROR);	// rfc7540,l,1153,1158
+		if (h2->refcnt >= h2->local_settings.max_concurrent_streams) {
+			VSLb(h2->vsl, SLT_Debug,
+			     "H2: stream %u: Hit maximum number of "
+			     "concurrent streams", h2->rxf_stream);
+			return (H2SE_REFUSED_STREAM);	// rfc7540,l,1200,1205
+		}
+		h2->highest_stream = h2->rxf_stream;
+		r2 = h2_new_req(wrk, h2, h2->rxf_stream, NULL);
+	}
 	AN(r2);
+
 	if (r2->state != H2_S_IDLE)
 		return (H2CE_PROTOCOL_ERROR);	// XXX spec ?
 	r2->state = H2_S_OPEN;
@@ -927,23 +941,6 @@ h2_procframe(struct worker *wrk, struct h2_sess *h2, h2_frame h2f)
 	    !(r2 && h2->new_req == r2->req && h2f == H2_F_CONTINUATION))
 		return (H2CE_PROTOCOL_ERROR);	// rfc7540,l,1859,1863
 
-	if (r2 == NULL && h2f->act_sidle == 0) {
-		if (h2->rxf_stream <= h2->highest_stream)
-			return (H2CE_PROTOCOL_ERROR);	// rfc7540,l,1153,1158
-		if (h2->refcnt >= h2->local_settings.max_concurrent_streams) {
-			VSLb(h2->vsl, SLT_Debug,
-			     "H2: stream %u: Hit maximum number of "
-			     "concurrent streams", h2->rxf_stream);
-			// rfc7540,l,1200,1205
-			h2_tx_rst(wrk, h2, h2->req0, h2->rxf_stream,
-				H2SE_REFUSED_STREAM);
-			return (0);
-		}
-		h2->highest_stream = h2->rxf_stream;
-		r2 = h2_new_req(wrk, h2, h2->rxf_stream, NULL);
-		AN(r2);
-	}
-
 	h2e = h2f->rxfunc(wrk, h2, r2);
 	if (h2e == 0)
 		return (0);
@@ -993,7 +990,6 @@ static int
 h2_sweep(struct worker *wrk, struct h2_sess *h2)
 {
 	int tmo = 0;
-	int nprio = 0;
 	struct h2_req *r2, *r22;
 
 	ASSERT_RXTHR(h2);
@@ -1025,20 +1021,18 @@ h2_sweep(struct worker *wrk, struct h2_sess *h2)
 			}
 			break;
 		case H2_S_IDLE:
-			/* This stream was created from receiving a
-			 * PRIORITY frame, and should not be counted
-			 * as an active stream keeping the connection
-			 * open. */
-			AZ(r2->scheduled);
-			nprio++;
-			break;
+			/* Current code make this unreachable: h2_new_req is
+			 * only called inside h2_rx_headers, which immediately
+			 * sets the new stream state to H2_S_OPEN */
+			/* FALLTHROUGH */
 		default:
+			WRONG("Wrong h2 stream state");
 			break;
 		}
 	}
 	if (tmo)
 		return (0);
-	return ((h2->refcnt - nprio) > 1);
+	return (h2->refcnt > 1);
 }
 
 

bin/varnishtest/tests/r02775.vtc

+varnishtest "Regression test for #2775: allow PRIORITY on closed stream"
+
+server s1 {
+	rxreq
+	txresp
+} -start
+
+varnish v1 -vcl+backend {} -start
+varnish v1 -cliok "param.set feature +http2"
+varnish v1 -cliok "param.set debug +syncvsl"
+
+client c1 {
+	stream 1 {
+		txreq
+		rxresp
+
+		txprio
+	} -run
+	stream 3 {
+		txreq
+		rxresp
+	} -run
+} -run