Call trimstore only once when copying the body after a 304

It is my understanding that the objtrimstore stevedore API function is
only to be called once when the object is complete, which I believe is
also in line with the comment on ObjExtend() that "The final flag must
be set on the last call".

If this understanding of the API is correct, we did not adhere to it in
the fetch code when we made a copy of an existing stale object after a
304 response: There, we iterated over the stale object and did not set
the final flag just once when the object was complete, but rather after
each storage segment was copied.

This commit fixes this, adds some pedentry to the simple storage and
extends b00062.vtc to test this behavior specifically. On top, g6.vtc
also triggered without the fix but the duplicate trim detection in place.

This issue has originally surfaced in the SLASH/fellow storage where the
trimstore function implicitly asserted to only be called once.

Ref 115742b0
Ref https://gitlab.com/uplex/varnish/slash/-/issues/33

bin/varnishd/cache/cache_fetch.c

@@ -769,7 +769,8 @@ vbf_objiterate(void *priv, unsigned flush, const void *ptr, ssize_t len)
 	uint8_t *pd;
 
 	CAST_OBJ_NOTNULL(bo, priv, BUSYOBJ_MAGIC);
-	(void)flush;
+
+	flush &= OBJ_ITER_END;
 
 	while (len > 0) {
 		l = len;
@@ -777,7 +778,7 @@ vbf_objiterate(void *priv, unsigned flush, const void *ptr, ssize_t len)
 			return (1);
 		l = vmin(l, len);
 		memcpy(pd, ps, l);
-		VFP_Extend(bo->vfc, l, l == len ? VFP_END : VFP_OK);
+		VFP_Extend(bo->vfc, l, flush && l == len ? VFP_END : VFP_OK);
 		ps += l;
 		len -= l;
 	}

bin/varnishd/storage/storage_simple.c

@@ -44,6 +44,9 @@
 /* Flags for allocating memory in sml_stv_alloc */
 #define LESS_MEM_ALLOCED_IS_OK	1
 
+// marker pointer for sml_trimstore
+static void *trim_once = &trim_once;
+
 /*-------------------------------------------------------------------*/
 
 static struct storage *
@@ -257,7 +260,8 @@ sml_bocfini(const struct stevedore *stv, struct boc *boc)
 	CHECK_OBJ_NOTNULL(stv, STEVEDORE_MAGIC);
 	CHECK_OBJ_NOTNULL(boc, BOC_MAGIC);
 
-	if (boc->stevedore_priv == NULL)
+	if (boc->stevedore_priv == NULL ||
+	    boc->stevedore_priv == trim_once)
 		return;
 
 	/* Free any leftovers from Trim */
@@ -533,6 +537,10 @@ sml_trimstore(struct worker *wrk, struct objcore *oc)
 	stv = oc->stobj->stevedore;
 	CHECK_OBJ_NOTNULL(stv, STEVEDORE_MAGIC);
 
+	if (oc->boc->stevedore_priv != NULL)
+		WRONG("sml_trimstore already called");
+	oc->boc->stevedore_priv = trim_once;
+
 	if (stv->sml_free == NULL)
 		return;
 
@@ -566,7 +574,6 @@ sml_trimstore(struct worker *wrk, struct objcore *oc)
 	VTAILQ_INSERT_HEAD(&o->list, st1, list);
 	Lck_Unlock(&oc->boc->mtx);
 	/* sml_bocdone frees this */
-	AZ(oc->boc->stevedore_priv);
 	oc->boc->stevedore_priv = st;
 }
 

bin/varnishtest/tests/b00062.vtc

@@ -2,7 +2,11 @@ varnishtest "Test that we properly wait for certain 304 cases"
 
 server s1 {
 	rxreq
-	txresp -hdr "Last-Modified: Wed, 11 Sep 2013 13:36:55 GMT" -body "Geoff Still Rules"
+	txresp -hdr "Last-Modified: Wed, 11 Sep 2013 13:36:55 GMT" \
+	       -hdr "Geoff: Still Rules" \
+	       -bodylen 130560
+
+	       # 2*64k-512 ^^^ see sml_trimstore() st->space - st->len < 512
 
 	# The IMS request we will spend some time to process for the sake of
 	# this test.
@@ -17,7 +21,7 @@ server s1 {
 	txresp -body "x"
 } -start
 
-varnish v1 -vcl+backend {
+varnish v1 -arg "-p fetch_maxchunksize=64k" -vcl+backend {
 	sub vcl_backend_response {
 		set beresp.ttl = 1s;
 		set beresp.grace = 1s;
@@ -30,7 +34,8 @@ client c1 {
 	txreq
 	rxresp
 	expect resp.status == 200
-	expect resp.body == "Geoff Still Rules"
+	expect resp.http.Geoff == "Still Rules"
+	expect resp.bodylen == 130560
 } -run
 
 # let the object's ttl and grace expire
@@ -42,7 +47,8 @@ client c2 {
 	rxresp
 	# we did not disable grace in the request, so we should get the graced object here
 	expect resp.status == 200
-	expect resp.body == "Geoff Still Rules"
+	expect resp.http.Geoff == "Still Rules"
+	expect resp.bodylen == 130560
 } -start
 
 delay .1
@@ -52,7 +58,8 @@ client c3 {
 	txreq
 	rxresp
 	expect resp.status == 200
-	expect resp.body == "Geoff Still Rules"
+	expect resp.http.Geoff == "Still Rules"
+	expect resp.bodylen == 130560
 } -start
 
 client c2 -wait