give PRIV_TOP a head separate from (req)PRIV_TASK in struct reqtop

This fixes #3003 properly restore tests/r02219.vtc to the same headroom as before we need additional workspace for the priv_top which now always gets initialized (32 bytes on my machine)
varnishcache · Nov 13, 2019 · 542bf9b · 542bf9b
1 parent fc04ea2
commit 542bf9b
Show file tree

Hide file tree

Showing 9 changed files with 31 additions and 15 deletions.
diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h
@@ -446,6 +446,7 @@ struct reqtop {
 #define REQTOP_MAGIC		0x57fbda52
 	struct req		*topreq;
 	struct vcl		*vcl0;
+	struct vrt_privs	privs[1];
 };
 
 struct req {

diff --git a/bin/varnishd/cache/cache_panic.c b/bin/varnishd/cache/cache_panic.c
@@ -493,6 +493,7 @@ pan_top(struct vsb *vsb, const struct reqtop *top)
 		return;
 	VSB_indent(vsb, 2);
 	pan_req(vsb, top->topreq);
+	pan_privs(vsb, top->privs);
 	VCL_Panic(vsb, "vcl0", top->vcl0);
 	VSB_indent(vsb, -2);
 	VSB_cat(vsb, "},\n");

diff --git a/bin/varnishd/cache/cache_req.c b/bin/varnishd/cache/cache_req.c
@@ -182,13 +182,19 @@ Req_Release(struct req *req)
  * TODO:
  * - check for code duplication with cnt_recv_prep
  * - re-check if complete
+ * - XXX should PRIV_TOP use vcl0?
+ * - XXX PRIV_TOP does not get rolled back, should it for !IS_TOPREQ ?
  */
 
 void
 Req_Rollback(struct req *req)
 {
+	if (IS_TOPREQ(req))
+		VCL_TaskLeave(req->vcl, req->top->privs);
 	VCL_TaskLeave(req->vcl, req->privs);
 	VCL_TaskEnter(req->vcl, req->privs);
+	if (IS_TOPREQ(req))
+		VCL_TaskEnter(req->vcl, req->top->privs);
 	HTTP_Clone(req->http, req->http0);
 	if (WS_Overflowed(req->ws))
 		req->wrk->stats->ws_client_overflow++;

diff --git a/bin/varnishd/cache/cache_req_fsm.c b/bin/varnishd/cache/cache_req_fsm.c
@@ -1104,8 +1104,11 @@ CNT_Request(struct req *req)
 	}
 	wrk->vsl = NULL;
 	if (nxt == REQ_FSM_DONE) {
-		if (IS_TOPREQ(req) && req->top->vcl0 != NULL)
-			VCL_Rel(&req->top->vcl0);
+		if (IS_TOPREQ(req)) {
+			VCL_TaskLeave(req->vcl, req->top->privs);
+			if (req->top->vcl0 != NULL)
+				VCL_Rel(&req->top->vcl0);
+		}
 		VCL_TaskLeave(req->vcl, req->privs);
 		AN(req->vsl->wid);
 		VRB_Free(req);

diff --git a/bin/varnishd/cache/cache_vpi.c b/bin/varnishd/cache/cache_vpi.c
@@ -94,6 +94,11 @@ VPI_vcl_select(VRT_CTX, VCL_VCL vcl)
 	if (IS_TOPREQ(req) && req->top->vcl0 != NULL)
 		return;		// Illegal, req-FSM will fail this later.
 
+	/* XXX VCL_Task* are somewhat duplicated to those in Req_Rollback called
+	 * from FSM for VCL_RET_VCL. Keeping them here to ensure there are no
+	 * tasks during calls to VCL_Rel / vcl_get
+	 */
+	VCL_TaskLeave(req->vcl, req->top->privs);
 	VCL_TaskLeave(req->vcl, req->privs);
 	if (IS_TOPREQ(req)) {
 		AN(req->top);
@@ -107,4 +112,5 @@ VPI_vcl_select(VRT_CTX, VCL_VCL vcl)
 	VSLb(ctx->req->vsl, SLT_VCL_use, "%s via %s",
 	    req->vcl->loaded_name, vcl->loaded_name);
 	VCL_TaskEnter(req->vcl, req->privs);
+	VCL_TaskEnter(req->vcl, req->top->privs);
 }
diff --git a/bin/varnishd/cache/cache_vrt_priv.c b/bin/varnishd/cache/cache_vrt_priv.c
@@ -170,16 +170,12 @@ VRT_priv_top(VRT_CTX, const void *vmod_id)
 		WRONG("PRIV_TOP is only accessible in client VCL context");
 		NEEDLESS(return (NULL));
 	}
-	CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC);
 	req = ctx->req;
-	if (req->top != NULL) {
-		CHECK_OBJ_NOTNULL(req->top, REQTOP_MAGIC);
-		CHECK_OBJ_NOTNULL(req->top->topreq, REQ_MAGIC);
-		req = req->top->topreq;
-	}
+	CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
+	CHECK_OBJ_NOTNULL(req->top, REQTOP_MAGIC);
 	return (vrt_priv_dynamic(
 	    req->ws,
-	    req->privs,
+	    req->top->privs,
 	    (uintptr_t)vmod_id
 	));
 }

diff --git a/bin/varnishd/http1/cache_http1_fsm.c b/bin/varnishd/http1/cache_http1_fsm.c
@@ -410,8 +410,10 @@ HTTP1_Session(struct worker *wrk, struct req *req)
 			req->task.priv = req;
 			wrk->stats->client_req++;
 			CNT_Embark(wrk, req);
-			if (req->req_step == R_STP_TRANSPORT)
+			if (req->req_step == R_STP_TRANSPORT) {
 				VCL_TaskEnter(req->vcl, req->privs);
+				VCL_TaskEnter(req->vcl, req->top->privs);
+			}
 			if (CNT_Request(req) == REQ_FSM_DISEMBARK)
 				return;
 			AZ(req->top->vcl0);

diff --git a/bin/varnishd/http2/cache_http2_proto.c b/bin/varnishd/http2/cache_http2_proto.c
@@ -523,8 +523,10 @@ h2_do_req(struct worker *wrk, void *priv)
 	CAST_OBJ_NOTNULL(r2, req->transport_priv, H2_REQ_MAGIC);
 	THR_SetRequest(req);
 	CNT_Embark(wrk, req);
-	if (req->req_step == R_STP_TRANSPORT)
+	if (req->req_step == R_STP_TRANSPORT) {
 		VCL_TaskEnter(req->vcl, req->privs);
+		VCL_TaskEnter(req->vcl, req->top->privs);
+	}
 
 	wrk->stats->client_req++;
 	if (CNT_Request(req) != REQ_FSM_DISEMBARK) {

diff --git a/bin/varnishtest/tests/r02219.vtc b/bin/varnishtest/tests/r02219.vtc
@@ -19,7 +19,7 @@ varnish v1 -arg "-p workspace_client=9k" -proto PROXY -vcl+backend {
 } -start
 
 client c1 {
-	send "PROXY TCP4 127.0.0.1 127.0.0.1 1111 2222\r\nGET /AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA HTTP/1.1\r\n\r\n"
+	send "PROXY TCP4 127.0.0.1 127.0.0.1 1111 2222\r\nGET /AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA HTTP/1.1\r\n\r\n"
 	rxresp
 } -run
 
@@ -58,14 +58,13 @@ client c2 {
 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42
 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42
 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42
-42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42
-42 42 42 42 42 42 42 42 42 42 42 42 42 42
+42 42 42 42 42 42 42
 20 48 54 54 50 2f 31 2e 31 0d 0a 0d 0a
 }
 	rxresp
 } -run
 
 client c3 {
-	send "PROXY TCP4 127.0.0.1 127.0.0.1 1111 2222\r\nGET /CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC HTTP/1.1\r\n\r\n"
+	send "PROXY TCP4 127.0.0.1 127.0.0.1 1111 2222\r\nGET /CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC HTTP/1.1\r\n\r\n"
 	rxresp
 } -run