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

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;						\

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

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