[master] d0db4c975 Avoid panic with PRIV_TOP arguments

[Nils Goroll]

commit d0db4c975f281c8d096b48b6fd6014cdfc606ab5
Author: Nils Goroll <nils.goroll at uplex.de>
Date:   Mon Jan 18 16:34:48 2021 +0100

    Avoid panic with PRIV_TOP arguments
    
    Instead of triggering a WRONG() panic, we now fail the VCL when any vmod
    function with PRIV_TOP argument(s) is present in a sub called from
    outside client context.
    
    Note that the VCL failure happens not when the vmod function with the
    PRIV_TOP argument is called, but rather when the containing SUB is
    called. This is because we prepare PRIV_TOP arguments in the function
    preamble.
    
    Fixes #3498

diff --git a/bin/varnishd/cache/cache_vrt_priv.c b/bin/varnishd/cache/cache_vrt_priv.c
index d6884c228..fcdece918 100644
--- a/bin/varnishd/cache/cache_vrt_priv.c
+++ b/bin/varnishd/cache/cache_vrt_priv.c
@@ -199,13 +199,20 @@ VRT_priv_task(VRT_CTX, const void *vmod_id)
 	    (uintptr_t)vmod_id));
 }
 
+/*
+ * XXX #3498 on VRT_fail(): Would be better to move the PRIV_TOP check to VCC
+ *
+ * This will fail in the preamble of any VCL SUB containing a call to a vmod
+ * function with a PRIV_TOP argument, which might not exactly be pola
+ */
+
 #define VRT_PRIV_TOP_PREP(ctx, req, sp, top)	do {			\
 		CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);			\
 		req = (ctx)->req;					\
 		if (req == NULL) {					\
-			WRONG("PRIV_TOP is only accessible "		\
+			VRT_fail(ctx, "PRIV_TOP is only accessible "	\
 			    "in client VCL context");			\
-			NEEDLESS(return (NULL));			\
+			return (NULL);					\
 		}							\
 		CHECK_OBJ(req, REQ_MAGIC);				\
 		sp = (ctx)->sp;						\
diff --git a/bin/varnishtest/tests/v00043.vtc b/bin/varnishtest/tests/v00043.vtc
index 828b06668..95b85770b 100644
--- a/bin/varnishtest/tests/v00043.vtc
+++ b/bin/varnishtest/tests/v00043.vtc
@@ -64,6 +64,8 @@ varnish v1 -cliok "param.set debug +syncvsl" -vcl+backend {
 	}
 
 	sub vcl_recv {
+		set req.http.x0 = debug.test_priv_top(req.url + req.esi_level);
+		# coverage
 		set req.http.x0 = debug.test_priv_top(req.url + req.esi_level);
 		if (req.url == "/foo1") {
 			o.test_priv_top(req.url + req.esi_level);
@@ -79,9 +81,28 @@ varnish v1 -cliok "param.set debug +syncvsl" -vcl+backend {
 		set req.http.o2 = o2.test_priv_top("");
 	}
 
+	# XXX because PRIV_TOP arguments get initialized in the
+	# function preamble, the mere presence of a vmod call with a
+	# PRIV_TOP argument in a SUB will trigger the failure if that
+	# sub is called at all.
+	#
+	# So to test #3498, we need to fence test_priv_top into its
+	# own sub
+	sub callingmewill503 {
+		debug.test_priv_top("only works on client side");
+	}
+
+	sub vcl_backend_fetch {
+		if (bereq.url == "/fail") {
+			call callingmewill503;
+		}
+		if (bereq.url == "/failo") {
+			o2.test_priv_top("only works on client side");
+		}
+	}
+
 	sub vcl_backend_response {
 		set beresp.do_esi = true;
-
 	}
 
 	sub vcl_deliver {
@@ -101,3 +122,18 @@ client c1 {
 } -run
 
 varnish v1 -expect client_req == 2
+
+client c1 {
+	txreq -url /fail
+	rxresp
+	expect resp.status == 503
+} -start
+
+client c2 {
+	txreq -url /failo
+	rxresp
+	expect resp.status == 503
+} -start
+
+client c1 -wait
+client c2 -wait
diff --git a/doc/sphinx/reference/vmod.rst b/doc/sphinx/reference/vmod.rst
index 48e99a28a..9c3e08317 100644
--- a/doc/sphinx/reference/vmod.rst
+++ b/doc/sphinx/reference/vmod.rst
@@ -496,11 +496,11 @@ The VCL compiler supports the following private pointers:
 * ``PRIV_TOP`` "per top-request" private pointers live for the
   duration of one request and all its ESI-includes. They are only
   defined for the client side. When used from backend VCL subs, a NULL
-  pointer will be passed.
+  pointer will potentially be passed and a VCL failure triggered.
   These private pointers live only for the duration of their top
   level request
 
-  .. XXX BROKEN https://github.com/varnishcache/varnish-cache/issues/3498
+  .. PRIV_TOP see #3498
 
 * ``PRIV_VCL`` "per vcl" private pointers are useful for such global
   state that applies to all calls in this VCL, for instance flags that