[master] 9867631 Prevent storage backends name collisions

[Dridi Boukelmoune]

commit 98676319878b7a6aca77be7e207e140202800ae1
Author: Dridi Boukelmoune <dridi.boukelmoune at gmail.com>
Date:   Tue May 2 23:57:15 2017 +0200

    Prevent storage backends name collisions
    
    Because of Transient storage being a special-case stevedore, the
    STV_Foreach logic would only work after the child's startup once
    all storage backends are defined. So during the setup, collisions
    would only be detected for the Transient storage.
    
    The underlying STV__iter function is now pretty much like a plain
    VTAILQ_FOREACH loop with an additional CHECK_OBJ_ORNULL step on all
    iterations.
    
    As a result, the transient storage is appended to the list at the
    end of the manager's setup and is now reported last in the CLI's
    storage.list command.
    
    Error messages related to the -s option are slightly more helpful.
    
    Fixes #2321

diff --git a/bin/varnishd/storage/mgt_stevedore.c b/bin/varnishd/storage/mgt_stevedore.c
index 304b3bd..1ddf8a5 100644
--- a/bin/varnishd/storage/mgt_stevedore.c
+++ b/bin/varnishd/storage/mgt_stevedore.c
@@ -57,17 +57,11 @@ STV__iter(struct stevedore ** const pp)
 
 	AN(pp);
 	CHECK_OBJ_ORNULL(*pp, STEVEDORE_MAGIC);
-	if (*pp == stv_transient) {
-		*pp = NULL;
-		return (0);
-	}
 	if (*pp != NULL)
 		*pp = VTAILQ_NEXT(*pp, list);
 	else
 		*pp = VTAILQ_FIRST(&stevedores);
-	if (*pp == NULL)
-		*pp = stv_transient;
-	return (1);
+	return (*pp != NULL);
 }
 
 /*--------------------------------------------------------------------*/
@@ -128,6 +122,26 @@ static const struct choice STV_choice[] = {
 	{ NULL,		NULL }
 };
 
+static void
+stv_check_ident(const char *spec, const char *ident)
+{
+	struct stevedore *stv;
+	unsigned found = 0;
+
+	if (!strcmp(ident, TRANSIENT_STORAGE))
+		found = (stv_transient != NULL);
+	else {
+		STV_Foreach(stv)
+			if (!strcmp(stv->ident, ident)) {
+				found = 1;
+				break;
+			}
+	}
+
+	if (found)
+		ARGV_ERR("(-s %s) '%s' is already defined\n", spec, ident);
+}
+
 void
 STV_Config(const char *spec)
 {
@@ -135,7 +149,6 @@ STV_Config(const char *spec)
 	const char *p, *q;
 	struct stevedore *stv;
 	const struct stevedore *stv2;
-	struct stevedore *stv3;
 	int ac, l;
 	static unsigned seq = 0;
 
@@ -183,15 +196,12 @@ STV_Config(const char *spec)
 		bprintf(stv->ident, "%.*s", l, spec);
 	}
 
-	STV_Foreach(stv3)
-		if (!strcmp(stv3->ident, stv->ident))
-			ARGV_ERR("(-s%s=%s) already defined once\n",
-			    stv->ident, stv->name);
+	stv_check_ident(spec, stv->ident);
 
 	if (stv->init != NULL)
 		stv->init(stv, ac, av);
 	else if (ac != 0)
-		ARGV_ERR("(-s%s) too many arguments\n", stv->name);
+		ARGV_ERR("(-s %s) too many arguments\n", stv->name);
 
 	AN(stv->allocobj);
 	AN(stv->methods);
@@ -199,9 +209,8 @@ STV_Config(const char *spec)
 	if (!strcmp(stv->ident, TRANSIENT_STORAGE)) {
 		AZ(stv_transient);
 		stv_transient = stv;
-	} else {
+	} else
 		VTAILQ_INSERT_TAIL(&stevedores, stv, list);
-	}
 	/* NB: Do not free av, stevedore gets to keep it */
 }
 
@@ -216,4 +225,5 @@ STV_Config_Transient(void)
 	VCLS_AddFunc(mgt_cls, MCF_AUTH, cli_stv);
 	if (stv_transient == NULL)
 		STV_Config(TRANSIENT_STORAGE "=malloc");
+	VTAILQ_INSERT_TAIL(&stevedores, stv_transient, list);
 }
diff --git a/bin/varnishtest/tests/r02321.vtc b/bin/varnishtest/tests/r02321.vtc
new file mode 100644
index 0000000..33c7658
--- /dev/null
+++ b/bin/varnishtest/tests/r02321.vtc
@@ -0,0 +1,19 @@
+varnishtest "Storage name collisions"
+
+# intentional collision
+shell -err -expect "Error: (-s main=malloc,100m) 'main' is already defined" {
+	exec varnishd -a :0 -f '' -n ${tmpdir} \
+		-s main=malloc,10m -s main=malloc,100m
+}
+
+# pseudo-accidental collision
+shell -err -expect "Error: (-s s0=malloc,100m) 's0' is already defined" {
+	exec varnishd -a :0 -f '' -n ${tmpdir} \
+		-s malloc,10m -s s0=malloc,100m
+}
+
+# Transient collision
+shell -err -expect "Error: (-s Transient=malloc,100m) 'Transient' is already defined" {
+	exec varnishd -a :0 -f '' -n ${tmpdir} \
+		-s Transient=malloc,10m -s Transient=malloc,100m
+}