[master] 89c1688f8 Call trimstore only once when copying the body after a 304

[Dridi Boukelmoune]

commit 89c1688f8845f7630dcc0955ed748861156d74c2
Author: Nils Goroll <nils.goroll at uplex.de>
Date:   Tue Oct 31 13:46:37 2023 +0100

    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 115742b07c8bad6d465f1c981ee264f934a4492b
    Ref https://gitlab.com/uplex/varnish/slash/-/issues/33

diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index 55c14f1d4..e365f6c88 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/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;
 	}
diff --git a/bin/varnishd/storage/storage_simple.c b/bin/varnishd/storage/storage_simple.c
index c94dcccb4..b91fbcb32 100644
--- a/bin/varnishd/storage/storage_simple.c
+++ b/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;
 }
 
diff --git a/bin/varnishtest/tests/b00062.vtc b/bin/varnishtest/tests/b00062.vtc
index 511a4b416..4a828a53b 100644
--- a/bin/varnishtest/tests/b00062.vtc
+++ b/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