[patch] have rtprio check that arguments are numeric; change atoi to strtol

[Kostik Belousov]

On Tue, Jan 04, 2011 at 11:40:45AM +0100, Giorgos Keramidas wrote:
> On Tue, 04 Jan 2011 11:36:38 +0100, Giorgos Keramidas <keramida at freebsd.org> wrote:
> > On Sun, 2 Jan 2011 18:46:47 -0500, Eitan Adler <lists at eitanadler.com> wrote:
> >> What about this patch? I incorporated  your feedback so I am not going
> >> to reply inline.
> >
> > Since the pattern of converting strings to int-derivative values appears
> > multiple times, I'd probably prefer something like a new function that
> > does the parsing *and* error-checking, to avoid duplication of errno
> > checks, invalidchar checks, and so on, e.g. something like this near the
> > top of rtprio.c:
> >
> >         #include <errno.h>
> >         #include <limits.h>
> >         #include <string.h>
> >
> >         /*
> >          * Parse an integer from 'string' into 'value'.  Return the first
> >          * invalid character in 'endp', if endp is non-NULL.  The return value
> >          * of parseint is 0 on success, -1 for any parse error.
> >          */
> >
> >         int
> >         parseint(const char *string, const char **endp, int base, int *value)
> >         {
> >                 long x;
> >
> >                 errno = 0;
> >                 x = strtol(string, endp, base);
> >                 if (errno != 0 || endp == str || (endp != NULL &&
> >                     endp != str && *endp != '\0' && (isdigit(*endp) == 0 ||
> >                     isspace(*endp) != 0)))
> >                         return -1;
> >                 if (x >= INT_MAX) {
> >                         errno = ERANGE;
> >                         return -1;
> >                 }
> >                 return (int)x;
> >         }
> 
> instead of `return (int)x' the last bits should be slightly different,
> of course:
> 
>                   if (value != NULL)
>                           *value = (int)x;
>                   return 0;
>           }
Well, I think it shall be simplified. What about this ?

diff --git a/usr.sbin/rtprio/rtprio.c b/usr.sbin/rtprio/rtprio.c
index 245f714..fc212b5 100644
--- a/usr.sbin/rtprio/rtprio.c
+++ b/usr.sbin/rtprio/rtprio.c
@@ -37,31 +37,31 @@ __FBSDID("$FreeBSD$");
 
 #include <sys/param.h>
 #include <sys/rtprio.h>
-#include <sys/errno.h>
 
 #include <ctype.h>
 #include <err.h>
+#include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
 
-static void usage();
+static int parseint(const char *, const char *);
+static void usage(void);
 
 int
-main(argc, argv)
-	int     argc;
-	char  **argv;
+main(int argc, char *argv[])
 {
-	char   *p;
-	int     proc = 0;
 	struct rtprio rtp;
+	char *p;
+	pid_t proc;
 
 	/* find basename */
 	if ((p = rindex(argv[0], '/')) == NULL)
 		p = argv[0];
 	else
 		++p;
+	proc = 0;
 
 	if (!strcmp(p, "rtprio"))
 		rtp.type = RTP_PRIO_REALTIME;
@@ -70,12 +70,12 @@ main(argc, argv)
 
 	switch (argc) {
 	case 2:
-		proc = abs(atoi(argv[1]));	/* Should check if numeric
-						 * arg! */
+		proc = parseint(argv[1], "pid");
+		proc = abs(proc);
 		/* FALLTHROUGH */
 	case 1:
 		if (rtprio(RTP_LOOKUP, proc, &rtp) != 0)
-			err(1, "%s", argv[0]);
+			err(1, "RTP_LOOKUP");
 		printf("%s: ", p);
 		switch (rtp.type) {
 		case RTP_PRIO_REALTIME:
@@ -103,19 +103,17 @@ main(argc, argv)
 					usage();
 					break;
 				}
-			} else {
-				rtp.prio = atoi(argv[1]);
-			}
+			} else
+				rtp.prio = parseint(argv[1], "priority");
 		} else {
 			usage();
 			break;
 		}
 
 		if (argv[2][0] == '-')
-			proc = -atoi(argv[2]);
-
+			proc = parseint(argv[2] + 1, "pid");
 		if (rtprio(RTP_SET, proc, &rtp) != 0)
-			err(1, "%s", argv[0]);
+			err(1, "RTP_SET");
 
 		if (proc == 0) {
 			execvp(argv[2], &argv[2]);
@@ -123,12 +121,28 @@ main(argc, argv)
 		}
 		exit(0);
 	}
-	exit (1);
+	exit(1);
+}
+
+static int
+parseint(const char *str, const char *errname)
+{
+	char *endp;
+	long res;
+
+	errno = 0;
+	res = strtol(str, &endp, 10);
+	if (errno != 0 || endp == str || *endp != '\0')
+		err(1, "%s shall be a number", errname);
+	if (res >= INT_MAX)
+		err(1, "Integer overflow parsing %s", errname);
+	return (res);
 }
 
 static void
-usage()
+usage(void)
 {
+
 	(void) fprintf(stderr, "%s\n%s\n%s\n%s\n%s\n%s\n",
 		"usage: [id|rt]prio",
 		"       [id|rt]prio [-]pid",
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20110104/d0e3be9b/attachment.pgp