[master] 5b4f0f1ad htc: Defer workspace rollbacks for request tasks
Dridi Boukelmoune
dridi.boukelmoune at gmail.com
Fri Aug 27 15:30:09 UTC 2021
commit 5b4f0f1addaab9c4cedb5a2e0cdcc97fe58836d4
Author: Dridi Boukelmoune <dridi.boukelmoune at gmail.com>
Date: Wed Aug 25 16:17:23 2021 +0200
htc: Defer workspace rollbacks for request tasks
When we take on a new request on a connection from which something was
already received, we need to pipeline it and we do so at the beginning
of the request workspace. There's a high probability that the pipeline
is coming from the same workspace, which is a form of use-after-free
only made safe by the workspace implementation details.
To avoid the conceptual use-after-free, we defer req workspace rollbacks
and perform them during the next HTC_RxInit() call before the pipelining
operation.
Because HTTP/1 works directly on the session, a worker can safely switch
back and forth between sess and req tasks. This means that unless the
session goes idle the same workspace is used from one client request
to the next, hence the rollback previously happening in Req_Cleanup().
With h2 however there is a disconnect between the session and streams.
The connection is received in req0's workspace, and then copied into
a stream's req workspace via the pipelining scheme. Rollbacks can be
deferred as well, but they need to happen otherwise the session will
soon overflow. Independent HTC_RxInit() calls happen for req0 in the
h2 session thread, and for h2 streams in the regular request task code
path.
PROXY Protocol parsing may result in receiving more than the proxy
preamble itself and pipelining will happen, whether it is via a req
for HTTP/1 or req0 for h2.
On the other end of the spectrum when Varnish acts as a client it
only sends one HTTP/1 request at a time for a given connection, so
we never expect pipelining to occur in fetch task.
diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index 8c641e5e4..6f6f3d637 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/bin/varnishd/cache/cache_fetch.c
@@ -153,7 +153,6 @@ Bereq_Rollback(VRT_CTX)
HTTP_Clone(bo->bereq, bo->bereq0);
bo->vfp_filter_list = NULL;
bo->err_reason = NULL;
- 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 eeb339b62..5a5e752f7 100644
--- a/bin/varnishd/cache/cache_req.c
+++ b/bin/varnishd/cache/cache_req.c
@@ -261,10 +261,6 @@ Req_Cleanup(struct sess *sp, struct worker *wrk, struct req *req)
if (WS_Overflowed(req->ws))
wrk->stats->ws_client_overflow++;
-
- /* no snapshot for h2 stream 0 */
- if (req->ws_req)
- WS_Rollback(req->ws, req->ws_req);
}
/*----------------------------------------------------------------------
diff --git a/bin/varnishd/cache/cache_session.c b/bin/varnishd/cache/cache_session.c
index f8215dc13..d6a711937 100644
--- a/bin/varnishd/cache/cache_session.c
+++ b/bin/varnishd/cache/cache_session.c
@@ -228,6 +228,12 @@ HTC_RxInit(struct http_conn *htc, struct ws *ws)
CHECK_OBJ_NOTNULL(htc, HTTP_CONN_MAGIC);
htc->ws = ws;
+
+ if (!strcasecmp(ws->id, "req"))
+ WS_Rollback(ws, 0);
+ else
+ AZ(htc->pipeline_b);
+
r = WS_ReserveAll(htc->ws);
htc->rxbuf_b = htc->rxbuf_e = WS_Reservation(ws);
if (htc->pipeline_b != NULL) {
diff --git a/bin/varnishd/http2/cache_http2_session.c b/bin/varnishd/http2/cache_http2_session.c
index 88bc74a95..21b9a0e4e 100644
--- a/bin/varnishd/http2/cache_http2_session.c
+++ b/bin/varnishd/http2/cache_http2_session.c
@@ -368,7 +368,6 @@ 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_Rollback(h2->ws, 0);
HTC_RxInit(h2->htc, h2->ws);
AN(WS_Reservation(h2->ws));
VSLb(h2->vsl, SLT_Debug, "H2: Got pu PRISM");
@@ -390,7 +389,6 @@ h2_new_session(struct worker *wrk, void *arg)
h2->cond = &wrk->cond;
while (h2_rxframe(wrk, h2)) {
- 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 29e806826..296e0e20b 100644
--- a/bin/varnishd/proxy/cache_proxy_proto.c
+++ b/bin/varnishd/proxy/cache_proxy_proto.c
@@ -151,7 +151,6 @@ 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_Rollback(req->htc->ws, 0);
return (0);
}
@@ -353,7 +352,6 @@ 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_Rollback(req->ws, 0);
p = (const void *)req->htc->rxbuf_b;
d = req->htc->rxbuf_b + 16L;
diff --git a/bin/varnishtest/tests/c00108.vtc b/bin/varnishtest/tests/c00108.vtc
new file mode 100644
index 000000000..9c2014329
--- /dev/null
+++ b/bin/varnishtest/tests/c00108.vtc
@@ -0,0 +1,53 @@
+varnishtest "Session pipelining exceeding available workspace"
+
+server s1 {
+ loop 2 {
+ rxreq
+ expect req.bodylen == 32769
+ txresp
+ }
+
+ rxreq
+ expect req.bodylen == 32768
+ txresp
+} -start
+
+varnish v1 -cliok "param.set workspace_client 24k"
+varnish v1 -cliok "param.set http_req_size 1k"
+varnish v1 -cliok "param.set http_resp_size 1k"
+varnish v1 -cliok "param.set vsl_buffer 1k"
+varnish v1 -vcl+backend { } -start
+
+client c1 {
+ # Multi-line strings aren't escaped, the send argument
+ # below contains actual carriage returns. The extra body
+ # byte on top of the 32kB is a line feed.
+ send {
+POST / HTTP/1.1
+host: ${localhost}
+content-length: 32769
+
+${string,repeat,1024,abcdefghijklmnopqrstuvwxyz012345}
+POST / HTTP/1.1
+host: ${localhost}
+content-length: 32769
+
+${string,repeat,1024,abcdefghijklmnopqrstuvwxyz012345}
+}
+
+ loop 2 {
+ rxresp
+ expect resp.status == 200
+ }
+} -run
+
+varnish v1 -cliok "param.set feature +http2"
+
+client c2 {
+ stream 1 {
+ txreq -method POST -hdr content-length 32768 -nostrend
+ txdata -datalen 16384 -nostrend
+ txdata -datalen 16384
+ rxresp
+ } -run
+} -run
More information about the varnish-commit
mailing list