An overflowed workspace must remain overflowed after WS_Reset()

We use workspace overflows to signal to bail out for example after a
failing `VRT_SetHdr()`. This is a guarantee that if some serious issue
occurred during processing, we rather send an error downstream than an
incomplete response or the result of incomplete processing.

We use the `WS_Snapshot() ...  WS_Reset()` pattern as some kind of
second order workspace allocation where the called code itself uses
`WS_Reserve()`.

With this usage pattern, `WS_Reset()` called `ws_ClearOverflow(ws)`,
potentially clearing the overflow bit from a previous relevant
failure.

We now avoid any other unintended clears of the overflow bit by
splitting two functions:

* WS_Rollback() is now what WS_Reset() used to be: It clears overflows
  and accepts the zero cookie for a reset-to-start

  It is only intended for use within varnishd and is thus declared
  in cache_varnishd.h

* WS_Reset() does not touch the overflow bit any longer, ensuring that
  a once-overflowed workspace stays overflowed

`WS_Snapshot()` now returns a magic value which gets recognized by
`WS_Reset()` to ensure that the overflowed marker is still present.
This serves two purposes:

- better debugging and

- a safety measure against passing a cookie from an already overflowed
  workspace to WS_Rollback()

Fixes #3194

bin/varnishd/cache/cache_fetch.c

@@ -110,8 +110,8 @@ void Bereq_Rollback(struct busyobj *bo)
 	VCL_TaskLeave(bo->privs);
 	VCL_TaskEnter(bo->privs);
 	HTTP_Clone(bo->bereq, bo->bereq0);
-	WS_Reset(bo->bereq->ws, bo->ws_bo);
-	WS_Reset(bo->ws, bo->ws_bo);
+	WS_Rollback(bo->bereq->ws, bo->ws_bo);
+	WS_Rollback(bo->ws, bo->ws_bo);
 }
 
 /*--------------------------------------------------------------------

bin/varnishd/cache/cache_req.c

@@ -200,7 +200,7 @@ Req_Rollback(struct req *req)
 	HTTP_Clone(req->http, req->http0);
 	if (WS_Overflowed(req->ws))
 		req->wrk->stats->ws_client_overflow++;
-	WS_Reset(req->ws, req->ws_req);
+	WS_Rollback(req->ws, req->ws_req);
 }
 
 /*----------------------------------------------------------------------
@@ -251,7 +251,7 @@ Req_Cleanup(struct sess *sp, struct worker *wrk, struct req *req)
 	if (WS_Overflowed(req->ws))
 		wrk->stats->ws_client_overflow++;
 
-	WS_Reset(req->ws, 0);
+	WS_Rollback(req->ws, 0);
 }
 
 /*----------------------------------------------------------------------

bin/varnishd/cache/cache_varnishd.h

@@ -463,6 +463,9 @@ void VMOD_Panic(struct vsb *);
 /* cache_wrk.c */
 void WRK_Init(void);
 
+/* cache_ws.c */
+void WS_Rollback(struct ws *, uintptr_t);
+
 /* http1/cache_http1_pipe.c */
 void V1P_Init(void);
 

bin/varnishd/cache/cache_vcl.c

@@ -163,7 +163,7 @@ VCL_Rel_CliCtx(struct vrt_ctx **ctx)
 	if (ctx_cli.vsl)
 		VSL_Flush(ctx_cli.vsl, 0);
 	WS_Assert(ctx_cli.ws);
-	WS_Reset(&ws_cli, ws_snapshot_cli);
+	WS_Rollback(&ws_cli, ws_snapshot_cli);
 	INIT_OBJ(*ctx, VRT_CTX_MAGIC);
 	*ctx = NULL;
 

bin/varnishd/cache/cache_wrk.c

@@ -348,7 +348,7 @@ Pool_Work_Thread(struct pool *pp, struct worker *wrk)
 		CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
 		tp = NULL;
 
-		WS_Reset(wrk->aws, 0);
+		WS_Rollback(wrk->aws, 0);
 		AZ(wrk->vsl);
 
 		Lck_Lock(&pp->mtx);

bin/varnishd/cache/cache_ws.c

@@ -36,6 +36,8 @@
 
 #include <stdio.h>
 
+static const void * const snap_overflowed = &snap_overflowed;
+
 void
 WS_Assert(const struct ws *ws)
 {
@@ -127,7 +129,12 @@ ws_ClearOverflow(struct ws *ws)
 }
 
 /*
- * Reset a WS to start or a given pointer, likely from WS_Snapshot
+ * Reset a WS to a cookie from WS_Snapshot
+ *
+ * for use by any code using cache.h
+ *
+ * does not reset the overflow bit and asserts that, if WS_Snapshot had found
+ * the workspace overflown, the marker is intact
  */
 
 void
@@ -136,20 +143,39 @@ WS_Reset(struct ws *ws, uintptr_t pp)
 	char *p;
 
 	WS_Assert(ws);
+	AN(pp);
+	if (pp == (uintptr_t)snap_overflowed) {
+		DSL(DBG_WORKSPACE, 0, "WS_Reset(%p, overflowed)", ws);
+		AN(WS_Overflowed(ws));
+		return;
+	}
 	p = (char *)pp;
 	DSL(DBG_WORKSPACE, 0, "WS_Reset(%p, %p)", ws, p);
 	assert(ws->r == NULL);
-	if (p == NULL)
-		ws->f = ws->s;
-	else {
-		assert(p >= ws->s);
-		assert(p <= ws->e);
-		ws->f = p;
-	}
-	ws_ClearOverflow(ws);
+	assert(p >= ws->s);
+	assert(p <= ws->e);
+	ws->f = p;
 	WS_Assert(ws);
 }
 
+/*
+ * Reset the WS to a cookie or its start and clears any overflow
+ *
+ * for varnishd internal use only
+ */
+
+void
+WS_Rollback(struct ws *ws, uintptr_t pp)
+{
+	WS_Assert(ws);
+
+	if (pp == 0)
+		pp = (uintptr_t)ws->s;
+
+	ws_ClearOverflow(ws);
+	WS_Reset(ws, pp);
+}
+
 void *
 WS_Alloc(struct ws *ws, unsigned bytes)
 {
@@ -224,8 +250,12 @@ WS_Snapshot(struct ws *ws)
 
 	WS_Assert(ws);
 	assert(ws->r == NULL);
+	if (WS_Overflowed(ws)) {
+		DSL(DBG_WORKSPACE, 0, "WS_Snapshot(%p) = overflowed", ws);
+		return ((uintptr_t) snap_overflowed);
+	}
 	DSL(DBG_WORKSPACE, 0, "WS_Snapshot(%p) = %p", ws, ws->f);
-	return (ws->f == ws->s ? 0 : (uintptr_t)ws->f);
+	return ((uintptr_t)ws->f);
 }
 
 /*

bin/varnishd/http1/cache_http1_line.c

@@ -139,7 +139,7 @@ V1L_Close(struct worker *wrk, uint64_t *cnt)
 	*cnt = v1l->cnt;
 	if (v1l->ws->r)
 		WS_Release(v1l->ws, 0);
-	WS_Reset(v1l->ws, v1l->res);
+	WS_Rollback(v1l->ws, v1l->res);
 	ZERO_OBJ(v1l, sizeof *v1l);
 	return (sc);
 }

bin/varnishd/http2/cache_http2_deliver.c

@@ -321,8 +321,7 @@ h2_deliver(struct req *req, struct boc *boc, int sendbody)
 	    sz, r, &req->acct.resp_hdrbytes);
 	H2_Send_Rel(r2->h2sess, r2);
 
-	if (!WS_Overflowed(req->ws))	// XXX: remove if when #3202 is fixed
-		WS_Reset(req->ws, ss);
+	WS_Reset(req->ws, ss);
 
 	/* XXX someone into H2 please add appropriate error handling */
 	if (sendbody) {

bin/varnishd/http2/cache_http2_session.c

@@ -365,7 +365,7 @@ h2_new_session(struct worker *wrk, void *arg)
 	}
 	assert(HTC_S_COMPLETE == H2_prism_complete(h2->htc));
 	HTC_RxPipeline(h2->htc, h2->htc->rxbuf_b + sizeof(H2_prism));
-	WS_Reset(h2->ws, 0);
+	WS_Rollback(h2->ws, 0);
 	HTC_RxInit(h2->htc, h2->ws);
 	AN(h2->ws->r);
 	VSLb(h2->vsl, SLT_Debug, "H2: Got pu PRISM");
@@ -387,7 +387,7 @@ h2_new_session(struct worker *wrk, void *arg)
 	h2->cond = &wrk->cond;
 
 	while (h2_rxframe(wrk, h2)) {
-		WS_Reset(h2->ws, 0);
+		WS_Rollback(h2->ws, 0);
 		HTC_RxInit(h2->htc, h2->ws);
 		if (WS_Overflowed(h2->ws)) {
 			VSLb(h2->vsl, SLT_Debug, "H2: Empty Rx Workspace");

bin/varnishd/proxy/cache_proxy_proto.c

@@ -145,7 +145,7 @@ vpx_proto1(const struct worker *wrk, const struct req *req)
 	VSL(SLT_Proxy, req->sp->vxid, "1 %s %s %s %s",
 	    fld[1], fld[3], fld[2], fld[4]);
 	HTC_RxPipeline(req->htc, q);
-	WS_Reset(req->htc->ws, 0);
+	WS_Rollback(req->htc->ws, 0);
 	return (0);
 }
 
@@ -345,7 +345,7 @@ vpx_proto2(const struct worker *wrk, struct req *req)
 	hdr_len = l + 16L;
 	assert(req->htc->rxbuf_e - req->htc->rxbuf_b >= hdr_len);
 	HTC_RxPipeline(req->htc, req->htc->rxbuf_b + hdr_len);
-	WS_Reset(req->ws, 0);
+	WS_Rollback(req->ws, 0);
 	p = (const void *)req->htc->rxbuf_b;
 	d = req->htc->rxbuf_b + 16L;