Send VFP_END with last data for v1f chunked

While working on request body caching improvements, it was noticed that, for chunked encoding, we do not know the request body size before we have attempted to read at least one additional byte because the chunked fetch did not return VFP_END with the last chunk, but rather only as a zero-length additional chunk. This commit changes chunked encoding processing to read the next chunk header before returning the last bytes of the previous chunk. This allows to return VFP_END properly. Test cases are adjusted accordingly. Diff is best viewed with -b option
varnishcache · May 28, 2022 · ec5c816 · ec5c816
1 parent 5e34d67
commit ec5c816
Show file tree

Hide file tree

Showing 8 changed files with 88 additions and 67 deletions.
diff --git a/bin/varnishd/http1/cache_http1_vfp.c b/bin/varnishd/http1/cache_http1_vfp.c
@@ -95,71 +95,86 @@ v1f_read(const struct vfp_ctx *vc, struct http_conn *htc, void *d, ssize_t len)
  * XXX: Reading one byte at a time is pretty pessimal.
  */
 
-static enum vfp_status v_matchproto_(vfp_pull_f)
-v1f_chunked_pull(struct vfp_ctx *vc, struct vfp_entry *vfe, void *ptr,
-    ssize_t *lp)
+static enum vfp_status
+v1f_chunked_hdr(struct vfp_ctx *vc, struct http_conn *htc, ssize_t *szp)
 {
-	struct http_conn *htc;
 	char buf[20];		/* XXX: 20 is arbitrary */
-	char *q;
 	unsigned u;
 	uintmax_t cll;
-	ssize_t cl, l, lr;
+	ssize_t cl, lr;
+	char *q;
 
-	CHECK_OBJ_NOTNULL(vc, VFP_CTX_MAGIC);
-	CHECK_OBJ_NOTNULL(vfe, VFP_ENTRY_MAGIC);
-	CAST_OBJ_NOTNULL(htc, vfe->priv1, HTTP_CONN_MAGIC);
-	AN(ptr);
-	AN(lp);
+	AN(szp);
+	assert(*szp == -1);
 
-	l = *lp;
-	*lp = 0;
-	if (vfe->priv2 == -1) {
-		/* Skip leading whitespace */
-		do {
-			lr = v1f_read(vc, htc, buf, 1);
-			if (lr <= 0)
-				return (VFP_Error(vc, "chunked read err"));
-		} while (vct_islws(buf[0]));
-
-		if (!vct_ishex(buf[0]))
-			 return (VFP_Error(vc, "chunked header non-hex"));
-
-		/* Collect hex digits, skipping leading zeros */
-		for (u = 1; u < sizeof buf; u++) {
-			do {
-				lr = v1f_read(vc, htc, buf + u, 1);
-				if (lr <= 0)
-					return (VFP_Error(vc,
-					    "chunked read err"));
-			} while (u == 1 && buf[0] == '0' && buf[u] == '0');
-			if (!vct_ishex(buf[u]))
-				break;
-		}
+	/* Skip leading whitespace */
+	do {
+		lr = v1f_read(vc, htc, buf, 1);
+		if (lr <= 0)
+			return (VFP_Error(vc, "chunked read err"));
+	} while (vct_islws(buf[0]));
 
-		if (u >= sizeof buf)
-			return (VFP_Error(vc, "chunked header too long"));
+	if (!vct_ishex(buf[0]))
+		return (VFP_Error(vc, "chunked header non-hex"));
 
-		/* Skip trailing white space */
-		while (vct_islws(buf[u]) && buf[u] != '\n') {
+	/* Collect hex digits, skipping leading zeros */
+	for (u = 1; u < sizeof buf; u++) {
+		do {
 			lr = v1f_read(vc, htc, buf + u, 1);
 			if (lr <= 0)
 				return (VFP_Error(vc, "chunked read err"));
-		}
+		} while (u == 1 && buf[0] == '0' && buf[u] == '0');
+		if (!vct_ishex(buf[u]))
+			break;
+	}
 
-		if (buf[u] != '\n')
-			return (VFP_Error(vc, "chunked header no NL"));
+	if (u >= sizeof buf)
+		return (VFP_Error(vc, "chunked header too long"));
 
-		buf[u] = '\0';
+	/* Skip trailing white space */
+	while (vct_islws(buf[u]) && buf[u] != '\n') {
+		lr = v1f_read(vc, htc, buf + u, 1);
+		if (lr <= 0)
+			return (VFP_Error(vc, "chunked read err"));
+	}
 
-		cll = strtoumax(buf, &q, 16);
-		if (q == NULL || *q != '\0')
-			return (VFP_Error(vc, "chunked header number syntax"));
-		cl = (ssize_t)cll;
-		if (cl < 0 || (uintmax_t)cl != cll)
-			return (VFP_Error(vc, "bogusly large chunk size"));
+	if (buf[u] != '\n')
+		return (VFP_Error(vc, "chunked header no NL"));
 
-		vfe->priv2 = cl;
+	buf[u] = '\0';
+
+	cll = strtoumax(buf, &q, 16);
+	if (q == NULL || *q != '\0')
+		return (VFP_Error(vc, "chunked header number syntax"));
+	cl = (ssize_t)cll;
+	if (cl < 0 || (uintmax_t)cl != cll)
+		return (VFP_Error(vc, "bogusly large chunk size"));
+
+	*szp = cl;
+	return (VFP_OK);
+}
+
+static enum vfp_status v_matchproto_(vfp_pull_f)
+v1f_chunked_pull(struct vfp_ctx *vc, struct vfp_entry *vfe, void *ptr,
+    ssize_t *lp)
+{
+	static enum vfp_status vfps;
+	struct http_conn *htc;
+	ssize_t l, lr;
+	char c;
+
+	CHECK_OBJ_NOTNULL(vc, VFP_CTX_MAGIC);
+	CHECK_OBJ_NOTNULL(vfe, VFP_ENTRY_MAGIC);
+	CAST_OBJ_NOTNULL(htc, vfe->priv1, HTTP_CONN_MAGIC);
+	AN(ptr);
+	AN(lp);
+
+	l = *lp;
+	*lp = 0;
+	if (vfe->priv2 == -1) {
+		vfps = v1f_chunked_hdr(vc, htc, &vfe->priv2);
+		if (vfps != VFP_OK)
+			return (vfps);
 	}
 	if (vfe->priv2 > 0) {
 		if (vfe->priv2 < l)
@@ -169,16 +184,22 @@ v1f_chunked_pull(struct vfp_ctx *vc, struct vfp_entry *vfe, void *ptr,
 			return (VFP_Error(vc, "chunked insufficient bytes"));
 		*lp = lr;
 		vfe->priv2 -= lr;
-		if (vfe->priv2 == 0)
-			vfe->priv2 = -1;
-		return (VFP_OK);
+		if (vfe->priv2 != 0)
+			return (VFP_OK);
+
+		vfe->priv2 = -1;
+		vfps = v1f_chunked_hdr(vc, htc, &vfe->priv2);
+		if (vfps != VFP_OK)
+			return (vfps);
+		if (vfe->priv2 != 0)
+			return (VFP_OK);
 	}
 	AZ(vfe->priv2);
-	if (v1f_read(vc, htc, buf, 1) <= 0)
+	if (v1f_read(vc, htc, &c, 1) <= 0)
 		return (VFP_Error(vc, "chunked read err"));
-	if (buf[0] == '\r' && v1f_read(vc, htc, buf, 1) <= 0)
+	if (c == '\r' && v1f_read(vc, htc, &c, 1) <= 0)
 		return (VFP_Error(vc, "chunked read err"));
-	if (buf[0] != '\n')
+	if (c != '\n')
 		return (VFP_Error(vc, "chunked tail no NL"));
 	return (VFP_END);
 }

diff --git a/bin/varnishtest/tests/c00062.vtc b/bin/varnishtest/tests/c00062.vtc
@@ -1,32 +1,23 @@
 varnishtest "Check that aborted backend body aborts client in streaming mode"
-
 barrier b1 cond 2
-barrier b2 cond 2
 
 server s1 {
 	rxreq
 	txresp -nolen -hdr "Transfer-encoding: chunked"
 	chunked {<HTML>}
+	send "00000004\n"
 	barrier b1 sync
-	chunked {<HTML>}
-	barrier b2 sync
 } -start
 
 varnish v1 -cliok "param.set debug +syncvsl" -vcl+backend {
 
 } -start
 
-
 client c1 {
 	txreq
 	rxresphdrs
 	expect resp.status == 200
 	rxchunk
 	barrier b1 sync
-	rxchunk
-	barrier b2 sync
 	expect_close
 } -run
-
-
-
diff --git a/bin/varnishtest/tests/c00067.vtc b/bin/varnishtest/tests/c00067.vtc
@@ -91,6 +91,7 @@ client c1 {
 	chunked {BLAST}
 	delay .2
 	chunkedlen 106
+	chunkedlen 1
 	expect_close
 } -run
 

diff --git a/bin/varnishtest/tests/c00092.vtc b/bin/varnishtest/tests/c00092.vtc
@@ -6,6 +6,7 @@ server s1 -listen "${tmpdir}/s1.sock" {
 	rxreq
 	txresp -nolen -hdr "Transfer-encoding: chunked"
 	chunked {<HTML>}
+	send "00000004\n"
 	barrier b1 sync
 } -start
 

diff --git a/bin/varnishtest/tests/r01184.vtc b/bin/varnishtest/tests/r01184.vtc
@@ -62,6 +62,8 @@ server s1 {
 		sendhex " 10 45 f3 a9 83 b8 18 1c  7b c2 30 55 04 17 13 c4"
 		sendhex " 0f 07 5f 7a 38 f4 8e 50  b3 37 d4 3a 32 4a 34 07"
 		sendhex " FF FF FF FF FF FF FF FF  72 ea 06 5f b3 1c fa dd"
+	send "\r\n"
+	chunkedlen 0
 	expect_close
 } -start
 
@@ -93,6 +95,8 @@ server s1 {
 		sendhex " 10 45 f3 a9 83 b8 18 1c  7b c2 30 55 04 17 13 c4"
 		sendhex " 0f 07 5f 7a 38 f4 8e 50  b3 37 d4 3a 32 4a 34 07"
 		sendhex " FF FF FF FF FF FF FF FF  72 ea 06 5f b3 1c fa dd"
+	send "\r\n"
+	chunkedlen 0
 	expect_close
 } -start
 

diff --git a/bin/varnishtest/tests/r01391.vtc b/bin/varnishtest/tests/r01391.vtc
@@ -7,8 +7,8 @@ server s1 {
 	rxreq
 	txresp -nolen -hdr "Transfer-Encoding: chunked" -hdr "Set-Cookie: foo=bar"
 	chunked "foo"
-	barrier b1 sync
 	chunked "bar"
+	barrier b1 sync
 	delay .1
 	chunkedlen 64
 	delay .1

diff --git a/bin/varnishtest/tests/r01468.vtc b/bin/varnishtest/tests/r01468.vtc
@@ -8,6 +8,7 @@ server s1 {
 	expect req.url == "/1"
 	txresp -nolen -hdr "Transfer-Encoding: chunked"
 	chunked {<HTML>}
+	send "00000004\n"
 	barrier b1 sync
 	close
 
@@ -16,6 +17,7 @@ server s1 {
 	expect req.url == "/2"
 	txresp -nolen -hdr "Transfer-Encoding: chunked"
 	chunked {<HTML>}
+	send "00000004\n"
 	barrier b2 sync
 } -start
 

diff --git a/bin/varnishtest/tests/r03189.vtc b/bin/varnishtest/tests/r03189.vtc
@@ -6,6 +6,7 @@ server s1 {
 	rxreq
 	txresp -nolen -hdr "Transfer-Encoding: chunked"
 	chunkedlen 1
+	chunkedlen 1
 	delay 1
 	non_fatal
 	barrier b sync