Re: brconfig: strto*l -> strtonum()
On Sat, 08 Aug 2020 05:09:22 +0200, Klemens Nanni wrote: > Alternatively, we can avoid duplicating the ioctl specific min/max > values in strtonum(3) calls, just use the struct member type's *_MAX > defines and rely on the kernel for appropiate boundary checks - this is > what the code does now. I like this approach. OK millert@ - todd
Re: brconfig: strto*l -> strtonum()
On Wed, Jul 29, 2020 at 07:39:43PM +0200, Klemens Nanni wrote:
>
> Poking and testing around in brconfig.c for tpmr(4) stuff, I noticed a
> lot of old code around strto*l(3).
>
> Many pass unbounded `long' values into the `[u]int32_t' struct members
> without limiting them to at least the type size the value is stored in,
> some report wrong commands in error messages, e.g.
>
> # ifconfig switch1 portno vether1 9223372036854775808 # LONG_MAX + 1
> ifconfig: invalid arg for portidx: 9223372036854775808
>
> some pass values to the kernel that are above limits documented in
> ifconfig(8), e.g.
>
> # ifconfig bridge0 maxage 50
> ifconfig: bridge0: Invalid argument
>
> and others overflow in-kernel due to that (causing persistent damage
> that causes deleting and adding bridge members to fix it).
>
> I'm aware that moving to strtonum(3) drops the ability to pass numbers
> in bases others than ten, e.g. `ifconfig bridge0 ifcost vether0 0x3'
> mut now be `... 15', but all of the options really want a decimal number
> as far as I can judge after reading ifconfig(8) and testing them.
>
> The only exception to this is switch(4)'s `datapath' which is printed in
> hexadecimal, so I left it untouched such that it can be copy-pasted and
> set as is.
>
> Diff below applies converts all places to the very same strtonum() idiom
> with proper range checks based on documented or type limits.
>
> I've tested each option manually: valid values are still valid and but
> more out of range values are catched now, many of them up front in
> ifconfig rather than the kerne's ioctl() path.
>
> I've also checked most of the ioctl() paths to see what those really
> check for. One or two diffs for the kernel should follow soon.
>
> Feedback? OK?
Ping.
Alternatively, we can avoid duplicating the ioctl specific min/max
values in strtonum(3) calls, just use the struct member type's *_MAX
defines and rely on the kernel for appropiate boundary checks - this is
what the code does now.
Feedback? OK?
Index: brconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.28
diff -u -p -r1.28 brconfig.c
--- brconfig.c 5 Aug 2020 06:22:11 - 1.28
+++ brconfig.c 8 Aug 2020 03:04:30 -
@@ -419,18 +419,13 @@ void
bridge_timeout(const char *arg, int d)
{
struct ifbrparam bp;
- long newtime;
- char *endptr;
+ const char *errstr;
- errno = 0;
- newtime = strtol(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' ||
- (newtime & ~INT_MAX) != 0L ||
- (errno == ERANGE && newtime == LONG_MAX))
- errx(1, "invalid arg for timeout: %s", arg);
+ bp.ifbrp_ctime = strtonum(arg, 0, UINT32_MAX, &errstr);
+ if (errstr)
+ err(1, "timeout %s is: %s", arg, errstr);
strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
- bp.ifbrp_ctime = newtime;
if (ioctl(sock, SIOCBRDGSTO, (caddr_t)&bp) == -1)
err(1, "%s", ifname);
}
@@ -439,17 +434,13 @@ void
bridge_maxage(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for maxage: %s", arg);
+ bp.ifbrp_maxage = strtonum(arg, 0, UINT8_MAX, &errstr);
+ if (errstr)
+ errx(1, "maxage %s is: %s", arg, errstr);
strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
- bp.ifbrp_maxage = v;
if (ioctl(sock, SIOCBRDGSMA, (caddr_t)&bp) == -1)
err(1, "%s", ifname);
}
@@ -458,17 +449,13 @@ void
bridge_priority(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for spanpriority: %s", arg);
+ bp.ifbrp_prio = strtonum(arg, 0, UINT16_MAX, &errstr);
+ if (errstr)
+ errx(1, "spanpriority %s is: %s", arg, errstr);
strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
- bp.ifbrp_prio = v;
if (ioctl(sock, SIOCBRDGSPRI, (caddr_t)&bp) == -1)
err(1, "%s", ifname);
}
@@ -479,7 +466,7 @@ bridge_protect(const char *ifsname, cons
struct ifbreq breq;
unsigned long v;
char *optlist, *str;
- char *endptr;
+ const char *errstr;
strlcpy(breq.ifbr_name, ifname, sizeof(breq.ifbr_name));
strlcpy(breq.ifbr_ifsname, ifsname, sizeof(breq.ifbr_ifsname));
@@ -492,11 +479,9 @@ bridge_protect(const char *ifsname, cons
brconfig: strto*l -> strtonum()
Poking and testing around in brconfig.c for tpmr(4) stuff, I noticed a
lot of old code around strto*l(3).
Many pass unbounded `long' values into the `[u]int32_t' struct members
without limiting them to at least the type size the value is stored in,
some report wrong commands in error messages, e.g.
# ifconfig switch1 portno vether1 9223372036854775808 # LONG_MAX + 1
ifconfig: invalid arg for portidx: 9223372036854775808
some pass values to the kernel that are above limits documented in
ifconfig(8), e.g.
# ifconfig bridge0 maxage 50
ifconfig: bridge0: Invalid argument
and others overflow in-kernel due to that (causing persistent damage
that causes deleting and adding bridge members to fix it).
I'm aware that moving to strtonum(3) drops the ability to pass numbers
in bases others than ten, e.g. `ifconfig bridge0 ifcost vether0 0x3'
mut now be `... 15', but all of the options really want a decimal number
as far as I can judge after reading ifconfig(8) and testing them.
The only exception to this is switch(4)'s `datapath' which is printed in
hexadecimal, so I left it untouched such that it can be copy-pasted and
set as is.
Diff below applies converts all places to the very same strtonum() idiom
with proper range checks based on documented or type limits.
I've tested each option manually: valid values are still valid and but
more out of range values are catched now, many of them up front in
ifconfig rather than the kerne's ioctl() path.
I've also checked most of the ioctl() paths to see what those really
check for. One or two diffs for the kernel should follow soon.
Feedback? OK?
Index: brconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.26
diff -u -p -r1.26 brconfig.c
--- brconfig.c 29 Jul 2020 12:13:28 - 1.26
+++ brconfig.c 29 Jul 2020 17:06:09 -
@@ -418,18 +418,13 @@ void
bridge_timeout(const char *arg, int d)
{
struct ifbrparam bp;
- long newtime;
- char *endptr;
+ const char *errstr;
- errno = 0;
- newtime = strtol(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' ||
- (newtime & ~INT_MAX) != 0L ||
- (errno == ERANGE && newtime == LONG_MAX))
- errx(1, "invalid arg for timeout: %s", arg);
+ bp.ifbrp_ctime = strtonum(arg, 0, UINT32_MAX, &errstr);
+ if (errstr)
+ err(1, "timeout %s is: %s", arg, errstr);
strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
- bp.ifbrp_ctime = newtime;
if (ioctl(sock, SIOCBRDGSTO, (caddr_t)&bp) == -1)
err(1, "%s", ifname);
}
@@ -438,17 +433,13 @@ void
bridge_maxage(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for maxage: %s", arg);
+ bp.ifbrp_maxage = strtonum(arg, 6, 40, &errstr);
+ if (errstr)
+ errx(1, "maxage %s is: %s", arg, errstr);
strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
- bp.ifbrp_maxage = v;
if (ioctl(sock, SIOCBRDGSMA, (caddr_t)&bp) == -1)
err(1, "%s", ifname);
}
@@ -457,17 +448,13 @@ void
bridge_priority(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for spanpriority: %s", arg);
+ bp.ifbrp_prio = strtonum(arg, 0, 61440, &errstr);
+ if (errstr)
+ errx(1, "spanpriority %s is: %s", arg, errstr);
strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
- bp.ifbrp_prio = v;
if (ioctl(sock, SIOCBRDGSPRI, (caddr_t)&bp) == -1)
err(1, "%s", ifname);
}
@@ -478,7 +465,7 @@ bridge_protect(const char *ifsname, cons
struct ifbreq breq;
unsigned long v;
char *optlist, *str;
- char *endptr;
+ const char *errstr;
strlcpy(breq.ifbr_name, ifname, sizeof(breq.ifbr_name));
strlcpy(breq.ifbr_ifsname, ifsname, sizeof(breq.ifbr_ifsname));
@@ -491,11 +478,9 @@ bridge_protect(const char *ifsname, cons
str = strtok(optlist, ",");
while (str != NULL) {
- errno = 0;
- v = strtoul(str, &endptr, 0);
- if (str[0] == '\0' || endptr[0] != '\0' || v == 0 || v > 31 ||
- (errno == ERANGE && v == ULONG_MAX))
- err(1, "invalid value for protected domain: %s", str);
+ v = strtonum(str, 1, 31, &errs
