[master] c98edbb finalize task privs when rolling back workspaces

[Nils Goroll]

commit c98edbb0214f6d64b7822def58036cd67e3bdb1d
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 a6b9ee0..d741441 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 b36e85d..5a5e5eb 100644
--- a/bin/varnishd/cache/cache_req_fsm.c
+++ b/bin/varnishd/cache/cache_req_fsm.c
@@ -208,8 +208,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;
@@ -856,8 +856,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 d2f35e3..292e336 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 adeb349..bc92daa 100644
--- a/bin/varnishd/cache/cache_vrt.c
+++ b/bin/varnishd/cache/cache_vrt.c
@@ -518,10 +518,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 7bdaa02..5c0fa75 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