[PATCH v9] options: Introduce boolean_flag() and review usages of atoi_warn

From: Frank Lichtenheld <[email protected]>

This uses atoi_constrained. Note that this makes the tests
stricter since previously any non-zero integer would be
interpreted as "true".

Change-Id: Ic30612971367a4aa54ca89f93452efc172d4f4e2
Signed-off-by: Frank Lichtenheld <[email protected]>
Acked-by: Razvan Cojocaru <[email protected]>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1151
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1151
This mail reflects revision 9 of this Change.

Acked-by according to Gerrit (reflected above):
Razvan Cojocaru <[email protected]>

        
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 2bca647..e75b452 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7748,7 +7748,7 @@
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->sc_info.challenge_text = p[1];
-        if (atoi_warn(p[2], msglevel))
+        if (boolean_flag(p[2], "static-challenge echo", msglevel))
         {
             options->sc_info.flags |= SC_ECHO;
         }
@@ -8123,7 +8123,7 @@
         options->exit_event_name = p[1];
         if (p[2])
         {
-            options->exit_event_initial_state = (atoi_warn(p[2], msglevel) != 0);
+            options->exit_event_initial_state = boolean_flag(p[2], "service state", msglevel);
         }
     }
     else if (streq(p[0], "allow-nonadmin") && !p[2])
@@ -9080,7 +9080,8 @@
     else if (streq(p[0], "show-pkcs11-ids") && !p[3])
     {
         char *provider = p[1];
-        bool cert_private = (p[2] == NULL ? false : (atoi_warn(p[2], msglevel) != 0));
+        bool cert_private = (p[2] == NULL ? false
+                                          : boolean_flag(p[2], "show-pkcs11-ids private", msglevel));
 
 #ifdef DEFAULT_PKCS11_MODULE
         if (!provider)
@@ -9126,14 +9127,12 @@
     }
     else if (streq(p[0], "pkcs11-protected-authentication"))
     {
-        int j;
-
         VERIFY_PERMISSION(OPT_P_GENERAL);
 
-        for (j = 1; j < MAX_PARMS && p[j] != NULL; ++j)
+        for (int j = 1; j < MAX_PARMS && p[j] != NULL; ++j)
         {
             options->pkcs11_protected_authentication[j - 1] =
-                atoi_warn(p[j], msglevel) != 0 ? 1 : 0;
+                boolean_flag(p[j], p[0], msglevel);
         }
     }
     else if (streq(p[0], "pkcs11-private-mode") && p[1])
@@ -9149,13 +9148,11 @@
     }
     else if (streq(p[0], "pkcs11-cert-private"))
     {
-        int j;
-
         VERIFY_PERMISSION(OPT_P_GENERAL);
 
-        for (j = 1; j < MAX_PARMS && p[j] != NULL; ++j)
+        for (int j = 1; j < MAX_PARMS && p[j] != NULL; ++j)
         {
-            options->pkcs11_cert_private[j - 1] = (bool)(atoi_warn(p[j], msglevel));
+            options->pkcs11_cert_private[j - 1] = boolean_flag(p[j], p[0], msglevel);
         }
     }
     else if (streq(p[0], "pkcs11-pin-cache") && p[1] && !p[2])
diff --git a/src/openvpn/options_util.c b/src/openvpn/options_util.c
index 8d0a143..c99d5f2 100644
--- a/src/openvpn/options_util.c
+++ b/src/openvpn/options_util.c
@@ -193,6 +193,14 @@
     return true;
 }
 
+bool
+boolean_flag(const char *str, const char *name, msglvl_t msglevel)
+{
+    int number = 0;
+    atoi_constrained(str, &number, name, 0, 1, msglevel);
+    return (bool)number;
+}
+
 static const char *updatable_options[] = { "block-ipv6", "block-outside-dns",
                                            "dhcp-option", "dns",
                                            "ifconfig", "ifconfig-ipv6",
diff --git a/src/openvpn/options_util.h b/src/openvpn/options_util.h
index 511d189..d0ea0a3 100644
--- a/src/openvpn/options_util.h
+++ b/src/openvpn/options_util.h
@@ -69,6 +69,12 @@
                       msglvl_t msglevel);
 
 /**
+ * Converts a str to an boolean if the string can be parsed as either 0 or 1.
+ * Otherwise print a warning with \p msglevel and return \c false.
+ */
+bool boolean_flag(const char *str, const char *name, msglvl_t msglevel);
+
+/**
  * Filter an option line by all pull filters.
  *
  * If a match is found, the line is modified depending on
diff --git a/tests/unit_tests/openvpn/test_misc.c b/tests/unit_tests/openvpn/test_misc.c
index a9ae33f..4fa57c1 100644
--- a/tests/unit_tests/openvpn/test_misc.c
+++ b/tests/unit_tests/openvpn/test_misc.c
@@ -442,6 +442,22 @@
     assert_string_equal(mock_msg_buf, "test: Must be an integer >= 1, not 0");
     assert_int_equal(parameter, -42);
 
+    /* special tests for boolean_flag */
+    assert_false(boolean_flag("0", "test", msglevel));
+    assert_true(boolean_flag("1", "test", msglevel));
+
+    CLEAR(mock_msg_buf);
+    assert_false(boolean_flag("foo77", "test", msglevel));
+    assert_string_equal(mock_msg_buf, "test: Cannot parse 'foo77' as integer");
+
+    CLEAR(mock_msg_buf);
+    assert_false(boolean_flag("-77", "test", msglevel));
+    assert_string_equal(mock_msg_buf, "test: Must be an integer between 0 and 1, not -77");
+
+    CLEAR(mock_msg_buf);
+    assert_false(boolean_flag("77", "test", msglevel));
+    assert_string_equal(mock_msg_buf, "test: Must be an integer between 0 and 1, not 77");
+
     mock_set_debug_level(saved_log_level);
 }