Fix VRT_priv_top() to use the topreq's workspace and change v43.vtc to

test the issue This bug triggered when a PRIV_TOP was requested only from esi_level > 0 and used by a "sibling" ESI request, that is, another request on the same ESI level or its descendants. The bug was introduced by someone(tm) in 542bf9b: I removed req = req->top->topreq but continued to use the workspace from req->ws. Fixes #3496
varnishcache · Jan 13, 2021 · 3228cb7 · 3228cb7
1 parent 1952d3c
commit 3228cb7
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 6 deletions.
diff --git a/bin/varnishd/cache/cache_vrt_priv.c b/bin/varnishd/cache/cache_vrt_priv.c
@@ -218,6 +218,8 @@ VRT_priv_top(VRT_CTX, const void *vmod_id)
 	CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
 	top = req->top;
 	CHECK_OBJ_NOTNULL(top, REQTOP_MAGIC);
+	req = top->topreq;
+	CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
 
 	Lck_Lock(&sp->mtx);
 	priv = vrt_priv_dynamic(req->ws, top->privs, (uintptr_t)vmod_id);

diff --git a/bin/varnishtest/tests/v00043.vtc b/bin/varnishtest/tests/v00043.vtc
@@ -7,15 +7,18 @@ server s1 {
 	expect req.url == "/a"
 	expect req.http.x0 == "/a0"
 	expect req.http.x1 == "/a0"
+	expect req.http.o1 == <undef>
 	txresp -body {
 		<html>
-		<esi:include src="/foo"/>
+		<esi:include src="/foo1"/>
+		<esi:include src="/foo2"/>
 	}
 
 	rxreq
-	expect req.url == "/foo"
+	expect req.url == "/foo1"
 	expect req.http.x0 == "/a0"
 	expect req.http.x1 == "/a0"
+	expect req.http.o1 == "/foo11"
 	txresp -body {
 		<html>
 		<esi:include src="/bar"/>
@@ -25,12 +28,24 @@ server s1 {
 	expect req.url == "/bar"
 	expect req.http.x0 == "/a0"
 	expect req.http.x1 == "/a0"
+	expect req.http.o1 == "/foo11"
 	txresp
 
+	rxreq
+	expect req.url == "/foo2"
+	expect req.http.x0 == "/a0"
+	expect req.http.x1 == "/a0"
+	expect req.http.o1 == "/foo11"
+	txresp -body {
+		<html>
+		<esi:include src="/bar"/>
+	}
+
 	rxreq
 	expect req.url == "/b"
 	expect req.http.x0 == "/b0"
 	expect req.http.x1 == "/b0"
+	expect req.http.o1 == <undef>
 
 	txresp
 } -start
@@ -40,15 +55,23 @@ varnish v1 -cliok "param.set debug +syncvsl" -vcl+backend {
 
 	sub vcl_init {
 		new o = debug.obj();
+		new o2 = debug.obj();
 	}
 
 	sub vcl_recv {
 		set req.http.x0 = debug.test_priv_top(req.url + req.esi_level);
-		o.test_priv_top(req.url + req.esi_level);
+		if (req.url == "/foo1") {
+			o.test_priv_top(req.url + req.esi_level);
+		} else {
+			o2.test_priv_top(req.url + req.esi_level);
+		}
 	}
 
 	sub vcl_miss {
 		set req.http.x1 = debug.test_priv_top("");
+		if (req.esi_level > 0) {
+			set req.http.o1 = o.test_priv_top("");
+		}
 	}
 
 	sub vcl_backend_response {
@@ -58,7 +81,6 @@ varnish v1 -cliok "param.set debug +syncvsl" -vcl+backend {
 
 	sub vcl_deliver {
 		set resp.http.x1 = debug.test_priv_top("");
-		set resp.http.o1 = o.test_priv_top("");
 	}
 } -start
 
@@ -67,12 +89,10 @@ client c1 {
 	txreq -url /a
 	rxresp
 	expect resp.http.x1 == "/a0"
-	expect resp.http.o1 == "/a0"
 
 	txreq -url /b
 	rxresp
 	expect resp.http.x1 == "/b0"
-	expect resp.http.o1 == "/b0"
 } -run
 
 varnish v1 -expect client_req == 2