Be much more cautious about how much workspace we have to build

predictive vary string.

Fixes #1120

bin/varnishd/cache/cache.h

@@ -956,6 +956,7 @@ void RES_WriteObj(struct sess *sp);
 struct vsb *VRY_Create(struct req *sp, const struct http *hp);
 int VRY_Match(struct req *, const uint8_t *vary);
 void VRY_Validate(const uint8_t *vary);
+void VRY_Prep(struct req *);
 
 /* cache_vcl.c */
 void VCL_Init(void);

bin/varnishd/cache/cache_center.c

@@ -1081,18 +1081,7 @@ cnt_lookup(struct sess *sp, struct worker *wrk, struct req *req)
 	CHECK_OBJ_NOTNULL(req->vcl, VCL_CONF_MAGIC);
 	AZ(req->busyobj);
 
-	if (req->hash_objhead == NULL) {
-		/* Not a waiting list return */
-		AZ(req->vary_b);
-		AZ(req->vary_l);
-		AZ(req->vary_e);
-		(void)WS_Reserve(req->ws, 0);
-	} else {
-		AN(req->ws->r);
-	}
-	req->vary_b = (void*)req->ws->f;
-	req->vary_e = (void*)req->ws->r;
-	req->vary_b[2] = '\0';
+	VRY_Prep(req);
 
 	AZ(req->objcore);
 	oc = HSH_Lookup(sp);
@@ -1359,7 +1348,7 @@ cnt_restart(struct sess *sp, const struct worker *wrk, struct req *req)
 	CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
 	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
 	CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
-	
+
 	req->director = NULL;
 	if (++req->restarts >= cache_param->max_restarts) {
 		req->err_code = 503;

bin/varnishd/cache/cache_vary.c

@@ -176,22 +176,61 @@ vry_cmp(const uint8_t *v1, const uint8_t *v2)
 	return (retval);
 }
 
+/**********************************************************************
+ * Prepare predictive vary string
+ */
+
+void
+VRY_Prep(struct req *req)
+{
+	if (req->hash_objhead == NULL) {
+		/* Not a waiting list return */
+		AZ(req->vary_b);
+		AZ(req->vary_l);
+		AZ(req->vary_e);
+		(void)WS_Reserve(req->ws, 0);
+	} else {
+		AN(req->ws->r);
+	}
+	req->vary_b = (void*)req->ws->f;
+	req->vary_e = (void*)req->ws->r;
+	if (req->vary_b + 2 < req->vary_e)
+		req->vary_b[2] = '\0';
+}
+
+/**********************************************************************
+ * Match vary strings, and build a new cached string if possible.
+ *
+ * Return zero if there is certainly no match.
+ * Return non-zero if there could be a match or if we couldn't tell.
+ */
+
 int
 VRY_Match(struct req *req, const uint8_t *vary)
 {
 	uint8_t *vsp = req->vary_b;
 	char *h, *e;
 	unsigned lh, ln;
-	int i, retval = 1, oflo = 0;
+	int i, oflo = 0;
 
 	AN(vsp);
 	while (vary[2]) {
+		if (vsp + 2 >= req->vary_e) {
+			/*
+			 * Too little workspace to find out
+			 */
+			oflo = 1;
+			break;
+		}
 		i = vry_cmp(vary, vsp);
 		if (i == 1) {
-			/* Build a new entry */
+			/*
+			 * Different header, build a new entry,
+			 * then compare again with that new entry.
+			 */
 
-			i = http_GetHdr(req->http,
-			    (const char*)(vary+2), &h);
+			ln = 2 + vary[2] + 2;
+			i = http_GetHdr(req->http, (const char*)(vary+2), &h);
 			if (i) {
 				/* Trim trailing space */
 				e = strchr(h, '\0');
@@ -199,55 +238,55 @@ VRY_Match(struct req *req, const uint8_t *vary)
 					e--;
 				lh = e - h;
 				assert(lh < 0xffff);
+				ln += lh;
 			} else {
 				e = h = NULL;
 				lh = 0xffff;
 			}
 
-			/* Length of the entire new vary entry */
-			ln = 2 + vary[2] + 2 + (lh == 0xffff ? 0 : lh);
-			if (vsp + ln >= req->vary_e) {
-				vsp = req->vary_b;
+			if (vsp + ln + 2 >= req->vary_e) {
+				/*
+				 * Not enough space to build new entry
+				 * and put terminator behind it.
+				 */
 				oflo = 1;
+				break;
 			}
 
-			/*
-			 * We MUST have space for one entry and the end marker
-			 * after it, which prevents old junk from confusing us
-			 */
-			assert(vsp + ln + 2 < req->vary_e);
-
 			vbe16enc(vsp, (uint16_t)lh);
 			memcpy(vsp + 2, vary + 2, vary[2] + 2);
-			if (h != NULL && e != NULL) {
-				memcpy(vsp + 2 + vsp[2] + 2, h, e - h);
-				vsp[2 + vary[2] + 2 + (e - h) + 2] = '\0';
-			} else
-				vsp[2 + vary[2] + 2 + 2] = '\0';
+			if (h != NULL)
+				memcpy(vsp + 2 + vsp[2] + 2, h, lh);
+			vsp[ln + 0] = 0xff;
+			vsp[ln + 1] = 0xff;
+			vsp[ln + 2] = 0;
+			VRY_Validate(vsp);
+			req->vary_l = vsp + 3;
 
 			i = vry_cmp(vary, vsp);
-			assert(i != 1);	/* hdr must be the same now */
+			assert(i == 0 || i == 2);
+		}
+		if (i == 0) {
+			/* Same header, same contents*/
+			vsp += vry_len(vsp);
+			vary += vry_len(vary);
+		} else if (i == 2) {
+			/* Same header, different contents, cannot match */
+			return (0);
 		}
-		if (i != 0)
-			retval = 0;
-		vsp += vry_len(vsp);
-		vary += vry_len(vary);
 	}
-	if (vsp + 3 > req->vary_e)
-		oflo = 1;
-
 	if (oflo) {
-		/* XXX: Should log this */
 		vsp = req->vary_b;
-	}
-	vsp[0] = 0xff;
-	vsp[1] = 0xff;
-	vsp[2] = 0;
-	if (oflo)
 		req->vary_l = NULL;
-	else
-		req->vary_l = vsp + 3;
-	return (retval);
+		if (vsp + 2 < req->vary_e) {
+			vsp[0] = 0xff;
+			vsp[1] = 0xff;
+			vsp[2] = 0;
+		}
+		return (0);
+	} else {
+		return (1);
+	}
 }
 
 void

bin/varnishtest/tests/r01120.vtc

+varnishtest "insanely long vary string"
+
+server s1 {
+	rxreq
+	txresp -hdr "Vary: Foo" -body "xxxx"
+	rxreq
+	txresp -hdr "Vary: Foo" -body "yyyyy"
+} -start
+
+varnish v1 \
+	-cliok "param.set workspace_client 8k" \
+	-cliok "param.set workspace_backend 200k" \
+	-vcl+backend {
+} -start
+
+client c1 {
+	txreq -hdr "Foo: blabla"
+	rxresp
+	expect resp.bodylen == 4
+	#txreq -hdr "Foo: blablaA"
+	txreq -hdr "Foo: blablaAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAaaaaaaaaaaaaaaaaAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+	rxresp
+	expect resp.bodylen == 5
+} -run