[master] ace8e8fe1 Don't transition to CLOS_REM state until we've seen END_STREAM

[Dag Haavi Finstad]

commit ace8e8fe1a3601f0a6b760a2b50cd3cbb4fc75fe
Author: Dag Haavi Finstad <daghf at varnish-software.com>
Date:   Fri Jun 22 11:20:12 2018 +0200

    Don't transition to CLOS_REM state until we've seen END_STREAM
    
    Previously we've been incorrectly transtitioning to CLOS_REM on
    END_HEADERS, which prevents us from seeing if a client incorrectly
    transmitted a DATA frame on a closed stream.
    
    This slightly complicates things in that we can now be in state OPEN
    with an inactive hpack decoding state, and we need to make sure on
    cleanup if that has already been finalized.
    
    This would be simpler if the h/2 spec had split the OPEN state in two
    parts, with an extra state transition on END_HEADERS.
    
    Again big thanks to @xcir for his help in diagnosing this.
    
    Fixes: #2623

diff --git a/bin/varnishd/http2/cache_http2_proto.c b/bin/varnishd/http2/cache_http2_proto.c
index 7001b60fc..2a0d73b81 100644
--- a/bin/varnishd/http2/cache_http2_proto.c
+++ b/bin/varnishd/http2/cache_http2_proto.c
@@ -225,7 +225,7 @@ h2_kill_req(struct worker *wrk, const struct h2_sess *h2,
 			AZ(pthread_cond_signal(r2->cond));
 		r2 = NULL;
 	} else {
-		if (r2->state == H2_S_OPEN) {
+		if (r2->state == H2_S_OPEN && r2->decode != NULL) {
 			(void)h2h_decode_fini(h2, r2->decode);
 			FREE_OBJ(r2->decode);
 		}
@@ -559,7 +559,8 @@ h2_end_headers(struct worker *wrk, struct h2_sess *h2,
 	assert(r2->state == H2_S_OPEN);
 	h2e = h2h_decode_fini(h2, r2->decode);
 	FREE_OBJ(r2->decode);
-	r2->state = H2_S_CLOS_REM;
+	if (h2->rxf_flags & H2FF_HEADERS_END_STREAM)
+		r2->state = H2_S_CLOS_REM;
 	h2->new_req = NULL;
 	if (h2e != NULL) {
 		Lck_Lock(&h2->sess->mtx);
@@ -693,7 +694,7 @@ h2_rx_continuation(struct worker *wrk, struct h2_sess *h2, struct h2_req *r2)
 	h2_error h2e;
 
 	ASSERT_RXTHR(h2);
-	if (r2 == NULL || r2->state != H2_S_OPEN)
+	if (r2 == NULL || r2->state != H2_S_OPEN || r2->req != h2->new_req)
 		return (H2CE_PROTOCOL_ERROR);	// XXX spec ?
 	req = r2->req;
 	h2e = h2h_decode_bytes(h2, r2->decode, h2->rxf_data, h2->rxf_len);
@@ -726,6 +727,10 @@ h2_rx_data(struct worker *wrk, struct h2_sess *h2, struct h2_req *r2)
 	ASSERT_RXTHR(h2);
 	if (r2 == NULL || !r2->scheduled)
 		return (0);
+	if (r2->state >= H2_S_CLOS_REM) {
+		r2->error = H2SE_STREAM_CLOSED;
+		return (H2SE_STREAM_CLOSED); // rfc7540,l,1766,1769
+	}
 	Lck_Lock(&h2->sess->mtx);
 	AZ(h2->mailcall);
 	h2->mailcall = r2;
@@ -780,6 +785,7 @@ h2_vfp_body(struct vfp_ctx *vc, struct vfp_entry *vfe, void *ptr, ssize_t *lp)
 	*lp = 0;
 
 	Lck_Lock(&h2->sess->mtx);
+	assert (r2->state == H2_S_OPEN);
 	r2->cond = &vc->wrk->cond;
 	while (h2->mailcall != r2 && h2->error == 0 && r2->error == 0)
 		AZ(Lck_CondWait(r2->cond, &h2->sess->mtx, 0));
@@ -803,8 +809,10 @@ h2_vfp_body(struct vfp_ctx *vc, struct vfp_entry *vfe, void *ptr, ssize_t *lp)
 			return (VFP_OK);
 		}
 		if (h2->rxf_len == 0) {
-			if (h2->rxf_flags & H2FF_DATA_END_STREAM)
+			if (h2->rxf_flags & H2FF_DATA_END_STREAM) {
 				retval = VFP_END;
+				r2->state = H2_S_CLOS_REM;
+			}
 		}
 		h2->mailcall = NULL;
 		AZ(pthread_cond_signal(h2->cond));
diff --git a/bin/varnishtest/tests/t02014.vtc b/bin/varnishtest/tests/t02014.vtc
index bc49f4e84..17489d362 100644
--- a/bin/varnishtest/tests/t02014.vtc
+++ b/bin/varnishtest/tests/t02014.vtc
@@ -1,8 +1,8 @@
 varnishtest "Exercise h/2 sender flow control code"
 
-barrier b1 sock 3
+barrier b1 sock 3 -cyclic
 
-server s1 {
+server s1 -repeat 2 {
 	rxreq
 	txresp -bodylen 66300
 } -start
@@ -45,3 +45,59 @@ client c1 {
 
 	stream 0 -wait
 } -run
+
+client c1 {
+	stream 0 {
+		barrier b1 sync
+	} -start
+
+	stream 1 {
+		txreq
+		txdata -data "fail"
+		rxrst
+		expect rst.err == STREAM_CLOSED
+		barrier b1 sync
+	} -run
+
+	stream 0 -wait
+} -run
+
+client c1 {
+	stream 0 {
+		barrier b1 sync
+		barrier b1 sync
+		delay .5
+		txwinup -size 256
+		delay .5
+		txwinup -size 256
+		delay .5
+		txwinup -size 256
+
+	} -start
+
+	stream 1 {
+		txreq -req "POST" -nostrend
+		txdata -data "ok"
+		rxwinup
+		txdata -data "fail"
+		rxrst
+		expect rst.err == STREAM_CLOSED
+		barrier b1 sync
+	} -run
+
+	stream 3 {
+		txreq
+		barrier b1 sync
+		delay .5
+		txwinup -size 256
+		delay .5
+		txwinup -size 256
+		delay .5
+		txwinup -size 256
+		rxresp
+		expect resp.status == 200
+		expect resp.bodylen == 66300
+	} -run
+
+	stream 0 -wait
+} -run