summaryrefslogtreecommitdiffstats
path: root/tests/stats
diff options
context:
space:
mode:
authorNeels Hofmeyr <neels@hofmeyr.de>2017-12-20 01:14:31 +0100
committerNeels Hofmeyr <neels@hofmeyr.de>2017-12-20 01:29:59 +0100
commit554f7b8a773b3cffd74707c18172366ab45ea306 (patch)
treee04404c2111366a5c25f68ececf6227ecab3ab68 /tests/stats
parentc0b0b623053f16790d7d675812befe382ebdfd6e (diff)
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, &gtphub_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
Diffstat (limited to 'tests/stats')
-rw-r--r--tests/stats/stats_test.c10
1 files changed, 5 insertions, 5 deletions
diff --git a/tests/stats/stats_test.c b/tests/stats/stats_test.c
index 35faf9a6..6ef88418 100644
--- a/tests/stats/stats_test.c
+++ b/tests/stats/stats_test.c
@@ -324,16 +324,16 @@ static void test_reporting()
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);