From owner-freebsd-hackers@FreeBSD.ORG Sun Jan 2 19:43:21 2011 Return-Path: Delivered-To: hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 020471065670 for ; Sun, 2 Jan 2011 19:43:21 +0000 (UTC) (envelope-from keramida@ceid.upatras.gr) Received: from igloo.linux.gr (igloo.linux.gr [62.1.205.36]) by mx1.freebsd.org (Postfix) with ESMTP id 6FD858FC08 for ; Sun, 2 Jan 2011 19:43:19 +0000 (UTC) X-Spam-Status: No X-Hellug-MailScanner-From: keramida@ceid.upatras.gr X-Hellug-MailScanner-SpamCheck: not spam, SpamAssassin (not cached, score=-2.9, required 5, autolearn=not spam, ALL_TRUSTED -1.00, BAYES_00 -1.90) X-Hellug-MailScanner: Found to be clean X-Hellug-MailScanner-ID: p02JTn78011381 Received: from gkeramidas-glaptop.linux.gr (217-162-216-74.dclient.hispeed.ch [217.162.216.74]) (authenticated bits=0) by igloo.linux.gr (8.14.3/8.14.3/Debian-9.4) with ESMTP id p02JTn78011381 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NOT); Sun, 2 Jan 2011 21:29:57 +0200 From: Giorgos Keramidas To: Kostik Belousov References: <6BEFA669-2E1F-44E6-897A-0A51DA939A74@gmail.com> <20110102101845.GC90883@deviant.kiev.zoral.com.ua> Date: Sun, 02 Jan 2011 20:29:30 +0100 In-Reply-To: <20110102101845.GC90883@deviant.kiev.zoral.com.ua> (Kostik Belousov's message of "Sun, 2 Jan 2011 12:18:45 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Cc: Eitan Adler , hackers@freebsd.org Subject: Re: [patch] have rtprio check that arguments are numeric; change atoi to strtol X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 02 Jan 2011 19:43:21 -0000 On Sun, 2 Jan 2011 12:18:45 +0200, Kostik Belousov 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 #include #include 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.