[master] 2f8e02a3d Check VCL_HEADER vmod arguments

[Nils Goroll]

commit 2f8e02a3d52d47864ba8bd453f7b7182c0e20472
Author: Nils Goroll <nils.goroll at uplex.de>
Date:   Wed Jun 2 18:23:59 2021 +0200

    Check VCL_HEADER vmod arguments
    
    When a VCL_HEADER argument is a vmod return value, it could be
    
    - NULL for an error condition
    - from a different context
    
    Also, we should not assert on HDR_OBJ in VRT_selecthttp() to
    facilitate error handling in vmods.
    
    Fixes #3623

diff --git a/bin/varnishd/cache/cache_vrt.c b/bin/varnishd/cache/cache_vrt.c
index d6f982b1d..1289c1ac2 100644
--- a/bin/varnishd/cache/cache_vrt.c
+++ b/bin/varnishd/cache/cache_vrt.c
@@ -257,6 +257,9 @@ VRT_selecthttp(VRT_CTX, enum gethdr_e where)
 	case HDR_RESP:
 		hp = ctx->http_resp;
 		break;
+	case HDR_OBJ:
+		hp = NULL;
+		break;
 	default:
 		WRONG("VRT_selecthttp 'where' invalid");
 	}
diff --git a/bin/varnishtest/tests/m00006.vtc b/bin/varnishtest/tests/m00006.vtc
index 24ad5f934..1004277c5 100644
--- a/bin/varnishtest/tests/m00006.vtc
+++ b/bin/varnishtest/tests/m00006.vtc
@@ -13,6 +13,15 @@ varnish v1 -vcl+backend {
 	sub vcl_recv {
 		std.collect(req.http.foo);
 	}
+	sub vcl_hash {
+		hash_data("/");
+		return (lookup);
+	}
+	sub vcl_hit {
+		if (req.url == "/obj") {
+			std.collect(obj.http.foo);
+		}
+	}
 	sub vcl_backend_fetch {
 		std.collect(bereq.http.baz);
 	}
@@ -29,4 +38,7 @@ client c1 {
 	rxresp
 	expect resp.http.bar == "a, b"
 	expect resp.http.qux == "c; d"
+	txreq -url "/obj"
+	rxresp
+	expect resp.status == 503
 } -run
diff --git a/vmod/vmod_debug.c b/vmod/vmod_debug.c
index fa75dd130..f75016da0 100644
--- a/vmod/vmod_debug.c
+++ b/vmod/vmod_debug.c
@@ -789,26 +789,34 @@ xyzzy_collect(VRT_CTX, VCL_STRANDS s)
 
 /* cf. VRT_SetHdr() */
 VCL_VOID
-xyzzy_sethdr(VRT_CTX, VCL_HEADER hs, VCL_STRANDS s)
+xyzzy_sethdr(VRT_CTX, VCL_HEADER hdr, VCL_STRANDS s)
 {
 	struct http *hp;
 	const char *b;
 
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
-	AN(hs);
-	AN(hs->what);
-	hp = VRT_selecthttp(ctx, hs->where);
+	if (hdr == NULL) {
+		VRT_fail(ctx, "debug.sethdr(): header argument is NULL");
+		return;
+	}
+	hp = VRT_selecthttp(ctx, hdr->where);
+	if (hp == NULL) {
+		VRT_fail(ctx, "debug.sethdr(): header argument "
+		    "can not be used here");
+		return;
+	}
+	AN(hdr->what);
 	CHECK_OBJ_NOTNULL(hp, HTTP_MAGIC);
 	if (s->n == 0) {
-		http_Unset(hp, hs->what);
+		http_Unset(hp, hdr->what);
 	} else {
-		b = VRT_StrandsWS(hp->ws, hs->what + 1, s);
+		b = VRT_StrandsWS(hp->ws, hdr->what + 1, s);
 		if (b == NULL) {
-			VSLb(ctx->vsl, SLT_LostHeader, "%s", hs->what + 1);
+			VSLb(ctx->vsl, SLT_LostHeader, "%s", hdr->what + 1);
 		} else {
 			if (*b != '\0')
 				WS_Assert_Allocated(hp->ws, b, strlen(b) + 1);
-			http_Unset(hp, hs->what);
+			http_Unset(hp, hdr->what);
 			http_SetHeader(hp, b);
 		}
 	}
diff --git a/vmod/vmod_std.c b/vmod/vmod_std.c
index bb24e04ce..719c5051a 100644
--- a/vmod/vmod_std.c
+++ b/vmod/vmod_std.c
@@ -182,7 +182,16 @@ vmod_collect(VRT_CTX, VCL_HEADER hdr, VCL_STRING sep)
 	struct http *hp;
 
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+	if (hdr == NULL) {
+		VRT_fail(ctx, "std.collect(): header argument is NULL");
+		return;
+	}
 	hp = VRT_selecthttp(ctx, hdr->where);
+	if (hp == NULL) {
+		VRT_fail(ctx, "std.collect(): header argument "
+		    "can not be used here");
+		return;
+	}
 	http_CollectHdrSep(hp, hdr->what, sep);
 }
 
diff --git a/vmod/vmod_std.vcc b/vmod/vmod_std.vcc
index e9812ee8e..fd0bf1a4a 100644
--- a/vmod/vmod_std.vcc
+++ b/vmod/vmod_std.vcc
@@ -73,6 +73,8 @@ headers, with an additional whitespace for pretty printing.
 Care should be taken when collapsing headers. In particular collapsing
 ``Set-Cookie`` will lead to unexpected results on the browser side.
 
+Using *hdr* from ``obj.http`` triggers a VCL failure.
+
 Examples::
 
 	std.collect(req.http.accept);