[6.0] 103ea7c57 finalize task privs when rolling back workspaces

[Dridi Boukelmoune]

commit 103ea7c573d153966db68763ce0b40d2e2c01554
Author: Nils Goroll <nils.goroll at uplex.de>
Date:   Sun Jun 10 10:07:29 2018 +0200

    finalize task privs when rolling back workspaces
    
    ... and introduce request functions for this purpose (for busy
    objects, there is only one use case yet, so we don't).
    
    Before we reset the workspace, we must ensure that there are no active
    references to objects on it. As PRIV_TASK and PRIV_TOP have the same
    lifetime as the respective workspace, they need to be destroyed. vmods
    must not use workspaces for storing information referenced via any of
    the other PRIVs unless the rollback case is considered.
    
    Note that while this bug was exposed by
    beeaa19cced3fe1ab79381b2b1b7b0b5594cbb18, it existed all along for any
    vmod priv state stored on the workspace, so if a vmod happened to
    access a TASK_PRIV stored on the workspace, it would likely have
    triggered a magic check assertion as well.
    
    I got plans for making std.rollback() more useful. While this change
    is required to do so, it only partly covers the planned changes.
    
    Fixes #2706

diff --git a/bin/varnishd/cache/cache_req.c b/bin/varnishd/cache/cache_req.c
index a6b9ee0b1..d741441ca 100644
--- a/bin/varnishd/cache/cache_req.c
+++ b/bin/varnishd/cache/cache_req.c
@@ -181,6 +181,28 @@ Req_Release(struct req *req)
 	MPL_Free(pp->mpl_req, req);
 }
 
+static void
+req_finalize(struct req *req)
+{
+	VRTPRIV_dynamic_kill(req->privs, (uintptr_t)req);
+	VRTPRIV_dynamic_kill(req->privs, (uintptr_t)&req->top);
+	assert(VTAILQ_EMPTY(&req->privs->privs));
+}
+
+/*----------------------------------------------------------------------
+ * TODO:
+ * - check for code duplication with cnt_recv_prep
+ * - re-check if complete
+ */
+
+void
+Req_Rollback(struct req *req)
+{
+	req_finalize(req);
+	HTTP_Copy(req->http, req->http0);
+	WS_Reset(req->ws, req->ws_req);
+}
+
 /*----------------------------------------------------------------------
  * TODO: remove code duplication with cnt_recv_prep
  */
@@ -207,9 +229,7 @@ Req_Cleanup(struct sess *sp, struct worker *wrk, struct req *req)
 		req->vcl = NULL;
 	}
 
-	VRTPRIV_dynamic_kill(req->privs, (uintptr_t)req);
-	VRTPRIV_dynamic_kill(req->privs, (uintptr_t)&req->top);
-	assert(VTAILQ_EMPTY(&req->privs->privs));
+	req_finalize(req);
 
 	/* Charge and log byte counters */
 	if (req->vsl->wid) {
diff --git a/bin/varnishd/cache/cache_req_fsm.c b/bin/varnishd/cache/cache_req_fsm.c
index 00a02491b..b55a0c007 100644
--- a/bin/varnishd/cache/cache_req_fsm.c
+++ b/bin/varnishd/cache/cache_req_fsm.c
@@ -209,8 +209,8 @@ cnt_vclfail(const struct worker *wrk, struct req *req)
 	AZ(req->objcore);
 	AZ(req->stale_oc);
 
-	HTTP_Copy(req->http, req->http0);
-	WS_Reset(req->ws, req->ws_req);
+	Req_Rollback(req);
+
 	req->err_code = 503;
 	req->err_reason = "VCL failed";
 	req->req_step = R_STP_SYNTH;
@@ -857,8 +857,7 @@ cnt_recv(struct worker *wrk, struct req *req)
 
 	VCL_recv_method(req->vcl, wrk, req, NULL, NULL);
 	if (wrk->handling == VCL_RET_VCL && req->restarts == 0) {
-		HTTP_Copy(req->http, req->http0);
-		WS_Reset(req->ws, req->ws_req);
+		Req_Rollback(req);
 		cnt_recv_prep(req, ci);
 		VCL_recv_method(req->vcl, wrk, req, NULL, NULL);
 	}
diff --git a/bin/varnishd/cache/cache_varnishd.h b/bin/varnishd/cache/cache_varnishd.h
index 111c6530f..af122f768 100644
--- a/bin/varnishd/cache/cache_varnishd.h
+++ b/bin/varnishd/cache/cache_varnishd.h
@@ -314,6 +314,7 @@ void VRG_dorange(struct req *req, const char *r);
 /* cache_req.c */
 struct req *Req_New(const struct worker *, struct sess *);
 void Req_Release(struct req *);
+void Req_Rollback(struct req *req);
 void Req_Cleanup(struct sess *sp, struct worker *wrk, struct req *req);
 void Req_Fail(struct req *req, enum sess_close reason);
 void Req_AcctLogCharge(struct VSC_main *, struct req *);
diff --git a/bin/varnishd/cache/cache_vrt.c b/bin/varnishd/cache/cache_vrt.c
index 433ffd02f..8a31ee157 100644
--- a/bin/varnishd/cache/cache_vrt.c
+++ b/bin/varnishd/cache/cache_vrt.c
@@ -519,10 +519,11 @@ VRT_Rollback(VRT_CTX, VCL_HTTP hp)
 	CHECK_OBJ_NOTNULL(hp, HTTP_MAGIC);
 	if (hp == ctx->http_req) {
 		CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC);
-		HTTP_Copy(ctx->req->http, ctx->req->http0);
-		WS_Reset(ctx->req->ws, ctx->req->ws_req);
+		Req_Rollback(ctx->req);
 	} else if (hp == ctx->http_bereq) {
 		CHECK_OBJ_NOTNULL(ctx->bo, BUSYOBJ_MAGIC);
+		// -> VBO_Rollback ?
+		VRTPRIV_dynamic_kill(ctx->bo->privs, (uintptr_t)ctx->bo);
 		HTTP_Copy(ctx->bo->bereq, ctx->bo->bereq0);
 		WS_Reset(ctx->bo->bereq->ws, ctx->bo->ws_bo);
 		WS_Reset(ctx->bo->ws, ctx->bo->ws_bo);
diff --git a/bin/varnishtest/tests/m00000.vtc b/bin/varnishtest/tests/m00000.vtc
index 7bdaa02b1..5c0fa7536 100644
--- a/bin/varnishtest/tests/m00000.vtc
+++ b/bin/varnishtest/tests/m00000.vtc
@@ -16,7 +16,15 @@ varnish v1 -vcl+backend {
 		debug.vsc_new();
 	}
 
+	sub vcl_synth {
+		set req.http.overwrite = "the workspace " +
+		    "to ensure we notice any unfinished privs";
+	}
 	sub vcl_recv {
+		if (req.url == "/fail") {
+			debug.test_priv_task("foo");
+			return (fail);
+		}
 		debug.rot52(req);
 		debug.vsc_count();
 	}
@@ -52,6 +60,9 @@ client c1 {
 	expect resp.http.encrypted == "ROT52"
 	expect resp.http.what >= 16
 	expect resp.http.not == -1
+	txreq -url "/fail"
+	rxresp
+	expect resp.status == 503
 } -run
 
 varnish v1 -expect DEBUG.count == 1