Widen the range of allowed cell broadcast channels #35

Flohack74 · 2022-06-24T12:30:56Z

In fact cell broadcast can subscribe to topics up to 9999 so lets remove that restriction

monich · 2022-09-12T18:50:08Z

Couple of comments.

Adding a new header for just a single define feels like an overkill. I suggest to add it to an existing file e.g. smsutil.h
9999/8+1 = 1250 bytes is more than I like to see being allocated from the stack. How about temporarily allocating it from the heap?

monich · 2022-09-12T18:51:21Z

And sorry for the delay

Flohack74 · 2022-09-19T22:08:57Z

@monich No worries, I updated as you suggested...

monich · 2022-09-20T12:07:17Z

unit/test-sms -p="/testsms/Range minimizer" crashes in a weird way:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000414f80 in cbs_optimize_ranges (ranges=0x48b210) at src/smsutil.c:4611
4611				*bitmap[byte_offset] |= 1 << bit;
(gdb) p bitmap_len
$1 = 1250
(gdb) p byte_offset
$2 = 416
(gdb) bt
#0  0x0000000000414f80 in cbs_optimize_ranges (ranges=0x43b890) at src/smsutil.c:4611
#1  0x000000000041524c in cbs_extract_topic_ranges (ranges=0x4184c0 "1-5, 3333") at src/smsutil.c:4682
#2  0x000000000040721c in test_range_minimizer () at unit/test-sms.c:1600
#3  0x0000007fbf99f0d4 in test_case_run (tc=0x4634f0) at ../glib/gtestutils.c:2655
#4  g_test_run_suite_internal (suite=suite@entry=0x461c60, path=path@entry=0x7ffffffa10 "/testsms/Range minimizer") at ../glib/gtestutils.c:2743
#5  0x0000007fbf99ef08 in g_test_run_suite_internal (suite=suite@entry=0x461c20, path=0x7ffffffa10 "/testsms/Range minimizer") at ../glib/gtestutils.c:2755
#6  0x0000007fbf99f4fc in g_test_run_suite (suite=0x461c20) at ../glib/gtestutils.c:2827
#7  0x0000007fbf99f560 in g_test_run () at ../glib/gtestutils.c:2064
#8  0x000000000040865c in main (argc=1, argv=0x7ffffff6f8) at unit/test-sms.c:1952
(gdb)

And this is how it looks from valgrind's point of view:

ok 22 /testsms/Test CBS Padding Character
==5603== Invalid read of size 1
==5603==    at 0x414F80: cbs_optimize_ranges (smsutil.c:4611)
==5603==    by 0x41524B: cbs_extract_topic_ranges (smsutil.c:4682)
==5603==    by 0x40721B: test_range_minimizer (test-sms.c:1600)
==5603==    by 0x4B500D3: test_case_run (gtestutils.c:2655)
==5603==    by 0x4B500D3: g_test_run_suite_internal (gtestutils.c:2743)
==5603==    by 0x4B4FF07: g_test_run_suite_internal (gtestutils.c:2755)
==5603==    by 0x4B50533: g_test_run_suite (gtestutils.c:2830)
==5603==    by 0x4B5055F: g_test_run (gtestutils.c:2064)
==5603==    by 0x40865B: main (test-sms.c:1952)
==5603==  Address 0x57ee470 is 518,672 bytes inside an unallocated block of size 3,876,224 in arena "client"

monich · 2022-09-20T12:10:25Z

BTW, the test itself needs to be updated e.g.

--- a/ofono/unit/test-sms.c
+++ b/ofono/unit/test-sms.c
@@ -1589,7 +1589,7 @@ static void test_cbs_padding_character(void)
 static const char *ranges[] = { "1-5, 2, 3, 600, 569-900, 999",
                                "0-20, 33, 44, 50-60, 20-50, 1-5, 5, 3, 5",
                                NULL };
-static const char *inv_ranges[] = { "1-5, 3333", "1-5, afbcd", "1-5, 3-5,,",
+static const char *inv_ranges[] = { "1-5, 33333", "1-5, afbcd", "1-5, 3-5,,",
                                        "1-5, 3-5, c", NULL };
 
 static void test_range_minimizer(void)

3333 is now a valid channel number

monich · 2022-09-20T12:13:27Z

ofono/src/smsutil.c

@@ -4593,12 +4593,13 @@ static gboolean next_range(const char *str, int *offset, gint *min, gint *max)
 GSList *cbs_optimize_ranges(GSList *ranges)
 {
 	struct cbs_topic_range *range;
-	unsigned char bitmap[125];
+	int bitmap_len = CBS_MAX_TOPIC / 8 + 1;
+	unsigned char (*bitmap)[bitmap_len];


This can be replaced with just one line:

unsigned char *bitmap = g_malloc0(CBS_MAX_TOPIC / 8 + 1);

monich · 2022-09-20T12:14:00Z

ofono/src/smsutil.c

 	GSList *l;
 	unsigned short i;
 	GSList *ret = NULL;

-	memset(bitmap, 0, sizeof(bitmap));
+	bitmap = g_malloc0(bitmap_len);


then this memset can just be deleted

monich · 2022-09-20T12:15:30Z

ofono/src/smsutil.c

@@ -4607,17 +4608,17 @@ GSList *cbs_optimize_ranges(GSList *ranges)
 			int byte_offset = i / 8;
 			int bit = i % 8;

-			bitmap[byte_offset] |= 1 << bit;
+			*bitmap[byte_offset] |= 1 << bit;


and this isn't necessary (besides this extra indirection seems to be wrong)

monich · 2022-09-20T12:16:09Z

ofono/src/smsutil.c

 		int byte_offset = i / 8;
 		int bit = i % 8;

-		if (is_bit_set(bitmap[byte_offset], bit) == FALSE) {
+		if (is_bit_set(*bitmap[byte_offset], bit) == FALSE) {


and this too is unnecessary

monich · 2022-09-20T12:20:57Z

All the necessary necessary changes can be reduced to something like this:

diff --git a/ofono/src/cbs.c b/ofono/src/cbs.c
index 8e3296b1..800aee8b 100644
--- a/ofono/src/cbs.c
+++ b/ofono/src/cbs.c
@@ -771,7 +771,7 @@ static void sim_cbmi_read_cb(int ok, int length, int record,
 
 		mi = (data[i] << 8) + data[i+1];
 
-		if (mi > 999)
+		if (mi > CBS_MAX_TOPIC)
 			continue;
 
 		range = g_new0(struct cbs_topic_range, 1);
@@ -818,7 +818,7 @@ static void sim_cbmir_read_cb(int ok, int length, int record,
 		min = (data[i] << 8) + data[i+1];
 		max = (data[i+2] << 8) + data[i+3];
 
-		if (min > 999 || max > 999 || min > max)
+		if (min > CBS_MAX_TOPIC || max > CBS_MAX_TOPIC || min > max)
 			continue;
 
 		range = g_new0(struct cbs_topic_range, 1);
diff --git a/ofono/src/smsutil.c b/ofono/src/smsutil.c
index d3d22243..29a6462f 100644
--- a/ofono/src/smsutil.c
+++ b/ofono/src/smsutil.c
@@ -4593,13 +4593,11 @@ out:
 GSList *cbs_optimize_ranges(GSList *ranges)
 {
 	struct cbs_topic_range *range;
-	unsigned char bitmap[125];
+	unsigned char *bitmap = g_malloc0(CBS_MAX_TOPIC / 8 + 1);
 	GSList *l;
 	unsigned short i;
 	GSList *ret = NULL;
 
-	memset(bitmap, 0, sizeof(bitmap));
-
 	for (l = ranges; l; l = l->next) {
 		range = l->data;
 
@@ -4613,7 +4611,7 @@ GSList *cbs_optimize_ranges(GSList *ranges)
 
 	range = NULL;
 
-	for (i = 0; i <= 999; i++) {
+	for (i = 0; i <= CBS_MAX_TOPIC; i++) {
 		int byte_offset = i / 8;
 		int bit = i % 8;
 
@@ -4641,6 +4639,7 @@ GSList *cbs_optimize_ranges(GSList *ranges)
 
 	ret = g_slist_reverse(ret);
 
+	g_free(bitmap);
 	return ret;
 }
 
@@ -4653,10 +4652,10 @@ GSList *cbs_extract_topic_ranges(const char *ranges)
 	GSList *tmp;
 
 	while (next_range(ranges, &offset, &min, &max) == TRUE) {
-		if (min < 0 || min > 999)
+		if (min < 0 || min > CBS_MAX_TOPIC)
 			return NULL;
 
-		if (max < 0 || max > 999)
+		if (max < 0 || max > CBS_MAX_TOPIC)
 			return NULL;
 
 		if (max < min)
diff --git a/ofono/src/smsutil.h b/ofono/src/smsutil.h
index 6197470a..ddd92143 100644
--- a/ofono/src/smsutil.h
+++ b/ofono/src/smsutil.h
@@ -23,6 +23,7 @@
 #include <ofono/types.h>
 
 #define CBS_MAX_GSM_CHARS 93
+#define CBS_MAX_TOPIC 9999
 #define SMS_MSGID_LEN 20
 
 enum sms_type {
diff --git a/ofono/unit/test-sms.c b/ofono/unit/test-sms.c
index 5ce262cf..95a60896 100644
--- a/ofono/unit/test-sms.c
+++ b/ofono/unit/test-sms.c
@@ -1589,7 +1589,7 @@ static void test_cbs_padding_character(void)
 static const char *ranges[] = { "1-5, 2, 3, 600, 569-900, 999",
 				"0-20, 33, 44, 50-60, 20-50, 1-5, 5, 3, 5",
 				NULL };
-static const char *inv_ranges[] = { "1-5, 3333", "1-5, afbcd", "1-5, 3-5,,",
+static const char *inv_ranges[] = { "1-5, 33333", "1-5, afbcd", "1-5, 3-5,,",
 					"1-5, 3-5, c", NULL };
 
 static void test_range_minimizer(void)

monich · 2022-10-25T10:51:40Z

I just pushed an alternative minimal PR #37

Flohack74 · 2022-10-26T22:18:42Z

Very cool, then this can be closed :)

monich · 2022-10-26T23:22:24Z

Merged #37, closing this one.

Widen the range of allowed cell broadcast channels

0a3122c

Flohack74 added 3 commits September 19, 2022 23:26

Review comment, unnecessary header

b4b3651

Do the cbs bitmap dynamically

ffebefc

Cleanup headers

f6d4bb3

monich reviewed Sep 20, 2022

View reviewed changes

monich mentioned this pull request Oct 25, 2022

Widen the range of allowed cell broadcast channels #37

Merged

monich closed this Oct 26, 2022

Flohack74 deleted the cbs-topics-max branch January 1, 2023 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Widen the range of allowed cell broadcast channels #35

Widen the range of allowed cell broadcast channels #35

Flohack74 commented Jun 24, 2022

monich commented Sep 12, 2022

monich commented Sep 12, 2022

Flohack74 commented Sep 19, 2022

monich commented Sep 20, 2022

monich commented Sep 20, 2022

monich Sep 20, 2022

monich Sep 20, 2022

monich Sep 20, 2022

monich Sep 20, 2022

monich commented Sep 20, 2022

monich commented Oct 25, 2022

Flohack74 commented Oct 26, 2022

monich commented Oct 26, 2022

Widen the range of allowed cell broadcast channels #35

Widen the range of allowed cell broadcast channels #35

Conversation

Flohack74 commented Jun 24, 2022

monich commented Sep 12, 2022

monich commented Sep 12, 2022

Flohack74 commented Sep 19, 2022

monich commented Sep 20, 2022

monich commented Sep 20, 2022

monich Sep 20, 2022

Choose a reason for hiding this comment

monich Sep 20, 2022

Choose a reason for hiding this comment

monich Sep 20, 2022

Choose a reason for hiding this comment

monich Sep 20, 2022

Choose a reason for hiding this comment

monich commented Sep 20, 2022

monich commented Oct 25, 2022

Flohack74 commented Oct 26, 2022

monich commented Oct 26, 2022