[6.0] 7c7a90fdf Allow PRIORITY frames on closed streams

[Dridi Boukelmoune]

commit 7c7a90fdf86d0029ea25090513afacfb348bfab1
Author: Carlo Cannas <carlo94 at gmail.com>
Date:   Sun Sep 23 03:34:02 2018 +0200

    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

diff --git a/bin/varnishd/http2/cache_http2_proto.c b/bin/varnishd/http2/cache_http2_proto.c
index b106f9e36..250af1806 100644
--- a/bin/varnishd/http2/cache_http2_proto.c
+++ b/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);
 }
 
 
diff --git a/bin/varnishtest/tests/r02775.vtc b/bin/varnishtest/tests/r02775.vtc
new file mode 100644
index 000000000..de66c10d1
--- /dev/null
+++ b/bin/varnishtest/tests/r02775.vtc
@@ -0,0 +1,23 @@
+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