[PATCH] sbin/ipfw fixes (final)

[Sepherosa Ziehau <sepherosa@xxxxxxxxx>]

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)