[6.1] 7a56c831e Allow PRIORITY frames on closed streams
hermunn
hermunn at varnish-software.com
Wed Oct 24 09:29:20 UTC 2018
commit 7a56c831e21fb0d22d4bea4af1ec0c24503005ea
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
More information about the varnish-commit
mailing list