[master] 3446fb1 Ensure bereq changes are not lost across vcl_backend_xxx methods

Federico G. Schwindt fgsch at lodoss.net
Thu Jul 31 13:55:11 CEST 2014


commit 3446fb1c563357054c6aa93a08c946ce080c26f1
Author: Federico G. Schwindt <fgsch at lodoss.net>
Date:   Thu Jul 31 12:48:03 2014 +0100

    Ensure bereq changes are not lost across vcl_backend_xxx methods
    
    Add std.rollback() to rollback req or bereq and mark the builtin rollback
    as deprecated.
    
    Original diff by Nils Goroll. Tweaks and reworked after phk comments
    by me.
    
    Fixes #1512

diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h
index f21c57d..e171d66 100644
--- a/bin/varnishd/cache/cache.h
+++ b/bin/varnishd/cache/cache.h
@@ -490,6 +490,7 @@ struct busyobj {
 	enum busyobj_state_e	state;
 
 	struct ws		ws[1];
+	char 			*ws_bo;
 	struct vbc		*vbc;
 	struct http		*bereq0;
 	struct http		*bereq;
diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index 1e45392..70f467a 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/bin/varnishd/cache/cache_fetch.c
@@ -220,6 +220,10 @@ vbf_stp_mkbereq(const struct worker *wrk, struct busyobj *bo)
 		}
 	}
 
+	HTTP_Setup(bo->bereq, bo->ws, bo->vsl, SLT_BereqMethod);
+	bo->ws_bo = WS_Snapshot(bo->ws);
+	HTTP_Copy(bo->bereq, bo->bereq0);
+
 	VBO_setstate(bo, BOS_REQ_DONE);
 	return (F_STP_STARTFETCH);
 }
@@ -267,9 +271,6 @@ vbf_stp_startfetch(struct worker *wrk, struct busyobj *bo)
 	else
 		AZ(bo->req);
 
-	HTTP_Setup(bo->bereq, bo->ws, bo->vsl, SLT_BereqMethod);
-	HTTP_Copy(bo->bereq, bo->bereq0);
-
 	http_PrintfHeader(bo->bereq, "X-Varnish: %u", VXID(bo->vsl->wid));
 
 	VCL_backend_fetch_method(bo->vcl, wrk, NULL, bo, bo->bereq->ws);
diff --git a/bin/varnishd/cache/cache_vrt.c b/bin/varnishd/cache/cache_vrt.c
index 4038693..edfa2a5 100644
--- a/bin/varnishd/cache/cache_vrt.c
+++ b/bin/varnishd/cache/cache_vrt.c
@@ -357,13 +357,22 @@ VRT_BOOL_string(unsigned val)
 /*--------------------------------------------------------------------*/
 
 void
-VRT_Rollback(const struct vrt_ctx *ctx)
+VRT_Rollback(const struct vrt_ctx *ctx, const struct http *hp)
 {
 
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
-	CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC);
-	HTTP_Copy(ctx->req->http, ctx->req->http0);
-	WS_Reset(ctx->req->ws, ctx->req->ws_req);
+	CHECK_OBJ_NOTNULL(hp, HTTP_MAGIC);
+	if (hp == ctx->http_req) {
+		CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC);
+		HTTP_Copy(ctx->req->http, ctx->req->http0);
+		WS_Reset(ctx->req->ws, ctx->req->ws_req);
+	} else if (hp == ctx->http_bereq) {
+		CHECK_OBJ_NOTNULL(ctx->bo, BUSYOBJ_MAGIC);
+		HTTP_Copy(ctx->bo->bereq, ctx->bo->bereq0);
+		WS_Reset(ctx->bo->bereq->ws, ctx->bo->ws_bo);
+		WS_Reset(ctx->bo->ws, ctx->bo->ws_bo);
+	} else
+		WRONG("VRT_Rollback 'hp' invalid");
 }
 
 /*--------------------------------------------------------------------*/
diff --git a/bin/varnishtest/tests/m00017.vtc b/bin/varnishtest/tests/m00017.vtc
new file mode 100644
index 0000000..5622ec0
--- /dev/null
+++ b/bin/varnishtest/tests/m00017.vtc
@@ -0,0 +1,69 @@
+varnishtest "Test std.rollback"
+
+server s1 {
+	rxreq
+	expect req.url == "/foo"
+	expect req.http.foobar == "bar"
+	txresp -status 400
+	accept
+	rxreq
+	expect req.url == "/bar"
+	expect req.http.foobar == "foo"
+	txresp
+	accept
+	rxreq
+	expect req.url == "/baz"
+	expect req.http.foobar == "qux"
+	txresp -status 400
+	accept
+	rxreq
+	expect req.url == "/qux"
+	expect req.http.foobar == "baz"
+	txresp
+} -start
+
+varnish v1 -vcl+backend {
+	import ${vmod_std};
+
+	sub vcl_recv {
+		if (req.url == "/foo") {
+			set req.http.foobar = "bar";
+		}
+	}
+
+	sub vcl_deliver {
+		if (resp.status == 400) {
+			std.rollback(req);
+			set req.url = "/bar";
+			return (restart);
+		}
+	}
+} -start
+
+client c1 {
+	txreq -url "/foo" -hdr "foobar: foo"
+	rxresp
+} -run
+
+varnish v1 -vcl+backend {
+	import ${vmod_std};
+
+	sub vcl_backend_fetch {
+		if (bereq.url == "/baz") {
+			set bereq.http.foobar = "qux";
+		}
+	}
+
+	sub vcl_backend_response {
+		if (beresp.status == 400) {
+			std.rollback(bereq);
+			set bereq.url = "/qux";
+			return (retry);
+		}
+	}
+}
+
+client c1 {
+	txreq -url "/baz" -hdr "foobar: baz"
+	rxresp
+} -run
diff --git a/bin/varnishtest/tests/r01512.vtc b/bin/varnishtest/tests/r01512.vtc
new file mode 100644
index 0000000..71496a2
--- /dev/null
+++ b/bin/varnishtest/tests/r01512.vtc
@@ -0,0 +1,56 @@
+varnishtest "Regression test for #1512"
+
+# First test bereq changes across v_b_r and v_b_f
+
+server s1 {
+	rxreq
+	txresp -status 700
+} -start
+
+varnish v1 -vcl+backend {
+	sub vcl_backend_fetch {
+		if (bereq.http.x-abandon == "1") {
+			return (abandon);
+		}
+	}
+	sub vcl_backend_response {
+		if (beresp.status == 700) {
+			set bereq.http.x-abandon = "1";
+			return (retry);
+		}
+	}
+	sub vcl_synth {
+		set resp.status = 701;
+	}
+} -start
+
+client c1 {
+	txreq
+	rxresp
+	expect resp.status == 701
+} -run
+
+# Then across v_b_e and v_b_f
+
+varnish v1 -vcl {
+	backend bad { .host = "${bad_ip}"; }
+	sub vcl_backend_fetch {
+		set bereq.backend = bad;
+		if (bereq.http.x-abandon == "2") {
+			return (abandon);
+		}
+	}
+	sub vcl_backend_error {
+		set bereq.http.x-abandon = "2";
+		return (retry);
+	}
+	sub vcl_synth {
+		set resp.status = 702;
+	}
+}
+
+client c1 {
+	txreq
+	rxresp
+	expect resp.status == 702
+} -run
diff --git a/doc/sphinx/reference/vcl.rst b/doc/sphinx/reference/vcl.rst
index 743689e..6828f99 100644
--- a/doc/sphinx/reference/vcl.rst
+++ b/doc/sphinx/reference/vcl.rst
@@ -369,7 +369,8 @@ return()
   in the request handling state machine.
 
 rollback()
-  Restore request HTTP headers to their original state.
+  Restore *req* HTTP headers to their original state. This function is
+  deprecated.  Use std.rollback() instead.
 
 synthetic(STRING)
   Prepare a synthetic response body containing the STRING. Available in
diff --git a/include/vrt.h b/include/vrt.h
index efc9c94..ead55d3 100644
--- a/include/vrt.h
+++ b/include/vrt.h
@@ -249,7 +249,7 @@ void VRT_hashdata(const struct vrt_ctx *, const char *str, ...);
 int VRT_strcmp(const char *s1, const char *s2);
 void VRT_memmove(void *dst, const void *src, unsigned len);
 
-void VRT_Rollback(const struct vrt_ctx *);
+void VRT_Rollback(const struct vrt_ctx *, const struct http *);
 
 /* Synthetic pages */
 void VRT_synth_page(const struct vrt_ctx *, const char *, ...);
diff --git a/lib/libvcc/vcc_action.c b/lib/libvcc/vcc_action.c
index 9b1d22d..1b0c4b2 100644
--- a/lib/libvcc/vcc_action.c
+++ b/lib/libvcc/vcc_action.c
@@ -376,7 +376,7 @@ parse_rollback(struct vcc *tl)
 {
 
 	vcc_NextToken(tl);
-	Fb(tl, 1, "VRT_Rollback(ctx);\n");
+	Fb(tl, 1, "VRT_Rollback(ctx, VRT_r_req(ctx));\n");
 }
 
 /*--------------------------------------------------------------------*/
diff --git a/lib/libvmod_std/vmod.vcc b/lib/libvmod_std/vmod.vcc
index fbee007..5bcc403 100644
--- a/lib/libvmod_std/vmod.vcc
+++ b/lib/libvmod_std/vmod.vcc
@@ -174,6 +174,13 @@ $Function INT port(IP)
 Description
 	Returns the port number of an IP address.
 
+$Function VOID rollback(HTTP)
+
+Description
+	Restore *h* HTTP headers to their original state.
+Example
+	std.rollback(bereq);
+
 $Function VOID timestamp(STRING)
 
 Description
diff --git a/lib/libvmod_std/vmod_std.c b/lib/libvmod_std/vmod_std.c
index 718fb71..c5d0cfe 100644
--- a/lib/libvmod_std/vmod_std.c
+++ b/lib/libvmod_std/vmod_std.c
@@ -205,6 +205,12 @@ vmod_port(const struct vrt_ctx *ctx, VCL_IP ip)
 	return (VSA_Port(ip));
 }
 
+VCL_VOID __match_proto__(td_std_rollback)
+vmod_rollback(const struct vrt_ctx *ctx, VCL_HTTP hp)
+{
+	VRT_Rollback(ctx, hp);
+}
+
 VCL_VOID __match_proto__(td_std_timestamp)
 vmod_timestamp(const struct vrt_ctx *ctx, VCL_STRING label)
 {



More information about the varnish-commit mailing list