Skip to content

Commit a01c909

Browse files
flichtenheldcron2
authored andcommitted
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 <[email protected]> Acked-by: Arne Schwabe <[email protected]> Message-Id: <[email protected]> URL: https://sourceforge.net/p/openvpn/mailman/message/59228172/ Signed-off-by: Gert Doering <[email protected]>
1 parent fccdb21 commit a01c909

File tree

4 files changed

+137
-107
lines changed

4 files changed

+137
-107
lines changed

src/openvpn/options.c

Lines changed: 42 additions & 106 deletions
Original file line numberDiff line numberDiff line change
@@ -6411,16 +6411,12 @@ add_option(struct options *options, char *p[], bool is_inline, const char *file,
64116411
}
64126412
else if (streq(p[0], "management-log-cache") && p[1] && !p[2])
64136413
{
6414-
int cache;
6415-
64166414
VERIFY_PERMISSION(OPT_P_GENERAL);
6417-
cache = atoi_warn(p[1], msglevel);
6418-
if (cache < 1)
6415+
if (!atoi_constrained(p[1], &options->management_log_history_cache,
6416+
p[0], 1, INT_MAX, msglevel))
64196417
{
6420-
msg(msglevel, "--management-log-cache parameter is out of range");
64216418
goto err;
64226419
}
6423-
options->management_log_history_cache = cache;
64246420
}
64256421
#endif /* ifdef ENABLE_MANAGEMENT */
64266422
#ifdef ENABLE_PLUGIN
@@ -6969,16 +6965,11 @@ add_option(struct options *options, char *p[], bool is_inline, const char *file,
69696965
}
69706966
else if (streq(p[0], "status-version") && p[1] && !p[2])
69716967
{
6972-
int version;
6973-
69746968
VERIFY_PERMISSION(OPT_P_GENERAL);
6975-
version = atoi_warn(p[1], msglevel);
6976-
if (version < 1 || version > 3)
6969+
if (!atoi_constrained(p[1], &options->status_file_version, p[0], 1, 3, msglevel))
69776970
{
6978-
msg(msglevel, "--status-version must be 1 to 3");
69796971
goto err;
69806972
}
6981-
options->status_file_version = version;
69826973
}
69836974
else if (streq(p[0], "remap-usr1") && p[1] && !p[2])
69846975
{
@@ -7151,16 +7142,11 @@ add_option(struct options *options, char *p[], bool is_inline, const char *file,
71517142
}
71527143
else if (streq(p[0], "shaper") && p[1] && !p[2])
71537144
{
7154-
int shaper;
7155-
71567145
VERIFY_PERMISSION(OPT_P_SHAPER);
7157-
shaper = atoi_warn(p[1], msglevel);
7158-
if (shaper < SHAPER_MIN || shaper > SHAPER_MAX)
7146+
if (!atoi_constrained(p[1], &options->shaper, p[0], SHAPER_MIN, SHAPER_MAX, msglevel))
71597147
{
7160-
msg(msglevel, "Bad shaper value, must be between %d and %d", SHAPER_MIN, SHAPER_MAX);
71617148
goto err;
71627149
}
7163-
options->shaper = shaper;
71647150
}
71657151
else if (streq(p[0], "port") && p[1] && !p[2])
71667152
{
@@ -7739,7 +7725,11 @@ add_option(struct options *options, char *p[], bool is_inline, const char *file,
77397725
else if (streq(p[0], "script-security") && p[1] && !p[2])
77407726
{
77417727
VERIFY_PERMISSION(OPT_P_GENERAL);
7742-
script_security_set(atoi_warn(p[1], msglevel));
7728+
int security;
7729+
if (atoi_constrained(p[1], &security, p[0], SSEC_NONE, SSEC_PW_ENV, msglevel))
7730+
{
7731+
script_security_set(security);
7732+
}
77437733
}
77447734
else if (streq(p[0], "mssfix") && !p[3])
77457735
{
@@ -7959,11 +7949,9 @@ add_option(struct options *options, char *p[], bool is_inline, const char *file,
79597949
int real, virtual;
79607950

79617951
VERIFY_PERMISSION(OPT_P_GENERAL);
7962-
real = atoi_warn(p[1], msglevel);
7963-
virtual = atoi_warn(p[2], msglevel);
7964-
if (real < 1 || virtual < 1)
7952+
if (!atoi_constrained(p[1], &real, "hash-size real", 1, INT_MAX, msglevel)
7953+
|| !atoi_constrained(p[2], &virtual, "hash-size virtual", 1, INT_MAX, msglevel))
79657954
{
7966-
msg(msglevel, "--hash-size sizes must be >= 1 (preferably a power of 2)");
79677955
goto err;
79687956
}
79697957
options->real_hash_size = real;
@@ -7974,49 +7962,34 @@ add_option(struct options *options, char *p[], bool is_inline, const char *file,
79747962
int cf_max, cf_per;
79757963

79767964
VERIFY_PERMISSION(OPT_P_GENERAL);
7977-
cf_max = atoi_warn(p[1], msglevel);
7978-
cf_per = atoi_warn(p[2], msglevel);
7979-
if (cf_max < 0 || cf_per < 0)
7965+
if (!atoi_constrained(p[1], &cf_max, "connect-freq n", 1, INT_MAX, msglevel)
7966+
|| !atoi_constrained(p[2], &cf_per, "connect-freq seconds", 1, INT_MAX, msglevel))
79807967
{
7981-
msg(msglevel, "--connect-freq parms must be > 0");
79827968
goto err;
79837969
}
79847970
options->cf_max = cf_max;
79857971
options->cf_per = cf_per;
79867972
}
79877973
else if (streq(p[0], "connect-freq-initial") && p[1] && p[2] && !p[3])
79887974
{
7989-
long cf_max, cf_per;
7975+
int cf_max, cf_per;
79907976

79917977
VERIFY_PERMISSION(OPT_P_GENERAL);
7992-
char *e1, *e2;
7993-
cf_max = strtol(p[1], &e1, 10);
7994-
cf_per = strtol(p[2], &e2, 10);
7995-
if (cf_max < 0 || cf_per < 0 || *e1 != '\0' || *e2 != '\0')
7978+
if (!atoi_constrained(p[1], &cf_max, "connect-freq-initial n", 1, INT_MAX, msglevel)
7979+
|| !atoi_constrained(p[2], &cf_per, "connect-freq-initial seconds", 1, INT_MAX, msglevel))
79967980
{
7997-
msg(msglevel, "--connect-freq-initial parameters must be integers and >= 0");
79987981
goto err;
79997982
}
80007983
options->cf_initial_max = cf_max;
80017984
options->cf_initial_per = cf_per;
80027985
}
80037986
else if (streq(p[0], "max-clients") && p[1] && !p[2])
80047987
{
8005-
int max_clients;
8006-
80077988
VERIFY_PERMISSION(OPT_P_GENERAL);
8008-
max_clients = atoi_warn(p[1], msglevel);
8009-
if (max_clients < 0)
7989+
if (!atoi_constrained(p[1], &options->max_clients, p[0], 1, MAX_PEER_ID, msglevel))
80107990
{
8011-
msg(msglevel, "--max-clients must be at least 1");
80127991
goto err;
80137992
}
8014-
if (max_clients >= MAX_PEER_ID) /* max peer-id value */
8015-
{
8016-
msg(msglevel, "--max-clients must be less than %d", MAX_PEER_ID);
8017-
goto err;
8018-
}
8019-
options->max_clients = max_clients;
80207993
}
80217994
else if (streq(p[0], "max-routes-per-client") && p[1] && !p[2])
80227995
{
@@ -8188,27 +8161,13 @@ add_option(struct options *options, char *p[], bool is_inline, const char *file,
81888161
}
81898162
else if (streq(p[0], "bcast-buffers") && p[1] && !p[2])
81908163
{
8191-
int n_bcast_buf;
8192-
81938164
VERIFY_PERMISSION(OPT_P_GENERAL);
8194-
n_bcast_buf = atoi_warn(p[1], msglevel);
8195-
if (n_bcast_buf < 1)
8196-
{
8197-
msg(msglevel, "--bcast-buffers parameter must be > 0");
8198-
}
8199-
options->n_bcast_buf = n_bcast_buf;
8165+
atoi_constrained(p[1], &options->n_bcast_buf, p[0], 1, INT_MAX, msglevel);
82008166
}
82018167
else if (streq(p[0], "tcp-queue-limit") && p[1] && !p[2])
82028168
{
8203-
int tcp_queue_limit;
8204-
82058169
VERIFY_PERMISSION(OPT_P_GENERAL);
8206-
tcp_queue_limit = atoi_warn(p[1], msglevel);
8207-
if (tcp_queue_limit < 1)
8208-
{
8209-
msg(msglevel, "--tcp-queue-limit parameter must be > 0");
8210-
}
8211-
options->tcp_queue_limit = tcp_queue_limit;
8170+
atoi_constrained(p[1], &options->tcp_queue_limit, p[0], 1, INT_MAX, msglevel);
82128171
}
82138172
#if PORT_SHARE
82148173
else if (streq(p[0], "port-share") && p[1] && p[2] && !p[4])
@@ -8354,21 +8313,24 @@ add_option(struct options *options, char *p[], bool is_inline, const char *file,
83548313
int ageing_time, check_interval;
83558314

83568315
VERIFY_PERMISSION(OPT_P_GENERAL);
8357-
ageing_time = atoi_warn(p[1], msglevel);
8316+
if (!atoi_constrained(p[1], &ageing_time, "stale-routes-check age", 1, INT_MAX, msglevel))
8317+
{
8318+
goto err;
8319+
}
8320+
83588321
if (p[2])
83598322
{
8360-
check_interval = atoi_warn(p[2], msglevel);
8323+
if (!atoi_constrained(p[2], &check_interval,
8324+
"stale-routes-check interval", 1, INT_MAX, msglevel))
8325+
{
8326+
goto err;
8327+
}
83618328
}
83628329
else
83638330
{
83648331
check_interval = ageing_time;
83658332
}
83668333

8367-
if (ageing_time < 1 || check_interval < 1)
8368-
{
8369-
msg(msglevel, "--stale-routes-check aging time and check interval must be >= 1");
8370-
goto err;
8371-
}
83728334
options->stale_routes_ageing_time = ageing_time;
83738335
options->stale_routes_check_interval = check_interval;
83748336
}
@@ -8386,7 +8348,7 @@ add_option(struct options *options, char *p[], bool is_inline, const char *file,
83868348
else if (streq(p[0], "push-continuation") && p[1] && !p[2])
83878349
{
83888350
VERIFY_PERMISSION(OPT_P_PULL_MODE);
8389-
options->push_continuation = atoi_warn(p[1], msglevel);
8351+
atoi_constrained(p[1], &options->push_continuation, p[0], 0, 2, msglevel);
83908352
}
83918353
else if (streq(p[0], "auth-user-pass") && !p[2])
83928354
{
@@ -8505,33 +8467,23 @@ add_option(struct options *options, char *p[], bool is_inline, const char *file,
85058467
{
85068468
if (!streq(p[2], "default"))
85078469
{
8508-
int offset = atoi_warn(p[2], msglevel);
8470+
int offset;
85098471

8510-
if (!(offset > -256 && offset < 256))
8472+
if (!atoi_constrained(p[2], &offset, "ip-win32 offset", -256, 256, msglevel))
85118473
{
8512-
msg(msglevel,
8513-
"--ip-win32 dynamic [offset] [lease-time]: offset (%d) must be > -256 and < 256",
8514-
offset);
85158474
goto err;
85168475
}
8517-
85188476
to->dhcp_masq_custom_offset = true;
85198477
to->dhcp_masq_offset = offset;
85208478
}
85218479

85228480
if (p[3])
85238481
{
8524-
const int min_lease = 30;
8525-
int lease_time;
8526-
lease_time = atoi_warn(p[3], msglevel);
8527-
if (lease_time < min_lease)
8482+
if (!atoi_constrained(p[3], &to->dhcp_lease_time,
8483+
"ip-win32 lease time", 30, INT_MAX, msglevel))
85288484
{
8529-
msg(msglevel,
8530-
"--ip-win32 dynamic [offset] [lease-time]: lease time parameter (%d) must be at least %d seconds",
8531-
lease_time, min_lease);
85328485
goto err;
85338486
}
8534-
to->dhcp_lease_time = lease_time;
85358487
}
85368488
}
85378489
}
@@ -8629,8 +8581,7 @@ add_option(struct options *options, char *p[], bool is_inline, const char *file,
86298581
}
86308582
else if (streq(p[1], "NBT") && p[2] && !p[3])
86318583
{
8632-
int t;
8633-
t = atoi_warn(p[2], msglevel);
8584+
int t = atoi_warn(p[2], msglevel);
86348585
if (!(t == 1 || t == 2 || t == 4 || t == 8))
86358586
{
86368587
msg(msglevel, "--dhcp-option NBT: parameter (%d) must be 1, 2, 4, or 8", t);
@@ -8704,15 +8655,11 @@ add_option(struct options *options, char *p[], bool is_inline, const char *file,
87048655
}
87058656
else if (streq(p[0], "tap-sleep") && p[1] && !p[2])
87068657
{
8707-
int s;
87088658
VERIFY_PERMISSION(OPT_P_DHCPDNS);
8709-
s = atoi_warn(p[1], msglevel);
8710-
if (s < 0 || s >= 256)
8659+
if (!atoi_constrained(p[1], &options->tuntap_options.tap_sleep, p[0], 0, 255, msglevel))
87118660
{
8712-
msg(msglevel, "--tap-sleep parameter must be between 0 and 255");
87138661
goto err;
87148662
}
8715-
options->tuntap_options.tap_sleep = s;
87168663
}
87178664
else if (streq(p[0], "dhcp-renew") && !p[1])
87188665
{
@@ -9152,30 +9099,19 @@ add_option(struct options *options, char *p[], bool is_inline, const char *file,
91529099
VERIFY_PERMISSION(OPT_P_GENERAL);
91539100
if (p[1])
91549101
{
9155-
int replay_window;
9156-
9157-
replay_window = atoi_warn(p[1], msglevel);
9158-
if (!(MIN_SEQ_BACKTRACK <= replay_window && replay_window <= MAX_SEQ_BACKTRACK))
9102+
if (!atoi_constrained(p[1], &options->replay_window, "replay-window windows size",
9103+
MIN_SEQ_BACKTRACK, MAX_SEQ_BACKTRACK, msglevel))
91599104
{
9160-
msg(msglevel, "replay-window window size parameter (%d) must be between %d and %d",
9161-
replay_window, MIN_SEQ_BACKTRACK, MAX_SEQ_BACKTRACK);
91629105
goto err;
91639106
}
9164-
options->replay_window = replay_window;
91659107

91669108
if (p[2])
91679109
{
9168-
int replay_time;
9169-
9170-
replay_time = atoi_warn(p[2], msglevel);
9171-
if (!(MIN_TIME_BACKTRACK <= replay_time && replay_time <= MAX_TIME_BACKTRACK))
9110+
if (!atoi_constrained(p[2], &options->replay_time, "replay-window time window",
9111+
MIN_TIME_BACKTRACK, MAX_TIME_BACKTRACK, msglevel))
91729112
{
9173-
msg(msglevel,
9174-
"replay-window time window parameter (%d) must be between %d and %d",
9175-
replay_time, MIN_TIME_BACKTRACK, MAX_TIME_BACKTRACK);
91769113
goto err;
91779114
}
9178-
options->replay_time = replay_time;
91799115
}
91809116
}
91819117
else
@@ -9771,7 +9707,7 @@ add_option(struct options *options, char *p[], bool is_inline, const char *file,
97719707
else if (!p[2])
97729708
{
97739709
char *endp = NULL;
9774-
int i = strtol(provider, &endp, 10);
9710+
long i = strtol(provider, &endp, 10);
97759711

97769712
if (*endp == 0)
97779713
{
@@ -9842,7 +9778,7 @@ add_option(struct options *options, char *p[], bool is_inline, const char *file,
98429778
else if (streq(p[0], "pkcs11-pin-cache") && p[1] && !p[2])
98439779
{
98449780
VERIFY_PERMISSION(OPT_P_GENERAL);
9845-
options->pkcs11_pin_cache_period = atoi_warn(p[1], msglevel);
9781+
options->pkcs11_pin_cache_period = positive_atoi(p[1], msglevel);
98469782
}
98479783
else if (streq(p[0], "pkcs11-id") && p[1] && !p[2])
98489784
{

src/openvpn/options_util.c

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,37 @@ atoi_warn(const char *str, int msglevel)
146146
return (int)i;
147147
}
148148

149+
bool
150+
atoi_constrained(const char *str, int *value, const char *name, int min, int max, int msglevel)
151+
{
152+
ASSERT(min < max);
153+
154+
char *endptr;
155+
long long i = strtoll(str, &endptr, 10);
156+
if (i < INT_MIN || *endptr != '\0' || i > INT_MAX)
157+
{
158+
msg(msglevel, "%s: Cannot parse '%s' as integer", name, str);
159+
return false;
160+
}
161+
if (i < min || i > max)
162+
{
163+
if (max == INT_MAX) /* nicer message for common case */
164+
{
165+
msg(msglevel, "%s: Must be an integer >= %d, not %lld",
166+
name, min, i);
167+
}
168+
else
169+
{
170+
msg(msglevel, "%s: Must be an integer between %d and %d, not %lld",
171+
name, min, max, i);
172+
}
173+
return false;
174+
}
175+
176+
*value = i;
177+
return true;
178+
}
179+
149180
static const char *updatable_options[] = { "block-ipv6", "block-outside-dns",
150181
"dhcp-option", "dns",
151182
"ifconfig", "ifconfig-ipv6",

src/openvpn/options_util.h

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,22 @@ int positive_atoi(const char *str, int msglevel);
4141

4242
/**
4343
* Converts a str to an integer if the string can be represented as an
44-
* integer number. Otherwise print a warning with msglevel and return 0
44+
* integer number. Otherwise print a warning with \p msglevel and return 0
4545
*/
4646
int atoi_warn(const char *str, int msglevel);
4747

48+
/**
49+
* Converts a str to an integer if the string can be represented as an
50+
* integer number and is between \p min and \p max.
51+
* The integer is stored in \p value.
52+
* On error, print a warning with \p msglevel using \p name. \p value is
53+
* not changed on error.
54+
*
55+
* @return \c true if the integer has been parsed and stored in value, \c false otherwise
56+
*/
57+
bool atoi_constrained(const char *str, int *value, const char *name, int min, int max,
58+
int msglevel);
59+
4860
/**
4961
* Filter an option line by all pull filters.
5062
*

0 commit comments

Comments
 (0)