fix osmo_mnc_from_str(): don't try to parse NULL

In osmo_mnc_from_str() do not try to return some values even if the validation
fails; hence don't try to decode a NULL pointer. That whole idea was half-baked
and a can of worms to begin with.

Change-Id: Ibaaa128ac60b941a015a31134eb52aef56bc6e22

3 files changed, 21 insertions, 23 deletions

diff --git a/src/gsm/gsm23003.c b/src/gsm/gsm23003.c
index 1f6bf7d0..574400de 100644
--- a/src/gsm/gsm23003.c
+++ b/src/gsm/gsm23003.c
@@ -206,7 +206,7 @@ void osmo_plmn_from_bcd(const uint8_t *bcd_src, struct osmo_plmn_id *plmn)
  * \param mnc[out]	MNC result buffer, or NULL.
  * \param[out] mnc_3_digits	Result buffer for 3-digit flag, or NULL.
  * \returns zero on success, -EINVAL in case of surplus characters, negative errno in case of conversion
- *          errors.
+ *          errors. In case of error, do not modify the out-arguments.
  */
 int osmo_mnc_from_str(const char *mnc_str, uint16_t *mnc, bool *mnc_3_digits)
 {
@@ -215,19 +215,17 @@ int osmo_mnc_from_str(const char *mnc_str, uint16_t *mnc, bool *mnc_3_digits)
 	char *endptr;
 	int rc = 0;
 
-	if (!mnc_str || !isdigit(mnc_str[0]) || strlen(mnc_str) > 3) {
-		/* return invalid, but give strtol a shot anyway, for callers that don't want to be
-		 * strict */
-		rc = -EINVAL;
-	}
+	if (!mnc_str || !isdigit(mnc_str[0]) || strlen(mnc_str) > 3)
+		return -EINVAL;
+
 	errno = 0;
 	_mnc = strtol(mnc_str, &endptr, 10);
 	if (errno)
 		rc = -errno;
 	else if (*endptr)
-		rc = -EINVAL;
+		return -EINVAL;
 	if (_mnc < 0 || _mnc > 999)
-		rc = -EINVAL;
+		return -ERANGE;
 	_mnc_3_digits = strlen(mnc_str) > 2;
 
 	if (mnc)
diff --git a/tests/gsm23003/gsm23003_test.c b/tests/gsm23003/gsm23003_test.c
index ea08d0d9..947aa18d 100644
--- a/tests/gsm23003/gsm23003_test.c
+++ b/tests/gsm23003/gsm23003_test.c
@@ -136,14 +136,14 @@ static struct test_mnc_from_str test_mnc_from_strs[] = {
 	{ "001", { 0, 1, true } },
 	{ "",	 { -EINVAL, 0, false } },
 	{ " ",	 { -EINVAL, 0, false } },
-	{ "-1",	 { -EINVAL, 65535, false } },
-	{ "1000", { -EINVAL, 1000, true } },
+	{ "-1",	 { -EINVAL, 0, false } },
+	{ "1000", { -EINVAL, 0, false } },
 	{ "0x",	 { -EINVAL, 0, false } },
-	{ " 23", { -EINVAL, 23, true } }, /* technically not a 3-digit MNC, but it's EINVAL anyway */
-	{ "23 ", { -EINVAL, 23, true } },
-	{ " 023", { -EINVAL, 23, true } },
-	{ "023 ", { -EINVAL, 23, true } },
-	{ "023 ", { -EINVAL, 23, true } },
+	{ " 23", { -EINVAL, 0, false } },
+	{ "23 ", { -EINVAL, 0, false } },
+	{ " 023", { -EINVAL, 0, false } },
+	{ "023 ", { -EINVAL, 0, false } },
+	{ "023 ", { -EINVAL, 0, false } },
 };
 
 static bool test_mnc_from_str()
@@ -154,7 +154,7 @@ static bool test_mnc_from_str()
 
 	for (i = 0; i < ARRAY_SIZE(test_mnc_from_strs); i++) {
 		struct test_mnc_from_str *t = &test_mnc_from_strs[i];
-		struct test_mnc_from_str_result result;
+		struct test_mnc_from_str_result result = {};
 		bool ok;
 
 		result.rc = osmo_mnc_from_str(t->mnc_str, &result.mnc,
diff --git a/tests/gsm23003/gsm23003_test.ok b/tests/gsm23003/gsm23003_test.ok
index 565c38cf..b435f450 100644
--- a/tests/gsm23003/gsm23003_test.ok
+++ b/tests/gsm23003/gsm23003_test.ok
@@ -51,11 +51,11 @@
  5: "001" rc=0 mnc=1 mnc_3_digits=1 pass
  6: "" rc=-22 mnc=0 mnc_3_digits=0 pass
  7: " " rc=-22 mnc=0 mnc_3_digits=0 pass
- 8: "-1" rc=-22 mnc=65535 mnc_3_digits=0 pass
- 9: "1000" rc=-22 mnc=1000 mnc_3_digits=1 pass
+ 8: "-1" rc=-22 mnc=0 mnc_3_digits=0 pass
+ 9: "1000" rc=-22 mnc=0 mnc_3_digits=0 pass
 10: "0x" rc=-22 mnc=0 mnc_3_digits=0 pass
-11: " 23" rc=-22 mnc=23 mnc_3_digits=1 pass
-12: "23 " rc=-22 mnc=23 mnc_3_digits=1 pass
-13: " 023" rc=-22 mnc=23 mnc_3_digits=1 pass
-14: "023 " rc=-22 mnc=23 mnc_3_digits=1 pass
-15: "023 " rc=-22 mnc=23 mnc_3_digits=1 pass
+11: " 23" rc=-22 mnc=0 mnc_3_digits=0 pass
+12: "23 " rc=-22 mnc=0 mnc_3_digits=0 pass
+13: " 023" rc=-22 mnc=0 mnc_3_digits=0 pass
+14: "023 " rc=-22 mnc=0 mnc_3_digits=0 pass
+15: "023 " rc=-22 mnc=0 mnc_3_digits=0 pass