[PATCH] sbin/ipfw fixes (final)
Sepherosa Ziehau
sepherosa at gmail.com
Mon Apr 25 06:34:54 PDT 2005
the attached patch did the following:
1) check all possible fomat error after strtoul()
2) change '\0' to NULL, when it is compared with char pointer
3) strip blank line between if() and the conditional code
please review it
there are still things to do on sbin/ipfw:
1) show_ipfw():
code to output "from ..." and "to ..." are duplicated. will it be
better to factor them out?
2) fill_icmptype():
IP_FW_ICMPTYPES_DIM * sizeof(unsigned) * 8
could be replaced by IP_FW_ICMPTYPES_MAX
which is defined in net/ipfw/ip_fw.h
this expression also exists in show_ipfw()
types[icmptype / (sizeof(unsigned) * 8)] |=
1 << (icmptype % (sizeof(unsigned) * 8));
IMHO, define macros like SET_ICMPTYPE()/TEST_ICMPTYPE()
in net/ipfw/ip_fw.h will be much better than spread this set/test
all arround (at least in three locations, first here, second in
show_ipfw(), third in sys/net/ipfw/ip_fw.c icmptype_match())
last line in the main loop of this function could be moved
outside of it, since this loop will be go through least once.
this may be not worth noticing :-)
BTW, sorry for the previous mail about sbin/ipfw
--
Live Free or Die
Index: ipfw.c
===================================================================
RCS file: /opt/df_cvs/src/sbin/ipfw/ipfw.c,v
retrieving revision 1.7
diff -u -r1.7 ipfw.c
--- ipfw.c 18 Dec 2004 21:43:38 -0000 1.7
+++ ipfw.c 25 Apr 2005 13:03:57 -0000
@@ -676,6 +676,7 @@
int bcwidth = 0;
int n, num = 0;
int nbytes;
+ char *endptr;
/* get rules or pipes from kernel, resizing array as necessary */
{
@@ -704,10 +705,15 @@
struct dn_flow_queue *q;
int l;
- if (ac > 0)
- rulenum = strtoul(*av++, NULL, 10);
- else
+ if (ac > 0) {
+ rulenum = strtoul(*av++, &endptr, 10);
+ if (*endptr != '\0') {
+ err(EX_USAGE, "invalid pipe number: %s",
+ *(av - 1));
+ }
+ } else {
rulenum = 0;
+ }
for (; nbytes >= sizeof *p; p = (struct dn_pipe *)next) {
double b = p->bandwidth;
char buf[30];
@@ -796,7 +802,6 @@
while (ac--) {
u_long rnum;
- char *endptr;
int seen;
/* convert command line rule # */
@@ -940,6 +945,19 @@
return(0);
}
+static int
+fill_netmask(struct in_addr *mask, const char *wid_str)
+{
+ char *ep;
+ u_long wid;
+
+ wid = strtoul(wid_str, &ep, 10);
+ if (*ep != '\0' || wid > 32)
+ return -1;
+ mask->s_addr = (wid == 0 ? 0 : htonl(~0 << (32 - wid)));
+ return 0;
+}
+
static void
fill_ip(struct in_addr *ipno, struct in_addr *mask, int *acp, char ***avp)
{
@@ -966,14 +984,8 @@
errx(EX_DATAERR, "bad netmask ``%s''", p);
break;
case '/':
- if (atoi(p) == 0) {
- mask->s_addr = 0;
- } else if (atoi(p) > 32) {
+ if (fill_netmask(mask, p) < 0)
errx(EX_DATAERR, "bad width ``%s''", p);
- } else {
- mask->s_addr =
- htonl(~0 << (32 - atoi(p)));
- }
break;
default:
mask->s_addr = htonl(~0);
@@ -994,7 +1006,7 @@
u_long val;
char *s;
- if (str == '\0')
+ if (str == NULL)
errx(EX_DATAERR, "missing unreachable code");
val = strtoul(str, &s, 0);
if (s != str && *s == '\0' && val < 0x100) {
@@ -1369,7 +1381,7 @@
* Return in bits if flags is NULL, else flag bits
* or bytes in flags and return the unconverted value.
*/
- if (inbytes && flags)
+ if (inbytes && flags != NULL)
*flags |= DN_QSIZE_IS_BYTES;
else if (inbytes && flags == NULL)
val *= 8;
@@ -1459,6 +1471,10 @@
" missing", *av);
if (*av[1] == '/') {
a = strtoul(av[1]+1, &end, 0);
+ if (*end != '\0' || a > 32) {
+ err(EX_USAGE, "invalid %s mask"
+ " %s", av[0], av[1]);
+ }
if (a == 32) /* special case... */
a = ~0;
else
@@ -1466,6 +1482,10 @@
fprintf(stderr, " mask is 0x%08x\n", a);
} else {
a = strtoul(av[1], &end, 0);
+ if (*end != '\0') {
+ err(EX_USAGE, "invalid %s mask"
+ " %s", av[0], av[1]);
+ }
}
if (par == &pipe.fs.flow_mask.src_port
|| par == &pipe.fs.flow_mask.dst_port) {
@@ -1490,29 +1510,46 @@
} /* end for */
} else if (!strncmp(*av, "red", strlen(*av))
|| !strncmp(*av, "gred", strlen(*av))) {
+ char *ep;
/* RED enabled */
pipe.fs.flags_fs |= DN_IS_RED;
if (*av[0] == 'g')
pipe.fs.flags_fs |= DN_IS_GENTLE_RED;
if ((end = strsep(&av[1], "/"))) {
- double w_q = strtod(end, NULL);
+ double w_q = strtod(end, &ep);
+ if (*ep != '\0') {
+ err(EX_USAGE, "invalid value for w_q"
+ " %s", end);
+ }
if (w_q > 1 || w_q <= 0)
errx(EX_DATAERR, "w_q %f must be "
"0 < x <= 1", w_q);
pipe.fs.w_q = (int) (w_q * (1 << SCALE_RED));
}
if ((end = strsep(&av[1], "/"))) {
- pipe.fs.min_th = strtoul(end, &end, 0);
- if (*end == 'K' || *end == 'k')
+ pipe.fs.min_th = strtoul(end, &ep, 0);
+ if (*ep == 'K' || *ep == 'k')
pipe.fs.min_th *= 1024;
+ else if (*ep != '\0') {
+ err(EX_USAGE, "invalid value for"
+ " min_th %s", end);
+ }
}
if ((end = strsep(&av[1], "/"))) {
- pipe.fs.max_th = strtoul(end, &end, 0);
+ pipe.fs.max_th = strtoul(end, &ep, 0);
if (*end == 'K' || *end == 'k')
pipe.fs.max_th *= 1024;
+ else if (*ep != '\0') {
+ err(EX_USAGE, "invalid value for"
+ " max_th %s", end);
+ }
}
if ((end = strsep(&av[1], "/"))) {
- double max_p = strtod(end, NULL);
+ double max_p = strtod(end, &ep);
+ if (*ep != '\0') {
+ err(EX_USAGE, "invalid value for"
+ " max_p %s", end);
+ }
if (max_p > 1 || max_p <= 0)
errx(EX_DATAERR, "max_p %f must be "
"0 < x <= 1", max_p);
@@ -1550,7 +1587,11 @@
av += 2;
ac -= 2;
} else if (!strncmp(*av, "delay", len)) {
- pipe.delay = strtoul(av[1], NULL, 0);
+ pipe.delay = strtoul(av[1], &end, 0);
+ if (*end != '\0') {
+ err(EX_USAGE, "invalid value"
+ " for delay %s", av[1]);
+ }
av += 2;
ac -= 2;
} else {
@@ -1561,11 +1602,19 @@
if (!strncmp(*av, "weight", len)) {
pipe.fs.weight =
strtoul(av[1], &end, 0);
+ if (*end != '\0') {
+ err(EX_USAGE, "invalid value"
+ " for weight %s", av[1]);
+ }
av += 2;
ac -= 2;
} else if (!strncmp(*av, "pipe", len)) {
pipe.fs.parent_nr =
strtoul(av[1], &end, 0);
+ if (*end != '\0') {
+ err(EX_USAGE, "invalid value"
+ " for pipe %s", av[1]);
+ }
av += 2;
ac -= 2;
} else {
@@ -1615,9 +1664,8 @@
len = sizeof(int);
if (sysctlbyname("net.inet.ip.dummynet.red_lookup_depth",
&lookup_depth, &len, NULL, 0) == -1)
-
- errx(1, "sysctlbyname(\"%s\")",
- "net.inet.ip.dummynet.red_lookup_depth");
+ errx(1, "sysctlbyname(\"%s\")",
+ "net.inet.ip.dummynet.red_lookup_depth");
if (lookup_depth == 0)
errx(EX_DATAERR, "net.inet.ip.dummynet.red_lookup_depth"
" must be greater than zero");
@@ -1625,7 +1673,6 @@
len = sizeof(int);
if (sysctlbyname("net.inet.ip.dummynet.red_avg_pkt_size",
&avg_pkt_size, &len, NULL, 0) == -1)
-
errx(1, "sysctlbyname(\"%s\")",
"net.inet.ip.dummynet.red_avg_pkt_size");
if (avg_pkt_size == 0)
More information about the Submit
mailing list