login
Search
When looking at the rtprio(1) source I noticed that the dg in revision
3291 has indicated that he wanted to check to ensure that the user
provided priority was numeric. Also the man page for atoi says the
function is deprecated - so I replaced those functions as well.
Index: rtprio.c
===================================================================
--- rtprio.c (revision 216679)
+++ rtprio.c (working copy)
@@ -56,6 +56,7 @@
char *p;
int proc = 0;
struct rtprio rtp;
+ int c;
/* find basename */
if ((p = rindex(argv[0], '/')) == NULL)
@@ -70,8 +71,10 @@
switch (argc) {
case 2:
- proc = abs(atoi(argv[1])); /* Should check if numeric
- * arg! */
+ for (c=0; c < strlen(argv[1]); ++c)
+ if (!isdigit(argv[1][c]))
+ errx(1,"%s", "Priority should be a number");
+ proc = (int)strtol(argv[1], (char **)NULL, 10);
/* FALLTHROUGH */
case 1:
if (rtprio(RTP_LOOKUP, proc, &rtp) != 0)
@@ -104,7 +107,10 @@
break;
}
} else {
- rtp.prio = atoi(argv[1]);
+ for (c=0; c < strlen(argv[1]); ++c)
+ if (!isdigit(argv[1][c]))
+ errx(1,"%s", "Priority should be a number");
+ rtp.prio = (int)strtol(argv[1], (char **)NULL, 10);
}
} else {
usage();
@@ -112,7 +118,7 @@
}
if (argv[2][0] == '-')
- proc = -atoi(argv[2]);
+ proc = -(int)strtol(argv[2], (char **)NULL, 10);
if (rtprio(RTP_SET, proc, &rtp) != 0)
err(1, "%s", argv[0]);
--
Eitan Adler
_______________________________________________
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"
How about this version? It also corrects a copy/paste error I have above
Index: rtprio.c
===================================================================
--- rtprio.c (revision 216679)
+++ rtprio.c (working copy)
@@ -56,6 +56,7 @@
char *p;
int proc = 0;
struct rtprio rtp;
+ char *invalidChar;
/* find basename */
if ((p = rindex(argv[0], '/')) == NULL)
@@ -70,8 +71,9 @@
switch (argc) {
case 2:
- proc = abs(atoi(argv[1])); /* Should check if numeric
- * arg! */
+ proc = abs((int)strtol(argv[1], &invalidChar, 10));
+ if (*invalidChar != '\0')
+ errx(1,"Process should be a number");
/* FALLTHROUGH */
case 1:
if (rtprio(RTP_LOOKUP, proc, &rtp) != 0)
@@ -104,16 +106,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);
}
} else {
usage();
break;
}
- if (argv[2][0] == '-')
- proc = -atoi(argv[2]);
-
+ if (argv[2][0] == '-') {
+ proc = -(int)strtol(argv[2], &invalidChar, 10);
+ if (*invalidChar != '\0')
+ errx(1,"Process should be a number");
+ }
if (rtprio(RTP_SET, proc, &rtp) != 0)
err(1, "%s", argv[0]);
--
Eitan Adler
While there, you may change the type of proc to pid_t. Also, move the initialization of proc out of local declaration section, Why is the cast needed there ? Also, I think that doing proc = strtol(); if (*invalid_char ...) ...; I suspect that the line overflows 80 characters limit. Consider wrapping The easier solution would be proc = strtol(argv[2] + 1, &invalid_char, 10); 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.
It's quite surprising how easy it is to use strtol() in an allegedly
"safe" manner, but miss some of the edge cases. We should probably check
for errno too, e.g.:
#include <errno.h>
#include <string.h>
#include <stdlib.h>
pid_t proc;
long x;
char *endp;
errno = 0;
x = strtol(argv[1], &endp, 0);
if (errno != 0 || (endp != NULL && endp != str && *endp != '\0' &&
(isdigit(*endp) == 0 || isspace(*endp) == 0)))
error();
Then if we want to avoid overflows of pid_t, we might have to check
against PID_MAX or at least INT32_MAX. The sizeof(pid_t) is __int32_t
on all FreeBSD architectures, so it may be useful to check for:
if (x >= INT32_MAX)
error();
proc = (pid_t)x;
But this is probably being too paranoid now.
_______________________________________________
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"
What about this patch? I incorporated your feedback so I am not going
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);
}
} 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]);
--
Eitan ...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");
+ }
...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;
}
Then you can replace all the atoi() calls with:
int x;
if (parseint(argv[1], NULL, 0, &x) != 0)
errx(1, "your message");
which is a relatively cleaner way of handling strtol() parsing errors,
without the associated clutter of checking properly all possible edge
cases at every call-point.
_______________________________________________
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"
instead of `return (int)x' the last bits should be slightly different,
of course:
if (value != NULL)
*value = (int)x;
return 0;
}
_______________________________________________
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"
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 ...Small nit, maybe use 'must' instead of 'shall'. -- John Baldwin _______________________________________________ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"
it seems at some point there has been a massive usage of the term 'shall' in manual pages, which people tried to get rid of. hence the 'usr/share/examples/mdoc/deshallify.sh' script. maybe this should be noted in style(9)? cheers. -- a13x _______________________________________________ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"
I know shall is used widely by opengroup when describing definitions and interfaces in the POSIX standards, but the connotation in English is very squishy, so I agree with John that must would be better. Thanks, -Garrett_______________________________________________ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"
That's a good point. I think we should change err() to errx() there. _______________________________________________ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"
kib was quick and already did it in r216967. Cheers! -Garrett_______________________________________________ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"
