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> wr=
ote:
> 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
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> --- rtprio.c =A0 =A0(revision 216679)
>> +++ rtprio.c =A0 =A0(working copy)
>> @@ -56,6 +56,7 @@
>> =A0 =A0 =A0 =A0char =A0 *p;
>> =A0 =A0 =A0 =A0int =A0 =A0 proc =3D 0;
>> =A0 =A0 =A0 =A0struct 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).
>
>> + =A0 =A0 =A0 char *invalidChar;
> We do not use camelCase, according to style(9).
>
>>
>> =A0 =A0 =A0 =A0/* find basename */
>> =A0 =A0 =A0 =A0if ((p =3D rindex(argv[0], '/')) =3D=3D NULL)
>> @@ -70,8 +71,9 @@
>>
>> =A0 =A0 =A0 =A0switch (argc) {
>> =A0 =A0 =A0 =A0case 2:
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 proc =3D abs(atoi(argv[1])); =A0 =A0 =A0/*=
Should check if numeric
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0* arg! */
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 proc =3D abs((int)strtol(argv[1], &invalid=
Char, 10));
>
> Why is the cast needed there ?
> Also, I think that doing
> proc =3D strtol();
> if (*invalid_char ...)
> ...;
> proc =3D 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 =3D 0;
x =3D strtol(argv[1], &endp, 0);
if (errno !=3D 0 || (endp !=3D NULL && endp !=3D str && *endp !=3D '\0'=
&&
(isdigit(*endp) =3D=3D 0 || isspace(*endp) =3D=3D 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 >=3D INT32_MAX)
error();
proc =3D (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>