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

[Kostik Belousov]

On Sun, Jan 02, 2011 at 06:46:47PM -0500, Eitan Adler wrote:
> What about this patch? I incorporated  your feedback so I am not going
> to reply inline.
> 
> > The syntax of the prio commands is weird, there is an obvious corner
> > (or wrongly handled) case, where the command name starts with a digit.
> > Command name starting with dash is even harder.
> >
> 
> I agree - and I wouldn't mind seeing the syntax changed (along with
> the licensed changed to a 2 clause BSD license) - but I suspect the
> benefits conferred by those two things would not be enough to overcome
> the reluctance to change a very old command (since 1994). If I'm wrong
> I'll gladly write a "cleanroom" version with sane syntax.
> 
> Index: rtprio.c
> ===================================================================
> --- rtprio.c	(revision 216679)
> +++ rtprio.c	(working copy)
> @@ -41,6 +41,7 @@
> 
>  #include <ctype.h>
>  #include <err.h>
> +#include <libgen.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -54,14 +55,12 @@
>  	char  **argv;
>  {
>  	char   *p;
> -	int     proc = 0;
> +	pid_t     proc;
>  	struct rtprio rtp;
> +	char *invalidchar;
> 
> -	/* find basename */
> -	if ((p = rindex(argv[0], '/')) == NULL)
> -		p = argv[0];
> -	else
> -		++p;
> +	proc = 0;
> +	p = basename(argv[0]);
> 
>  	if (!strcmp(p, "rtprio"))
>  		rtp.type = RTP_PRIO_REALTIME;
> @@ -70,8 +69,10 @@
> 
>  	switch (argc) {
>  	case 2:
> -		proc = abs(atoi(argv[1]));	/* Should check if numeric
> -						 * arg! */
> +		proc = (int)strtol(argv[1], &invalidchar, 10);
> +		if (*invalidchar != '\0')
> +			errx(1,"Process should be a pid");
> +		proc = abs(proc);
>  		/* FALLTHROUGH */
>  	case 1:
>  		if (rtprio(RTP_LOOKUP, proc, &rtp) != 0)
> @@ -104,16 +105,20 @@
>  					break;
>  				}
>  			} else {
> -				rtp.prio = atoi(argv[1]);
> +				rtp.prio = (int)strtol(argv[1], &invalidchar, 10);
> +				if (*invalidchar != '\0')
> +					errx(1,"Priority should be a number", invalidchar);
This did not compiled for me.

>  			}
>  		} else {
>  			usage();
>  			break;
>  		}
> 
> -		if (argv[2][0] == '-')
> -			proc = -atoi(argv[2]);
> -
> +		if (argv[2][0] == '-') {
> +			proc = (int)strtol(argv[2]+1, &invalidchar, 10);
> +			if (*invalidchar != '\0')
> +				errx(1,"Process should be a pid");
> +		}
>  		if (rtprio(RTP_SET, proc, &rtp) != 0)
>  			err(1, "%s", argv[0]);
> 
> 
Unless loud objections are heard, I indend to commit the following patch,
based on your submission.

diff --git a/usr.sbin/rtprio/rtprio.c b/usr.sbin/rtprio/rtprio.c
index 245f714..bdd61ea 100644
--- a/usr.sbin/rtprio/rtprio.c
+++ b/usr.sbin/rtprio/rtprio.c
@@ -37,7 +37,6 @@ __FBSDID("$FreeBSD$");
 
 #include <sys/param.h>
 #include <sys/rtprio.h>
-#include <sys/errno.h>
 
 #include <ctype.h>
 #include <err.h>
@@ -46,22 +45,21 @@ __FBSDID("$FreeBSD$");
 #include <string.h>
 #include <unistd.h>
 
-static void usage();
+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 *invalidchar, *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 +68,14 @@ main(argc, argv)
 
 	switch (argc) {
 	case 2:
-		proc = abs(atoi(argv[1]));	/* Should check if numeric
-						 * arg! */
+		proc = strtol(argv[1], &invalidchar, 10);
+		if (*invalidchar != '\0' || invalidchar == argv[1])
+			errx(1, "Pid should be a number");
+		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:
@@ -104,18 +104,23 @@ main(argc, argv)
 					break;
 				}
 			} else {
-				rtp.prio = atoi(argv[1]);
+				rtp.prio = strtol(argv[1], &invalidchar, 10);
+				if (*invalidchar != '\0' ||
+				    invalidchar == argv[1])
+					errx(1, "Priority should be a number");
 			}
 		} else {
 			usage();
 			break;
 		}
 
-		if (argv[2][0] == '-')
-			proc = -atoi(argv[2]);
-
+		if (argv[2][0] == '-') {
+			proc = strtol(argv[2] + 1, &invalidchar, 10);
+			if (*invalidchar != '\0' || invalidchar == argv[2] + 1)
+				errx(1, "Pid should be a number");
+		}
 		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 +128,13 @@ main(argc, argv)
 		}
 		exit(0);
 	}
-	exit (1);
+	exit(1);
 }
 
 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/20110103/b92b98b1/attachment.pgp