On Sun, 2 Jan 2011 12:18:45 +0200, Kostik Belousov <kostikbel@gmail.com> wrote:
quoted text
> On Sun, Jan 02, 2011 at 02:41:10AM -0500, Eitan Adler wrote:
>> > Just set the second argument to strtol to something non-NULL and then check
>> > the value returned; that will help provide the error handling with
>> > simplicity that you desire :).
>>
>> 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;
> While there, you may change the type of proc to pid_t.
> Also, move the initialization of proc out of local declaration section,
> according to style(9).
>
>> +       char *invalidChar;
> We do not use camelCase, according to style(9).
>
>>
>>        /* 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));
>
> Why is the cast needed there ?
> Also, I think that doing
> 	proc = strtol();
> 	if (*invalid_char ...)
> 		...;
> 	proc = abs(proc);

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"