From 68ac5a6690f37393771f4ecf923b00286aa95a0e Mon Sep 17 00:00:00 2001
From: Poul-Henning Kamp <phk@FreeBSD.org>
Date: Sat, 18 Jun 2016 17:15:59 +0000
Subject: [PATCH] Some time ago I decided that it was more convenient if it
 were actually possible to locate the compiled VCL shared library based on the
 loaded VCL name, and removed the part of the subdirectory name which made it
 unique.

Bad idea.

Add a comment to make sure I don't get that Idea again.

Fixes #1933
---
 bin/varnishd/cache/cache_vcl.c |  5 +++++
 bin/varnishd/mgt/mgt_vcc.c     | 35 +++++++++++++++++++++++++++++++++-
 bin/varnishd/mgt/mgt_vcl.c     | 25 ++++++++++++++++--------
 3 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index 055c84107c..41ae74743d 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -299,6 +299,11 @@ VCL_Open(const char *fn, struct vsb *msg)
 	AN(fn);
 	AN(msg);
 
+#ifdef RTLD_NOLOAD
+	/* Detect bogus caching by dlopen(3) */
+	dlh = dlopen(fn, RTLD_NOW | RTLD_NOLOAD);
+	AZ(dlh);
+#endif
 	dlh = dlopen(fn, RTLD_NOW | RTLD_LOCAL);
 	if (dlh == NULL) {
 		VSB_printf(msg, "Could not load compiled VCL.\n");
diff --git a/bin/varnishd/mgt/mgt_vcc.c b/bin/varnishd/mgt/mgt_vcc.c
index d7f3565b14..ca8ae94798 100644
--- a/bin/varnishd/mgt/mgt_vcc.c
+++ b/bin/varnishd/mgt/mgt_vcc.c
@@ -47,6 +47,7 @@
 #include "vcli_serve.h"
 #include "vfil.h"
 #include "vsub.h"
+#include "vtim.h"
 
 struct vcc_priv {
 	unsigned	magic;
@@ -272,7 +273,39 @@ mgt_VccCompile(struct cli *cli, const char *vclname, const char *vclsrc,
 	vp.vclsrc = vclsrc;
 	vp.vclsrcfile = vclsrcfile;
 
-	VSB_printf(sb, "vcl_%s", vclname);
+	/*
+	 * The subdirectory must have a unique name to 100% certain evade
+	 * the refcounting semantics of dlopen(3).
+	 *
+	 * Bad implementations of dlopen(3) think the shlib you are opening
+	 * is the same, if the filename is the same as one already opened.
+	 *
+	 * Sensible implementations do a stat(2) and requires st_ino and
+	 * st_dev to also match.
+	 *
+	 * A correct implementation would run on filesystems which tickle
+	 * st_gen, and also insist that be the identical, before declaring
+	 * a match.
+	 *
+	 * Since no correct implementations are known to exist, we are subject
+	 * to really interesting races if you do something like:
+	 *
+	 *	(running on 'boot' vcl)
+	 *	vcl.load foo /foo.vcl
+	 *	vcl.use foo
+	 *	few/slow requests
+	 *	vcl.use boot
+	 *	vcl.discard foo
+	 *	vcl.load foo /foo.vcl	// dlopen(3) says "same-same"
+	 *	vcl.use foo
+	 *
+	 * Because discard of the first 'foo' lingers on non-zero reference
+	 * count, and when it finally runs, it trashes the second 'foo' because
+	 * dlopen(3) decided they were really the same thing.
+	 *
+	 * The Best way to reproduce ths is to have regexps in the VCL.
+	 */
+	VSB_printf(sb, "vcl_%s.%.9f", vclname, VTIM_real());
 	AZ(VSB_finish(sb));
 	vp.dir = strdup(VSB_data(sb));
 	AN(vp.dir);
diff --git a/bin/varnishd/mgt/mgt_vcl.c b/bin/varnishd/mgt/mgt_vcl.c
index d9cd0438f7..d0843537c5 100644
--- a/bin/varnishd/mgt/mgt_vcl.c
+++ b/bin/varnishd/mgt/mgt_vcl.c
@@ -92,16 +92,25 @@ mgt_vcl_add(const char *name, const char *libfile, const char *state)
 static void
 mgt_vcl_del(struct vclprog *vp)
 {
-	char dn[256];
+	char *p;
 
 	VTAILQ_REMOVE(&vclhead, vp, list);
-	if (strcmp(vp->state, VCL_STATE_LABEL))
-		XXXAZ(unlink(vp->fname));
-	bprintf(dn, "vcl_%s", vp->name);
-	VJ_master(JAIL_MASTER_FILE);
-	(void)rmdir(dn);		// compiler droppings, eg gcov
-	VJ_master(JAIL_MASTER_LOW);
-	free(vp->fname);
+	if (strcmp(vp->state, VCL_STATE_LABEL)) {
+		AZ(unlink(vp->fname));
+		p = strrchr(vp->fname, '/');
+		AN(p);
+		*p = '\0';
+		VJ_master(JAIL_MASTER_FILE);
+		/*
+		 * This will fail if any files are dropped next to the library
+		 * without us knowing.  This happens for instance with GCOV.
+		 * Assume developers know how to clean up after themselves
+		 * (or alternatively:  How to run out of disk space).
+		 */
+		(void)rmdir(vp->fname);
+		VJ_master(JAIL_MASTER_LOW);
+		free(vp->fname);
+	}
 	free(vp->name);
 	free(vp);
 }