rate_ctr: Enforce counter (and ctr_group) names are valid identifiers

As rate counters are automatically exposed on the CTRL interface,
we need to make sure they don't contain special characters such as '.'
which are not permitted/supported by CTRL.

In order to be able to run old versions of osmocom programs with
libosmocore versions after this commit, we introduce some special
name mangling:  Any '.' in the names are replaced with ':' during
counter group registration, if valid identifiers can be obtained
this way.

Change-Id: Ifc6ac824f5dae9a848bb4a5d067c64a69eb40b56

4 files changed, 161 insertions, 45 deletions

diff --git a/include/osmocom/core/rate_ctr.h b/include/osmocom/core/rate_ctr.h
index 74414e98..6ce2dfed 100644
--- a/include/osmocom/core/rate_ctr.h
+++ b/include/osmocom/core/rate_ctr.h
@@ -48,7 +48,7 @@ struct rate_ctr_group_desc {
 	/*! The class to which this group belongs */
 	int class_id;
 	/*! The number of counters in this group */
-	const unsigned int num_ctr;
+	unsigned int num_ctr;
 	/*! Pointer to array of counter names */
 	const struct rate_ctr_desc *ctr_desc;
 };
diff --git a/src/rate_ctr.c b/src/rate_ctr.c
index a04a7760..6de59a02 100644
--- a/src/rate_ctr.c
+++ b/src/rate_ctr.c
@@ -1,4 +1,4 @@
-/* (C) 2009-2010 by Harald Welte <laforge@gnumonks.org>
+/* (C) 2009-2017 by Harald Welte <laforge@gnumonks.org>
  *
  * All Rights Reserved
  *
@@ -55,6 +55,7 @@
  *
  * \file rate_ctr.c */
 
+#include <stdbool.h>
 #include <stdint.h>
 #include <string.h>
 
@@ -63,11 +64,117 @@
 #include <osmocom/core/talloc.h>
 #include <osmocom/core/timer.h>
 #include <osmocom/core/rate_ctr.h>
+#include <osmocom/core/logging.h>
 
 static LLIST_HEAD(rate_ctr_groups);
 
 static void *tall_rate_ctr_ctx;
 
+
+static bool rate_ctrl_group_desc_validate(const struct rate_ctr_group_desc *desc, bool quiet)
+{
+	unsigned int i;
+	const struct rate_ctr_desc *ctr_desc = desc->ctr_desc;
+
+	if (!desc) {
+		LOGP(DLGLOBAL, LOGL_ERROR, "NULL is not a valid counter group descriptor\n");
+		return false;
+	}
+
+	DEBUGP(DLGLOBAL, "validating counter group %p(%s) with %u counters\n", desc,
+		desc->group_name_prefix, desc->num_ctr);
+
+	if (!osmo_identifier_valid(desc->group_name_prefix)) {
+		if (!quiet)
+			LOGP(DLGLOBAL, LOGL_ERROR, "'%s' is not a valid counter group identifier\n",
+				desc->group_name_prefix);
+		return false;
+	}
+
+	for (i = 0; i < desc->num_ctr; i++) {
+		if (!osmo_identifier_valid(ctr_desc[i].name)) {
+			if (!quiet)
+				LOGP(DLGLOBAL, LOGL_ERROR, "'%s' is not a valid counter identifier\n",
+					ctr_desc[i].name);
+			return false;
+		}
+	}
+
+	return true;
+}
+
+/* return 'in' if it doesn't contaon any '.'; otherwise allocate a copy and
+ * replace all '.' with ':' */
+static char *mangle_identifier_ifneeded(const void *ctx, const char *in)
+{
+	char *out;
+	unsigned int i;
+
+	if (!in)
+		return NULL;
+
+	if (!strchr(in, '.'))
+		return (char *)in;
+
+	out = talloc_strdup(ctx, in);
+	OSMO_ASSERT(out);
+
+	for (i = 0; i < strlen(out); i++) {
+		if (out[i] == '.')
+			out[i] = ':';
+	}
+
+	return out;
+}
+
+/* "mangle" a rate counter group descriptor, i.e. replace any '.' with ':' */
+static struct rate_ctr_group_desc *
+rate_ctr_group_desc_mangle(void *ctx, const struct rate_ctr_group_desc *desc)
+{
+	struct rate_ctr_group_desc *desc_new = talloc_zero(ctx, struct rate_ctr_group_desc);
+	int i;
+
+	OSMO_ASSERT(desc_new);
+
+	/* mangle the name_prefix but copy/keep the rest */
+	desc_new->group_name_prefix = mangle_identifier_ifneeded(desc_new, desc->group_name_prefix);
+	desc_new->group_description = desc->group_description;
+	desc_new->class_id = desc->class_id;
+	desc_new->num_ctr = desc->num_ctr;
+	desc_new->ctr_desc = talloc_array(desc_new, struct rate_ctr_desc, desc_new->num_ctr);
+	OSMO_ASSERT(desc_new->ctr_desc);
+
+	for (i = 0; i < desc->num_ctr; i++) {
+		struct rate_ctr_desc *ctrd_new = (struct rate_ctr_desc *) desc_new->ctr_desc;
+		const struct rate_ctr_desc *ctrd = desc->ctr_desc;
+
+		if (!ctrd[i].name) {
+			LOGP(DLGLOBAL, LOGL_ERROR, "counter group '%s'[%d] == NULL, aborting\n",
+				desc->group_name_prefix, i);
+			goto err_free;
+		}
+
+		ctrd_new[i].name = mangle_identifier_ifneeded(desc_new->ctr_desc, ctrd[i].name);
+		ctrd_new[i].description = ctrd[i].description;
+	}
+
+	if (!rate_ctrl_group_desc_validate(desc_new, false)) {
+		/* simple mangling of identifiers ('.' -> ':') was not sufficient to render a valid
+		 * descriptor, we have to bail out */
+		LOGP(DLGLOBAL, LOGL_ERROR, "counter group '%s' still invalid after mangling\n",
+			desc->group_name_prefix);
+		goto err_free;
+	}
+
+	LOGP(DLGLOBAL, LOGL_INFO, "Needed to mangle ounter group '%s' names still using '.' as "
+		"separator, please consider updating the application\n", desc->group_name_prefix);
+
+	return desc_new;
+err_free:
+	talloc_free(desc_new);
+	return NULL;
+}
+
 /*! Allocate a new group of counters according to description
  *  \param[in] ctx \ref talloc context
  *  \param[in] desc Rate counter group description
@@ -80,6 +187,15 @@ struct rate_ctr_group *rate_ctr_group_alloc(void *ctx,
 	unsigned int size;
 	struct rate_ctr_group *group;
 
+	/* attempt to mangle all '.' in identifiers to ':' for backwards compat */
+	if (!rate_ctrl_group_desc_validate(desc, true)) {
+		/* don't use 'ctx' here as it would screw up memory leak debugging e.g.
+		 * in osmo-msc */
+		desc = rate_ctr_group_desc_mangle(NULL, desc);
+		if (!desc)
+			return NULL;
+	}
+
 	size = sizeof(struct rate_ctr_group) +
 			desc->num_ctr * sizeof(struct rate_ctr);
 
diff --git a/tests/gb/gprs_ns_test.c b/tests/gb/gprs_ns_test.c
index 0bbf9432..fac3c36f 100644
--- a/tests/gb/gprs_ns_test.c
+++ b/tests/gb/gprs_ns_test.c
@@ -261,7 +261,7 @@ static void dump_rate_ctr_group(FILE *stream, const char *prefix,
 
 	for (i = 0; i < ctrg->desc->num_ctr; i++) {
 		struct rate_ctr *ctr = &ctrg->ctr[i];
-		if (ctr->current && !strchr(ctrg->desc->ctr_desc[i].name, '.'))
+		if (ctr->current && !strchr(ctrg->desc->ctr_desc[i].name, ':'))
 			fprintf(stream, " %s%s: %llu%s",
 				prefix, ctrg->desc->ctr_desc[i].description,
 				(long long)ctr->current,
diff --git a/tests/stats/stats_test.ok b/tests/stats/stats_test.ok
index a0c001b9..cb9daf22 100644
--- a/tests/stats/stats_test.ok
+++ b/tests/stats/stats_test.ok
@@ -2,14 +2,14 @@ Start test: test_reporting
   test1: open
   test2: open
 report (initial):
-  test2: counter p= g=ctr-test.one i=2 n=ctr.a v=0 d=0
-  test1: counter p= g=ctr-test.one i=2 n=ctr.a v=0 d=0
-  test2: counter p= g=ctr-test.one i=2 n=ctr.b v=0 d=0
-  test1: counter p= g=ctr-test.one i=2 n=ctr.b v=0 d=0
-  test2: counter p= g=ctr-test.one i=1 n=ctr.a v=0 d=0
-  test1: counter p= g=ctr-test.one i=1 n=ctr.a v=0 d=0
-  test2: counter p= g=ctr-test.one i=1 n=ctr.b v=0 d=0
-  test1: counter p= g=ctr-test.one i=1 n=ctr.b v=0 d=0
+  test2: counter p= g=ctr-test:one i=2 n=ctr:a v=0 d=0
+  test1: counter p= g=ctr-test:one i=2 n=ctr:a v=0 d=0
+  test2: counter p= g=ctr-test:one i=2 n=ctr:b v=0 d=0
+  test1: counter p= g=ctr-test:one i=2 n=ctr:b v=0 d=0
+  test2: counter p= g=ctr-test:one i=1 n=ctr:a v=0 d=0
+  test1: counter p= g=ctr-test:one i=1 n=ctr:a v=0 d=0
+  test2: counter p= g=ctr-test:one i=1 n=ctr:b v=0 d=0
+  test1: counter p= g=ctr-test:one i=1 n=ctr:b v=0 d=0
   test2: item p= g=test.one i=2 n=item.a v=-1 u=ma
   test1: item p= g=test.one i=2 n=item.a v=-1 u=ma
   test2: item p= g=test.one i=2 n=item.b v=-1 u=kb
@@ -19,19 +19,19 @@ report (initial):
   test2: item p= g=test.one i=1 n=item.b v=-1 u=kb
   test1: item p= g=test.one i=1 n=item.b v=-1 u=kb
 report (srep1 global):
-  test2: counter p= g=ctr-test.one i=2 n=ctr.a v=0 d=0
-  test2: counter p= g=ctr-test.one i=2 n=ctr.b v=0 d=0
-  test2: counter p= g=ctr-test.one i=1 n=ctr.a v=0 d=0
-  test2: counter p= g=ctr-test.one i=1 n=ctr.b v=0 d=0
+  test2: counter p= g=ctr-test:one i=2 n=ctr:a v=0 d=0
+  test2: counter p= g=ctr-test:one i=2 n=ctr:b v=0 d=0
+  test2: counter p= g=ctr-test:one i=1 n=ctr:a v=0 d=0
+  test2: counter p= g=ctr-test:one i=1 n=ctr:b v=0 d=0
   test2: item p= g=test.one i=2 n=item.a v=-1 u=ma
   test2: item p= g=test.one i=2 n=item.b v=-1 u=kb
   test2: item p= g=test.one i=1 n=item.a v=-1 u=ma
   test2: item p= g=test.one i=1 n=item.b v=-1 u=kb
 report (srep1 peer):
-  test2: counter p= g=ctr-test.one i=2 n=ctr.a v=0 d=0
-  test2: counter p= g=ctr-test.one i=2 n=ctr.b v=0 d=0
-  test2: counter p= g=ctr-test.one i=1 n=ctr.a v=0 d=0
-  test2: counter p= g=ctr-test.one i=1 n=ctr.b v=0 d=0
+  test2: counter p= g=ctr-test:one i=2 n=ctr:a v=0 d=0
+  test2: counter p= g=ctr-test:one i=2 n=ctr:b v=0 d=0
+  test2: counter p= g=ctr-test:one i=1 n=ctr:a v=0 d=0
+  test2: counter p= g=ctr-test:one i=1 n=ctr:b v=0 d=0
   test2: item p= g=test.one i=2 n=item.a v=-1 u=ma
   test1: item p= g=test.one i=2 n=item.a v=-1 u=ma
   test2: item p= g=test.one i=2 n=item.b v=-1 u=kb
@@ -41,14 +41,14 @@ report (srep1 peer):
   test2: item p= g=test.one i=1 n=item.b v=-1 u=kb
   test1: item p= g=test.one i=1 n=item.b v=-1 u=kb
 report (srep1 subscriber):
-  test2: counter p= g=ctr-test.one i=2 n=ctr.a v=0 d=0
-  test1: counter p= g=ctr-test.one i=2 n=ctr.a v=0 d=0
-  test2: counter p= g=ctr-test.one i=2 n=ctr.b v=0 d=0
-  test1: counter p= g=ctr-test.one i=2 n=ctr.b v=0 d=0
-  test2: counter p= g=ctr-test.one i=1 n=ctr.a v=0 d=0
-  test1: counter p= g=ctr-test.one i=1 n=ctr.a v=0 d=0
-  test2: counter p= g=ctr-test.one i=1 n=ctr.b v=0 d=0
-  test1: counter p= g=ctr-test.one i=1 n=ctr.b v=0 d=0
+  test2: counter p= g=ctr-test:one i=2 n=ctr:a v=0 d=0
+  test1: counter p= g=ctr-test:one i=2 n=ctr:a v=0 d=0
+  test2: counter p= g=ctr-test:one i=2 n=ctr:b v=0 d=0
+  test1: counter p= g=ctr-test:one i=2 n=ctr:b v=0 d=0
+  test2: counter p= g=ctr-test:one i=1 n=ctr:a v=0 d=0
+  test1: counter p= g=ctr-test:one i=1 n=ctr:a v=0 d=0
+  test2: counter p= g=ctr-test:one i=1 n=ctr:b v=0 d=0
+  test1: counter p= g=ctr-test:one i=1 n=ctr:b v=0 d=0
   test2: item p= g=test.one i=2 n=item.a v=-1 u=ma
   test1: item p= g=test.one i=2 n=item.a v=-1 u=ma
   test2: item p= g=test.one i=2 n=item.b v=-1 u=kb
@@ -59,49 +59,49 @@ report (srep1 subscriber):
   test1: item p= g=test.one i=1 n=item.b v=-1 u=kb
 report (srep2 disabled):
   test2: close
-  test1: counter p= g=ctr-test.one i=2 n=ctr.a v=0 d=0
-  test1: counter p= g=ctr-test.one i=2 n=ctr.b v=0 d=0
-  test1: counter p= g=ctr-test.one i=1 n=ctr.a v=0 d=0
-  test1: counter p= g=ctr-test.one i=1 n=ctr.b v=0 d=0
+  test1: counter p= g=ctr-test:one i=2 n=ctr:a v=0 d=0
+  test1: counter p= g=ctr-test:one i=2 n=ctr:b v=0 d=0
+  test1: counter p= g=ctr-test:one i=1 n=ctr:a v=0 d=0
+  test1: counter p= g=ctr-test:one i=1 n=ctr:b v=0 d=0
   test1: item p= g=test.one i=2 n=item.a v=-1 u=ma
   test1: item p= g=test.one i=2 n=item.b v=-1 u=kb
   test1: item p= g=test.one i=1 n=item.a v=-1 u=ma
   test1: item p= g=test.one i=1 n=item.b v=-1 u=kb
 report (srep2 enabled, no flush forced):
   test2: open
-  test2: counter p= g=ctr-test.one i=2 n=ctr.a v=0 d=0
-  test2: counter p= g=ctr-test.one i=2 n=ctr.b v=0 d=0
-  test2: counter p= g=ctr-test.one i=1 n=ctr.a v=0 d=0
-  test2: counter p= g=ctr-test.one i=1 n=ctr.b v=0 d=0
+  test2: counter p= g=ctr-test:one i=2 n=ctr:a v=0 d=0
+  test2: counter p= g=ctr-test:one i=2 n=ctr:b v=0 d=0
+  test2: counter p= g=ctr-test:one i=1 n=ctr:a v=0 d=0
+  test2: counter p= g=ctr-test:one i=1 n=ctr:b v=0 d=0
   test2: item p= g=test.one i=2 n=item.a v=-1 u=ma
   test2: item p= g=test.one i=2 n=item.b v=-1 u=kb
   test2: item p= g=test.one i=1 n=item.a v=-1 u=ma
   test2: item p= g=test.one i=1 n=item.b v=-1 u=kb
 report (should be empty):
 report (group 1, counter 1 update):
-  test2: counter p= g=ctr-test.one i=1 n=ctr.a v=1 d=1
-  test1: counter p= g=ctr-test.one i=1 n=ctr.a v=1 d=1
+  test2: counter p= g=ctr-test:one i=1 n=ctr:a v=1 d=1
+  test1: counter p= g=ctr-test:one i=1 n=ctr:a v=1 d=1
 report (group 1, item 1 update):
   test2: item p= g=test.one i=1 n=item.a v=10 u=ma
   test1: item p= g=test.one i=1 n=item.a v=10 u=ma
 report (remove statg1, ctrg1):
-  test2: counter p= g=ctr-test.one i=2 n=ctr.a v=0 d=0
-  test1: counter p= g=ctr-test.one i=2 n=ctr.a v=0 d=0
-  test2: counter p= g=ctr-test.one i=2 n=ctr.b v=0 d=0
-  test1: counter p= g=ctr-test.one i=2 n=ctr.b v=0 d=0
+  test2: counter p= g=ctr-test:one i=2 n=ctr:a v=0 d=0
+  test1: counter p= g=ctr-test:one i=2 n=ctr:a v=0 d=0
+  test2: counter p= g=ctr-test:one i=2 n=ctr:b v=0 d=0
+  test1: counter p= g=ctr-test:one i=2 n=ctr:b v=0 d=0
   test2: item p= g=test.one i=2 n=item.a v=-1 u=ma
   test1: item p= g=test.one i=2 n=item.a v=-1 u=ma
   test2: item p= g=test.one i=2 n=item.b v=-1 u=kb
   test1: item p= g=test.one i=2 n=item.b v=-1 u=kb
 report (remove srep1):
   test1: close
-  test2: counter p= g=ctr-test.one i=2 n=ctr.a v=0 d=0
-  test2: counter p= g=ctr-test.one i=2 n=ctr.b v=0 d=0
+  test2: counter p= g=ctr-test:one i=2 n=ctr:a v=0 d=0
+  test2: counter p= g=ctr-test:one i=2 n=ctr:b v=0 d=0
   test2: item p= g=test.one i=2 n=item.a v=-1 u=ma
   test2: item p= g=test.one i=2 n=item.b v=-1 u=kb
 report (remove statg2):
-  test2: counter p= g=ctr-test.one i=2 n=ctr.a v=0 d=0
-  test2: counter p= g=ctr-test.one i=2 n=ctr.b v=0 d=0
+  test2: counter p= g=ctr-test:one i=2 n=ctr:a v=0 d=0
+  test2: counter p= g=ctr-test:one i=2 n=ctr:b v=0 d=0
 report (remove srep2):
   test2: close
 report (remove ctrg2, should be empty):