Date: Sun, 02 Jan 2011 20:29:30 +0100 From: Giorgos Keramidas <keramida@ceid.upatras.gr> To: Kostik Belousov <kostikbel@gmail.com> Cc: Eitan Adler <lists@eitanadler.com>, hackers@freebsd.org Subject: Re: [patch] have rtprio check that arguments are numeric; change atoi to strtol Message-ID: <xeiaipy7f6jp.fsf@kobe.laptop> In-Reply-To: <20110102101845.GC90883@deviant.kiev.zoral.com.ua> (Kostik Belousov's message of "Sun, 2 Jan 2011 12:18:45 %2B0200") References: <AANLkTimiJPiHBSw5i5TVJYfh9uGOyrNJx%2BoUPeB%2Bt%2BY_@mail.gmail.com> <6BEFA669-2E1F-44E6-897A-0A51DA939A74@gmail.com> <AANLkTimmQHRoQOZBgfheds=_sv5SMzD9DrPfYk9em7j8@mail.gmail.com> <AANLkTimdEpZ1DBoKU9g6w-j=F9KS0ONvo9racgOesdPf@mail.gmail.com> <20110102101845.GC90883@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 2 Jan 2011 12:18:45 +0200, Kostik Belousov <kostikbel@gmail.com> wrote:
> 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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?xeiaipy7f6jp.fsf>