[master] 896151b41 An overflowed workspace must remain overflowed after WS_Reset()
Dridi Boukelmoune
dridi.boukelmoune at gmail.com
Tue Mar 3 10:02:10 UTC 2020
commit 896151b411bda1674112d859e9bc3eb4ad611038
Author: Nils Goroll <nils.goroll at uplex.de>
Date: Mon Jan 27 16:42:48 2020 +0100
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
diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index 7bc595613..dd51a468e 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/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);
}
/*--------------------------------------------------------------------
diff --git a/bin/varnishd/cache/cache_req.c b/bin/varnishd/cache/cache_req.c
index 122dfdb6d..d9ad87203 100644
--- a/bin/varnishd/cache/cache_req.c
+++ b/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);
}
/*----------------------------------------------------------------------
diff --git a/bin/varnishd/cache/cache_varnishd.h b/bin/varnishd/cache/cache_varnishd.h
index 6e98f425d..a9050a099 100644
--- a/bin/varnishd/cache/cache_varnishd.h
+++ b/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);
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index fe8758a6b..6064a9c08 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/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;
diff --git a/bin/varnishd/cache/cache_wrk.c b/bin/varnishd/cache/cache_wrk.c
index d7fb95530..b4ad19b90 100644
--- a/bin/varnishd/cache/cache_wrk.c
+++ b/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);
diff --git a/bin/varnishd/cache/cache_ws.c b/bin/varnishd/cache/cache_ws.c
index 39a91992f..3da9ba732 100644
--- a/bin/varnishd/cache/cache_ws.c
+++ b/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);
}
/*
diff --git a/bin/varnishd/http1/cache_http1_line.c b/bin/varnishd/http1/cache_http1_line.c
index 88ff2ffd9..a6914d3e6 100644
--- a/bin/varnishd/http1/cache_http1_line.c
+++ b/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);
}
diff --git a/bin/varnishd/http2/cache_http2_deliver.c b/bin/varnishd/http2/cache_http2_deliver.c
index a1cdbf9c1..6996e879a 100644
--- a/bin/varnishd/http2/cache_http2_deliver.c
+++ b/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) {
diff --git a/bin/varnishd/http2/cache_http2_session.c b/bin/varnishd/http2/cache_http2_session.c
index fec88569b..543eea868 100644
--- a/bin/varnishd/http2/cache_http2_session.c
+++ b/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");
diff --git a/bin/varnishd/proxy/cache_proxy_proto.c b/bin/varnishd/proxy/cache_proxy_proto.c
index a9b267458..4eb5526ee 100644
--- a/bin/varnishd/proxy/cache_proxy_proto.c
+++ b/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;
More information about the varnish-commit
mailing list