[Openvpn-devel] [M] Change in openvpn[master]: options: Factor out usages of strtoll and atoll

[flichtenheld (Code Review)]

Attention is currently required from: plaisthos.

Hello plaisthos,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1154?usp=email

to review the following change.


Change subject: options: Factor out usages of strtoll and atoll
......................................................................

options: Factor out usages of strtoll and atoll

This covers the cases where we actually want to
allow numbers > 2^31

Change-Id: I454126b3f8fa9d14501f6c4b1ed9ce7b2904be61
Signed-off-by: Frank Lichtenheld <fr...@lichtenheld.com>
---
M src/openvpn/options.c
M src/openvpn/options_util.c
M src/openvpn/options_util.h
M tests/unit_tests/openvpn/test_misc.c
4 files changed, 41 insertions(+), 12 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/54/1154/1

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 7c685e2..6e97ae4 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7181,8 +7181,7 @@
         options->inactivity_timeout = positive_atoi(p[1], msglevel);
         if (p[2])
         {
-            int64_t val = atoll(p[2]);
-            options->inactivity_minimum_bytes = (val < 0) ? 0 : val;
+            positive_atoll(p[2], &options->inactivity_minimum_bytes, p[0], 
msglevel);
             if (options->inactivity_minimum_bytes > INT_MAX)
             {
                 msg(M_WARN,
@@ -9528,26 +9527,18 @@
     else if (streq(p[0], "reneg-bytes") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_TLS_PARMS);
-        char *end;
-        long long reneg_bytes = strtoll(p[1], &end, 10);
-        if (*end != '\0' || reneg_bytes < 0)
+        if (!positive_atoll(p[1], &options->renegotiate_bytes, p[0], msglevel))
         {
-            msg(msglevel, "--reneg-bytes parameter must be an integer and >= 
0");
             goto err;
         }
-        options->renegotiate_bytes = reneg_bytes;
     }
     else if (streq(p[0], "reneg-pkts") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_TLS_PARMS);
-        char *end;
-        long long pkt_max = strtoll(p[1], &end, 10);
-        if (*end != '\0' || pkt_max < 0)
+        if (!positive_atoll(p[1], &options->renegotiate_packets, p[0], 
msglevel))
         {
-            msg(msglevel, "--reneg-pkts parameter must be an integer and >= 
0");
             goto err;
         }
-        options->renegotiate_packets = pkt_max;
     }
     else if (streq(p[0], "reneg-sec") && p[1] && !p[3])
     {
diff --git a/src/openvpn/options_util.c b/src/openvpn/options_util.c
index 32a9edb..e930078 100644
--- a/src/openvpn/options_util.c
+++ b/src/openvpn/options_util.c
@@ -131,6 +131,22 @@
     return (int)i;
 }

+bool
+positive_atoll(const char *str, int64_t *value, const char *name, int msglevel)
+{
+    char *endptr;
+    long long ll = strtoll(str, &endptr, 10);
+
+    if (ll < 0 || *endptr != '\0')
+    {
+        msg(msglevel, "%s: Cannot parse '%s' as non-negative integer", name, 
str);
+        return false;
+    }
+
+    *value = (int64_t)ll;
+    return true;
+}
+
 int
 atoi_warn(const char *str, int msglevel)
 {
diff --git a/src/openvpn/options_util.h b/src/openvpn/options_util.h
index b9e1569..5d38eda 100644
--- a/src/openvpn/options_util.h
+++ b/src/openvpn/options_util.h
@@ -41,6 +41,17 @@

 /**
  * Converts a str to an integer if the string can be represented as an
+ * integer number and is >= 0.
+ * The integer is stored in \p value.
+ * On error, print a warning with \p msglevel using \p name. \p value is
+ * not changed on error.
+ *
+ * @return \c true if the integer has been parsed and stored in value, \c 
false otherwise
+ */
+bool positive_atoll(const char *str, int64_t *value, const char *name, int 
msglevel);
+
+/**
+ * Converts a str to an integer if the string can be represented as an
  * integer number. Otherwise print a warning with \p msglevel and return 0
  */
 int atoi_warn(const char *str, int msglevel);
diff --git a/tests/unit_tests/openvpn/test_misc.c 
b/tests/unit_tests/openvpn/test_misc.c
index 2d2cc9e..ff96644 100644
--- a/tests/unit_tests/openvpn/test_misc.c
+++ b/tests/unit_tests/openvpn/test_misc.c
@@ -359,6 +359,17 @@
     assert_true(atoi_constrained("-1194", &parameter, "test", INT_MIN, 
INT_MAX, msglevel));
     assert_int_equal(parameter, -1194);

+    int64_t parameter64 = 0;
+    assert_true(positive_atoll("1234", &parameter64, "test", msglevel));
+    assert_int_equal(parameter64, 1234);
+    assert_true(positive_atoll("0", &parameter64, "test", msglevel));
+    assert_int_equal(parameter64, 0);
+    assert_true(positive_atoll("2147483653", &parameter64, "test", msglevel));
+    assert_int_equal(parameter64, 2147483653);
+    /* overflow gets capped to LLONG_MAX */
+    assert_true(positive_atoll("9223372036854775810", &parameter64, "test", 
msglevel));
+    assert_int_equal(parameter64, 9223372036854775807);
+
     CLEAR(mock_msg_buf);
     assert_int_equal(positive_atoi("-1234", msglevel), 0);
     assert_string_equal(mock_msg_buf, "Cannot parse argument '-1234' as 
non-negative integer");

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1154?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I454126b3f8fa9d14501f6c4b1ed9ce7b2904be61
Gerrit-Change-Number: 1154
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld <fr...@lichtenheld.com>
Gerrit-Reviewer: plaisthos <arne-open...@rfc2549.org>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-open...@rfc2549.org>
Gerrit-MessageType: newchange


_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel