This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Review at https://gerrit.osmocom.org/5516
rate_ctr: fix osmo-sgsn DoS: don't return NULL on already used index
Recent patch I563764af1d28043e909234ebb048239125ce6ecd introduced returning
NULL from rate_ctr_group_alloc() when the index passed already exists.
Instead of returning NULL, find an unused group index and use that, adjust the
error message.
In stats_test.c, adjust, and also assert allocated counter group indexes
everywhere.
Rationale:
The original patch causes osmo-sgsn to crash as soon as the second subscriber
attempts to establish an MM context. Of course osmo-sgsn is wrong to a) fail to
check a NULL return value and crash and b) to fail to allocate an MM context
just because the rate counter group could not be allocated (it still rejects
the MM context completely if rate_ctr_group_alloc() fails).
Nevertheless, the price we pay for rate counter correctness is, at least in
this instance, way too high: osmo-sgsn becomes completely unusable for more
than one subscriber.
Numerous other places exist where rate_ctr_group_alloc() is called with a
constant index number; from a quick grep magic I found these possible breaking
points:
osmo-sgsn/src/gprs/gb_proxy.c:1431: cfg->ctrg = rate_ctr_group_alloc(tall_bsc_ctx, &global_ctrg_desc, 0);
osmo-sgsn/src/gprs/gprs_sgsn.c:139: sgsn->rate_ctrs = rate_ctr_group_alloc(tall_bsc_ctx, &sgsn_ctrg_desc, 0);
osmo-sgsn/src/gprs/gprs_sgsn.c:270: ctx->ctrg = rate_ctr_group_alloc(ctx, &mmctx_ctrg_desc, 0);
osmo-sgsn/src/gprs/gtphub.c:888: b->counters_io = rate_ctr_group_alloc(osmo_gtphub_ctx,
>phub_ctrg_io_desc, 0);
osmo-bsc/src/libfilter/bsc_msg_acc.c:87: lst->stats = rate_ctr_group_alloc(lst, &bsc_cfg_acc_list_desc, 0);
osmo-pcu/src/bts.cpp:228: m_ratectrs = rate_ctr_group_alloc(tall_pcu_ctx, &bts_ctrg_desc, 0);
osmo-pcu/src/tbf.cpp:793: tbf->m_ctrs = rate_ctr_group_alloc(tbf, &tbf_ctrg_desc, 0);
osmo-pcu/src/tbf.cpp:879: tbf->m_ul_egprs_ctrs = rate_ctr_group_alloc(tbf, &tbf_ul_egprs_ctrg_desc, 0);
osmo-pcu/src/tbf.cpp:880: tbf->m_ul_gprs_ctrs = rate_ctr_group_alloc(tbf, &tbf_ul_gprs_ctrg_desc, 0);
osmo-pcu/src/tbf.cpp:970: tbf->m_dl_egprs_ctrs = rate_ctr_group_alloc(tbf, &tbf_dl_egprs_ctrg_desc, 0);
osmo-pcu/src/tbf.cpp:977: tbf->m_dl_gprs_ctrs = rate_ctr_group_alloc(tbf, &tbf_dl_gprs_ctrg_desc, 0);
osmo-pcu/src/tbf.cpp:1475: ul_tbf->m_ctrs = rate_ctr_group_alloc(ul_tbf, &tbf_ctrg_desc, 0);
osmo-pcu/src/bts.cpp:226: m_ratectrs = rate_ctr_group_alloc(tall_pcu_ctx, &bts_ctrg_desc, 1);
We can fix all of these callers and then reconsider returning NULL, but IMO
even into the future, rate counter group indexes are not something worth
failing to provide service for. For future bugs we should keep the automatic
index picking in case of index collisions. We will get an error message barfed
and can fix the issue in our own time, while the application remains completely
usable, and even the rate counters can still be queried (at wrong indexes, but
life is tough).
Related: I49aa95b610f2faec52dede2e4816da47ca1dfb14 (osmo-sgsn's segfault)
Change-Id: Iba6e41b8eeaea5ff6ed862bab3f34a62ab976914
---
M src/rate_ctr.c
M tests/stats/stats_test.c
2 files changed, 31 insertions(+), 8 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/16/5516/1
diff --git a/src/rate_ctr.c b/src/rate_ctr.c
index 72c6bb1..ec054c5 100644
--- a/src/rate_ctr.c
+++ b/src/rate_ctr.c
@@ -178,6 +178,27 @@
return NULL;
}
+/*! Find an unused index for this rate counter group.
+ * \param[in] name Name of the counter group
+ * \returns the largest used index number + 1, or 0 if none exist yet. */
+static unsigned int rate_ctr_get_unused_name_idx(const char *name)
+{
+ unsigned int idx = 0;
+ struct rate_ctr_group *ctrg;
+
+ llist_for_each_entry(ctrg, &rate_ctr_groups, list) {
+ if (!ctrg->desc)
+ continue;
+
+ if (strcmp(ctrg->desc->group_name_prefix, name))
+ continue;
+
+ if (idx <= ctrg->idx)
+ idx = ctrg->idx + 1;
+ }
+ return idx;
+}
+
/*! Allocate a new group of counters according to description
* \param[in] ctx \ref talloc context
* \param[in] desc Rate counter group description
@@ -191,9 +212,11 @@
struct rate_ctr_group *group;
if (rate_ctr_get_group_by_name_idx(desc->group_name_prefix, idx)) {
- LOGP(DLGLOBAL, LOGL_ERROR, "counter group '%s' already exists for index %u\n",
- desc->group_name_prefix, idx);
- return NULL; /* group already exist */
+ unsigned int new_idx = rate_ctr_get_unused_name_idx(desc->group_name_prefix);
+ LOGP(DLGLOBAL, LOGL_ERROR, "counter group '%s' already exists for index %u,"
+ " instead using index %u. This is a software bug that needs fixing.\n",
+ desc->group_name_prefix, idx, new_idx);
+ idx = new_idx;
}
size = sizeof(struct rate_ctr_group) +
diff --git a/tests/stats/stats_test.c b/tests/stats/stats_test.c
index 35faf9a..6ef8841 100644
--- a/tests/stats/stats_test.c
+++ b/tests/stats/stats_test.c
@@ -324,16 +324,16 @@
statg2 = osmo_stat_item_group_alloc(stats_ctx, &statg_desc, 2);
OSMO_ASSERT(statg2 != NULL);
ctrg1 = rate_ctr_group_alloc(stats_ctx, &ctrg_desc, 1);
- OSMO_ASSERT(ctrg1 != NULL);
+ OSMO_ASSERT(ctrg1 && ctrg1->idx == 1);
ctrg2 = rate_ctr_group_alloc(stats_ctx, &ctrg_desc, 2);
- OSMO_ASSERT(ctrg2 != NULL);
+ OSMO_ASSERT(ctrg2 && ctrg2->idx == 2);
ctrg_dup = rate_ctr_group_alloc(stats_ctx, &ctrg_desc, 2);
- if (ctrg_dup != NULL && ctrg2 != NULL)
- printf("FAIL: successfully allocated already existing counter group!\n");
+ OSMO_ASSERT(ctrg_dup && ctrg_dup->idx == 3);
+ rate_ctr_group_free(ctrg_dup);
ctrg3 = rate_ctr_group_alloc(stats_ctx, &ctrg_desc_dot, 3);
- OSMO_ASSERT(ctrg3 != NULL);
+ OSMO_ASSERT(ctrg3 && ctrg3->idx == 3);
srep1 = stats_reporter_create_test("test1");
OSMO_ASSERT(srep1 != NULL);
--
To view, visit https://gerrit.osmocom.org/5516
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iba6e41b8eeaea5ff6ed862bab3f34a62ab976914
Gerrit-PatchSet: 1
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>