cache_req_fsm: Keep object cores referenced during the task

Context:

Only while we hold a reference to an object are we allowed to access any
attributes from it. With restarts, we might want to keep references to
attributes of objects from previous restart iterations. Before this patch, this
could lead to use-after-dereference, which was "fixed" by always copying object
variables into the request's workspace.

But that is wasteful, so we should rather make sure that we keep references
until we are done with the task and not copy (which is the next commit).

An argument had been made in the past that keeping object references until the
end of the task would increase their lifetime by too much, but restarts in VCL
really should be done within milliseconds in most cases - and if keeping
references _is_ an actual problem in specific situations, those can be avoided
by either not restarting or rolling back also. In general, until now we have
charged all Varnish-Cache users with the cost for specific use cases, but we
should rather only charge the specific use cases instead.

Implementation:

stash_oc() allocates basically a Variable Length Array (VLA) on the workspace
with a fallback to heap. The fallback exists to not complicate the calling code
with additional error handling.

For each invocation, stash_oc() copies the passed objcore to one of max_restarts
+ 1 slots and clears the original location as HSH_DerefObjCore() would.

Alternatives considered:

* Dynamically allocating space for each objcore pointer was dismissed due to the
  substantial overhead.

* Using a TASK_PRIV was tried and dismissed because this would require setting
  up a VRT_CTX or pulling the VRT_CTX out one level to the core of the FSM,
  which was intrusive and increased complexity substantially.

Author: nigoroll

bin/varnishd/cache/cache.h

@@ -117,6 +117,7 @@ struct cli;
 struct http_conn;
 struct listen_sock;
 struct mempool;
+struct ocstash;
 struct objcore;
 struct objhead;
 struct pool;
@@ -548,6 +549,8 @@ struct req {
 	struct vrt_privs	privs[1];
 
 	struct vcf		*vcf;
+
+	struct ocstash		*ocstash;
 };
 
 #define IS_TOPREQ(req) ((req)->top->topreq == (req))

bin/varnishd/cache/cache_req_fsm.c

@@ -40,6 +40,8 @@
 
 #include "config.h"
 
+#include <stdlib.h>
+
 #include "cache_varnishd.h"
 #include "cache_filter.h"
 #include "cache_objhead.h"
@@ -78,6 +80,68 @@
 REQ_STEPS
 #undef REQ_STEP
 
+/*--------------------------------------------------------------------
+ * Remember additional objcores to deref when the task ends
+ */
+
+struct ocstash {
+	unsigned		magic;
+#define OCSTASH_MAGIC		0xe361b34d
+	unsigned		malloced;
+	unsigned		n;
+	unsigned		l;
+	struct objcore *	ocs[] v_counted_by_(l);
+};
+
+static void
+ocstash_fini(struct worker *wrk, struct ocstash **stashp)
+{
+	struct ocstash *stash;
+	unsigned u;
+
+	AN(stashp);
+	if (*stashp == NULL)
+		return;
+	TAKE_OBJ_NOTNULL(stash, stashp, OCSTASH_MAGIC);
+	assert(stash->n <= stash->l);
+	for (u = 0; u < stash->n; u++)
+		(void)HSH_DerefObjCore(wrk, &stash->ocs[u], HSH_RUSH_POLICY);
+	if (stash->malloced)
+		free(stash);
+}
+
+static inline size_t
+stash_sz(unsigned cap)
+{
+	return (offsetof(struct ocstash, ocs) + sizeof(struct objcore *) * cap);
+}
+
+// never fails unless malloc() fails
+static void
+stash_oc(struct ocstash **stashp, struct objcore **ocp, struct ws *ws, unsigned l)
+{
+	struct ocstash *stash;
+
+	AN(stashp);
+	AN(ocp);
+
+	stash = *stashp;
+	if (stash == NULL) {
+		stash = WS_Alloc(ws, (unsigned)stash_sz(l));
+		if (stash == NULL)
+			stash = malloc(stash_sz(l));
+		AN(stash);
+		memset(stash, 0, stash_sz(l));
+		stash->magic = OCSTASH_MAGIC;
+		stash->l = l;
+		*stashp = stash;
+	}
+	CHECK_OBJ(stash, OCSTASH_MAGIC);
+	assert(stash->n < stash->l);
+	TAKE_OBJ_NOTNULL(stash->ocs[stash->n], ocp, OBJCORE_MAGIC);
+	stash->n++;
+}
+
 /*--------------------------------------------------------------------
  * Handle "Expect:" and "Connection:" on incoming request
  */
@@ -240,7 +304,7 @@ cnt_deliver(struct worker *wrk, struct req *req)
 
 	if (wrk->vpi->handling != VCL_RET_DELIVER) {
 		HSH_Cancel(wrk, req->objcore, NULL);
-		(void)HSH_DerefObjCore(wrk, &req->objcore, HSH_RUSH_POLICY);
+		stash_oc(&req->ocstash, &req->objcore, req->ws, req->max_restarts + 1);
 		http_Teardown(req->resp);
 
 		switch (wrk->vpi->handling) {
@@ -286,6 +350,8 @@ cnt_vclfail(struct worker *wrk, struct req *req)
 	AZ(req->objcore);
 	AZ(req->stale_oc);
 
+	ocstash_fini(wrk, &req->ocstash);
+
 	INIT_OBJ(ctx, VRT_CTX_MAGIC);
 	VCL_Req2Ctx(ctx, req);
 
@@ -557,6 +623,7 @@ cnt_fetch(struct worker *wrk, struct req *req)
 	if (req->objcore->flags & OC_F_FAILED) {
 		req->err_code = 503;
 		req->req_step = R_STP_SYNTH;
+		// VCL did not get to see this object, no need to stash
 		(void)HSH_DerefObjCore(wrk, &req->objcore, 1);
 		AZ(req->objcore);
 		return (REQ_FSM_MORE);
@@ -1213,6 +1280,7 @@ CNT_Request(struct req *req)
 	}
 	wrk->vsl = NULL;
 	if (nxt == REQ_FSM_DONE) {
+		ocstash_fini(wrk, &req->ocstash);
 		INIT_OBJ(ctx, VRT_CTX_MAGIC);
 		VCL_Req2Ctx(ctx, req);
 		if (IS_TOPREQ(req)) {

vmod/tests/std_c00001.vtc

@@ -299,7 +299,7 @@ varnish v1 -vcl+backend {
 			set req.http.response = "123";
 			set req.http.response2 = "Another response";
 			if (req.url == "/9") {
-				vtc.workspace_alloc(client, -200);
+				vtc.workspace_alloc(client, -256);
 			} else if (req.url == "/10") {
 				vtc.workspace_alloc(client, -10);
 				std.rollback(req);