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
beeaa19c, 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

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) {

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);
 	}

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 *);

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);

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