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/+/1150?usp=email
to review the following change.
Change subject: options: Introduce atoi_constrained and review usages of
atoi_warn
......................................................................
options: Introduce atoi_constrained and review usages of atoi_warn
This is a more powerful version of atoi_warn that can
- check minimum and maximum values
- report error seperately from parsed value
This can be used to simplify a lot of option parsing.
Change-Id: Ibc7526d59c1de17a0f9d8ed88f75c6f070ab11e7
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, 143 insertions(+), 107 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/50/1150/1
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index e9584a8..b62e22c 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -6405,16 +6405,12 @@
}
else if (streq(p[0], "management-log-cache") && p[1] && !p[2])
{
- int cache;
-
VERIFY_PERMISSION(OPT_P_GENERAL);
- cache = atoi_warn(p[1], msglevel);
- if (cache < 1)
+ if (!atoi_constrained(p[1], &options->management_log_history_cache,
+ "management-log-cache", 1, 0, msglevel))
{
- msg(msglevel, "--management-log-cache parameter is out of range");
goto err;
}
- options->management_log_history_cache = cache;
}
#endif /* ifdef ENABLE_MANAGEMENT */
#ifdef ENABLE_PLUGIN
@@ -6963,16 +6959,11 @@
}
else if (streq(p[0], "status-version") && p[1] && !p[2])
{
- int version;
-
VERIFY_PERMISSION(OPT_P_GENERAL);
- version = atoi_warn(p[1], msglevel);
- if (version < 1 || version > 3)
+ if (!atoi_constrained(p[1], &options->status_file_version,
"status-version", 1, 3, msglevel))
{
- msg(msglevel, "--status-version must be 1 to 3");
goto err;
}
- options->status_file_version = version;
}
else if (streq(p[0], "remap-usr1") && p[1] && !p[2])
{
@@ -7145,16 +7136,11 @@
}
else if (streq(p[0], "shaper") && p[1] && !p[2])
{
- int shaper;
-
VERIFY_PERMISSION(OPT_P_SHAPER);
- shaper = atoi_warn(p[1], msglevel);
- if (shaper < SHAPER_MIN || shaper > SHAPER_MAX)
+ if (!atoi_constrained(p[1], &options->shaper, "shaper", SHAPER_MIN,
SHAPER_MAX, msglevel))
{
- msg(msglevel, "Bad shaper value, must be between %d and %d",
SHAPER_MIN, SHAPER_MAX);
goto err;
}
- options->shaper = shaper;
}
else if (streq(p[0], "port") && p[1] && !p[2])
{
@@ -7723,7 +7709,12 @@
else if (streq(p[0], "script-security") && p[1] && !p[2])
{
VERIFY_PERMISSION(OPT_P_GENERAL);
- script_security_set(atoi_warn(p[1], msglevel));
+ int security;
+ if (atoi_constrained(p[1], &security,
+ "script-security", SSEC_NONE, SSEC_PW_ENV,
msglevel))
+ {
+ script_security_set(security);
+ }
}
else if (streq(p[0], "mssfix") && !p[3])
{
@@ -7943,11 +7934,9 @@
int real, virtual;
VERIFY_PERMISSION(OPT_P_GENERAL);
- real = atoi_warn(p[1], msglevel);
- virtual = atoi_warn(p[2], msglevel);
- if (real < 1 || virtual < 1)
+ if (!atoi_constrained(p[1], &real, "hash-size real", 1, 0, msglevel)
+ || !atoi_constrained(p[2], &virtual, "hash-size virtual", 1, 0,
msglevel))
{
- msg(msglevel, "--hash-size sizes must be >= 1 (preferably a power
of 2)");
goto err;
}
options->real_hash_size = real;
@@ -7958,11 +7947,9 @@
int cf_max, cf_per;
VERIFY_PERMISSION(OPT_P_GENERAL);
- cf_max = atoi_warn(p[1], msglevel);
- cf_per = atoi_warn(p[2], msglevel);
- if (cf_max < 0 || cf_per < 0)
+ if (!atoi_constrained(p[1], &cf_max, "connect-freq n", 1, 0, msglevel)
+ || !atoi_constrained(p[2], &cf_per, "connect-freq seconds", 1, 0,
msglevel))
{
- msg(msglevel, "--connect-freq parms must be > 0");
goto err;
}
options->cf_max = cf_max;
@@ -7970,15 +7957,12 @@
}
else if (streq(p[0], "connect-freq-initial") && p[1] && p[2] && !p[3])
{
- long cf_max, cf_per;
+ int cf_max, cf_per;
VERIFY_PERMISSION(OPT_P_GENERAL);
- char *e1, *e2;
- cf_max = strtol(p[1], &e1, 10);
- cf_per = strtol(p[2], &e2, 10);
- if (cf_max < 0 || cf_per < 0 || *e1 != '\0' || *e2 != '\0')
+ if (!atoi_constrained(p[1], &cf_max, "connect-freq-initial n", 1, 0,
msglevel)
+ || !atoi_constrained(p[2], &cf_per, "connect-freq-initial
seconds", 1, 0, msglevel))
{
- msg(msglevel, "--connect-freq-initial parameters must be integers
and >= 0");
goto err;
}
options->cf_initial_max = cf_max;
@@ -7986,21 +7970,11 @@
}
else if (streq(p[0], "max-clients") && p[1] && !p[2])
{
- int max_clients;
-
VERIFY_PERMISSION(OPT_P_GENERAL);
- max_clients = atoi_warn(p[1], msglevel);
- if (max_clients < 0)
+ if (!atoi_constrained(p[1], &options->max_clients, "max-clients", 1,
MAX_PEER_ID, msglevel))
{
- msg(msglevel, "--max-clients must be at least 1");
goto err;
}
- if (max_clients >= MAX_PEER_ID) /* max peer-id value */
- {
- msg(msglevel, "--max-clients must be less than %d", MAX_PEER_ID);
- goto err;
- }
- options->max_clients = max_clients;
}
else if (streq(p[0], "max-routes-per-client") && p[1] && !p[2])
{
@@ -8172,27 +8146,13 @@
}
else if (streq(p[0], "bcast-buffers") && p[1] && !p[2])
{
- int n_bcast_buf;
-
VERIFY_PERMISSION(OPT_P_GENERAL);
- n_bcast_buf = atoi_warn(p[1], msglevel);
- if (n_bcast_buf < 1)
- {
- msg(msglevel, "--bcast-buffers parameter must be > 0");
- }
- options->n_bcast_buf = n_bcast_buf;
+ atoi_constrained(p[1], &options->n_bcast_buf, "bcast-buffers", 1, 0,
msglevel);
}
else if (streq(p[0], "tcp-queue-limit") && p[1] && !p[2])
{
- int tcp_queue_limit;
-
VERIFY_PERMISSION(OPT_P_GENERAL);
- tcp_queue_limit = atoi_warn(p[1], msglevel);
- if (tcp_queue_limit < 1)
- {
- msg(msglevel, "--tcp-queue-limit parameter must be > 0");
- }
- options->tcp_queue_limit = tcp_queue_limit;
+ atoi_constrained(p[1], &options->tcp_queue_limit, "tcp-queue-limit",
1, 0, msglevel);
}
#if PORT_SHARE
else if (streq(p[0], "port-share") && p[1] && p[2] && !p[4])
@@ -8338,21 +8298,24 @@
int ageing_time, check_interval;
VERIFY_PERMISSION(OPT_P_GENERAL);
- ageing_time = atoi_warn(p[1], msglevel);
+ if (!atoi_constrained(p[1], &ageing_time, "stale-routes-check age", 1,
0, msglevel))
+ {
+ goto err;
+ }
+
if (p[2])
{
- check_interval = atoi_warn(p[2], msglevel);
+ if (!atoi_constrained(p[1], &check_interval,
+ "stale-routes-check interval", 1, 0,
msglevel))
+ {
+ goto err;
+ }
}
else
{
check_interval = ageing_time;
}
- if (ageing_time < 1 || check_interval < 1)
- {
- msg(msglevel, "--stale-routes-check aging time and check interval
must be >= 1");
- goto err;
- }
options->stale_routes_ageing_time = ageing_time;
options->stale_routes_check_interval = check_interval;
}
@@ -8370,7 +8333,7 @@
else if (streq(p[0], "push-continuation") && p[1] && !p[2])
{
VERIFY_PERMISSION(OPT_P_PULL_MODE);
- options->push_continuation = atoi_warn(p[1], msglevel);
+ atoi_constrained(p[1], &options->push_continuation,
"push-continuation", 0, 2, msglevel);
}
else if (streq(p[0], "auth-user-pass") && !p[2])
{
@@ -8489,33 +8452,22 @@
{
if (!streq(p[2], "default"))
{
- int offset = atoi_warn(p[2], msglevel);
+ int offset;
- if (!(offset > -256 && offset < 256))
+ if (!atoi_constrained(p[1], &offset, "ip-win32 offset",
-256, 256, msglevel))
{
- msg(msglevel,
- "--ip-win32 dynamic [offset] [lease-time]: offset
(%d) must be > -256 and < 256",
- offset);
goto err;
}
-
to->dhcp_masq_custom_offset = true;
to->dhcp_masq_offset = offset;
}
if (p[3])
{
- const int min_lease = 30;
- int lease_time;
- lease_time = atoi_warn(p[3], msglevel);
- if (lease_time < min_lease)
+ if (!atoi_constrained(p[1], &to->dhcp_lease_time,
"ip-win32 lease time", 30, 0, msglevel))
{
- msg(msglevel,
- "--ip-win32 dynamic [offset] [lease-time]: lease
time parameter (%d) must be at least %d seconds",
- lease_time, min_lease);
goto err;
}
- to->dhcp_lease_time = lease_time;
}
}
}
@@ -8613,8 +8565,7 @@
}
else if (streq(p[1], "NBT") && p[2] && !p[3])
{
- int t;
- t = atoi_warn(p[2], msglevel);
+ int t = atoi_warn(p[2], msglevel);
if (!(t == 1 || t == 2 || t == 4 || t == 8))
{
msg(msglevel, "--dhcp-option NBT: parameter (%d) must be 1, 2,
4, or 8", t);
@@ -8688,15 +8639,12 @@
}
else if (streq(p[0], "tap-sleep") && p[1] && !p[2])
{
- int s;
VERIFY_PERMISSION(OPT_P_DHCPDNS);
- s = atoi_warn(p[1], msglevel);
- if (s < 0 || s >= 256)
+ if (!atoi_constrained(p[1], &options->tuntap_options.tap_sleep,
+ "tap-sleep", 0, 255, msglevel))
{
- msg(msglevel, "--tap-sleep parameter must be between 0 and 255");
goto err;
}
- options->tuntap_options.tap_sleep = s;
}
else if (streq(p[0], "dhcp-renew") && !p[1])
{
@@ -9136,30 +9084,19 @@
VERIFY_PERMISSION(OPT_P_GENERAL);
if (p[1])
{
- int replay_window;
-
- replay_window = atoi_warn(p[1], msglevel);
- if (!(MIN_SEQ_BACKTRACK <= replay_window && replay_window <=
MAX_SEQ_BACKTRACK))
+ if (!atoi_constrained(p[1], &options->replay_window,
"replay-window windows size",
+ MIN_SEQ_BACKTRACK, MAX_SEQ_BACKTRACK,
msglevel))
{
- msg(msglevel, "replay-window window size parameter (%d) must
be between %d and %d",
- replay_window, MIN_SEQ_BACKTRACK, MAX_SEQ_BACKTRACK);
goto err;
}
- options->replay_window = replay_window;
if (p[2])
{
- int replay_time;
-
- replay_time = atoi_warn(p[2], msglevel);
- if (!(MIN_TIME_BACKTRACK <= replay_time && replay_time <=
MAX_TIME_BACKTRACK))
+ if (!atoi_constrained(p[2], &options->replay_time,
"replay-window time window",
+ MIN_TIME_BACKTRACK, MAX_TIME_BACKTRACK,
msglevel))
{
- msg(msglevel,
- "replay-window time window parameter (%d) must be
between %d and %d",
- replay_time, MIN_TIME_BACKTRACK, MAX_TIME_BACKTRACK);
goto err;
}
- options->replay_time = replay_time;
}
}
else
@@ -9755,7 +9692,7 @@
else if (!p[2])
{
char *endp = NULL;
- int i = strtol(provider, &endp, 10);
+ long i = strtol(provider, &endp, 10);
if (*endp == 0)
{
@@ -9826,7 +9763,7 @@
else if (streq(p[0], "pkcs11-pin-cache") && p[1] && !p[2])
{
VERIFY_PERMISSION(OPT_P_GENERAL);
- options->pkcs11_pin_cache_period = atoi_warn(p[1], msglevel);
+ options->pkcs11_pin_cache_period = positive_atoi(p[1], msglevel);
}
else if (streq(p[0], "pkcs11-id") && p[1] && !p[2])
{
diff --git a/src/openvpn/options_util.c b/src/openvpn/options_util.c
index c3938a7..c1f7c74 100644
--- a/src/openvpn/options_util.c
+++ b/src/openvpn/options_util.c
@@ -146,6 +146,41 @@
return (int)i;
}
+bool
+atoi_constrained(const char *str, int *value, const char *name, int min, int
max, int msglevel)
+{
+ if (!max)
+ {
+ max = INT_MAX;
+ }
+ ASSERT(min < max);
+
+ char *endptr;
+ long long i = strtoll(str, &endptr, 10);
+ if (i < INT_MIN || *endptr != '\0' || i > INT_MAX)
+ {
+ msg(msglevel, "%s: Cannot parse '%s' as integer", name, str);
+ return false;
+ }
+ if (i < min || i > max)
+ {
+ if (max == INT_MAX) /* nicer message for common case */
+ {
+ msg(msglevel, "%s: Must be an integer >= %d, not %lld",
+ name, min, i);
+ }
+ else
+ {
+ msg(msglevel, "%s: Must be an integer between %d and %d, not %lld",
+ name, min, max, i);
+ }
+ return false;
+ }
+
+ *value = i;
+ return true;
+}
+
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 6f81b1e..d5ae8b9 100644
--- a/src/openvpn/options_util.h
+++ b/src/openvpn/options_util.h
@@ -41,11 +41,24 @@
/**
* Converts a str to an integer if the string can be represented as an
- * integer number. Otherwise print a warning with msglevel and return 0
+ * integer number. Otherwise print a warning with \p msglevel and return 0
*/
int atoi_warn(const char *str, int msglevel);
/**
+ * Converts a str to an integer if the string can be represented as an
+ * integer number and is between \p min and \p max.
+ * If \p max is 0, \p INT_MAX is used instead.
+ * 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 atoi_constrained(const char *str, int *value, const char *name, int min,
int max,
+ int 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 1857922..80a5dee 100644
--- a/tests/unit_tests/openvpn/test_misc.c
+++ b/tests/unit_tests/openvpn/test_misc.c
@@ -351,6 +351,14 @@
assert_int_equal(atoi_warn("0", msglevel), 0);
assert_int_equal(atoi_warn("-1194", msglevel), -1194);
+ int parameter = 0;
+ assert_true(atoi_constrained("1234", ¶meter, "test", 0, 0, msglevel));
+ assert_int_equal(parameter, 1234);
+ assert_true(atoi_constrained("0", ¶meter, "test", -1, 0, msglevel));
+ assert_int_equal(parameter, 0);
+ assert_true(atoi_constrained("-1194", ¶meter, "test", INT_MIN,
INT_MAX, msglevel));
+ assert_int_equal(parameter, -1194);
+
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");
@@ -365,6 +373,12 @@
assert_string_equal(mock_msg_buf, "Cannot parse argument '2147483653' as
integer");
CLEAR(mock_msg_buf);
+ parameter = -42;
+ assert_false(atoi_constrained("2147483653", ¶meter, "test", 0, 0,
msglevel));
+ assert_string_equal(mock_msg_buf, "test: Cannot parse '2147483653' as
integer");
+ assert_int_equal(parameter, -42);
+
+ CLEAR(mock_msg_buf);
assert_int_equal(positive_atoi("foo77", msglevel), 0);
assert_string_equal(mock_msg_buf, "Cannot parse argument 'foo77' as
non-negative integer");
@@ -373,6 +387,18 @@
assert_string_equal(mock_msg_buf, "Cannot parse argument '77foo' as
non-negative integer");
CLEAR(mock_msg_buf);
+ parameter = -42;
+ assert_false(atoi_constrained("foo77", ¶meter, "test", 0, 0,
msglevel));
+ assert_string_equal(mock_msg_buf, "test: Cannot parse 'foo77' as integer");
+ assert_int_equal(parameter, -42);
+
+ CLEAR(mock_msg_buf);
+ parameter = -42;
+ assert_false(atoi_constrained("77foo", ¶meter, "test", 0, 0,
msglevel));
+ assert_string_equal(mock_msg_buf, "test: Cannot parse '77foo' as integer");
+ assert_int_equal(parameter, -42);
+
+ CLEAR(mock_msg_buf);
assert_int_equal(atoi_warn("foo77", msglevel), 0);
assert_string_equal(mock_msg_buf, "Cannot parse argument 'foo77' as
integer");
@@ -380,6 +406,31 @@
assert_int_equal(atoi_warn("77foo", msglevel), 0);
assert_string_equal(mock_msg_buf, "Cannot parse argument '77foo' as
integer");
+ /* special tests for _constrained */
+ CLEAR(mock_msg_buf);
+ parameter = -42;
+ assert_false(atoi_constrained("77", ¶meter, "test", 0, 76, msglevel));
+ assert_string_equal(mock_msg_buf, "test: Must be an integer between 0 and
76, not 77");
+ assert_int_equal(parameter, -42);
+
+ CLEAR(mock_msg_buf);
+ parameter = -42;
+ assert_false(atoi_constrained("-77", ¶meter, "test", -76, 76,
msglevel));
+ assert_string_equal(mock_msg_buf, "test: Must be an integer between -76
and 76, not -77");
+ assert_int_equal(parameter, -42);
+
+ CLEAR(mock_msg_buf);
+ parameter = -42;
+ assert_false(atoi_constrained("-77", ¶meter, "test", 0, 0, msglevel));
+ assert_string_equal(mock_msg_buf, "test: Must be an integer >= 0, not
-77");
+ assert_int_equal(parameter, -42);
+
+ CLEAR(mock_msg_buf);
+ parameter = -42;
+ assert_false(atoi_constrained("0", ¶meter, "test", 1, 0, msglevel));
+ assert_string_equal(mock_msg_buf, "test: Must be an integer >= 1, not 0");
+ assert_int_equal(parameter, -42);
+
mock_set_debug_level(saved_log_level);
}
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1150?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: Ibc7526d59c1de17a0f9d8ed88f75c6f070ab11e7
Gerrit-Change-Number: 1150
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
