From owner-svn-src-all@freebsd.org Sat May 21 01:11:01 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 7E967B431F3; Sat, 21 May 2016 01:11:01 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 478721F70; Sat, 21 May 2016 01:11:00 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-106-149-109.carlnfd1.nsw.optusnet.com.au (c122-106-149-109.carlnfd1.nsw.optusnet.com.au [122.106.149.109]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id D5346781EA3; Sat, 21 May 2016 11:10:52 +1000 (AEST) Date: Sat, 21 May 2016 11:10:52 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Conrad Meyer cc: Konstantin Belousov , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r300332 - in head/sys: amd64/amd64 i386/i386 In-Reply-To: Message-ID: <20160521103528.I1539@besplex.bde.org> References: <201605201950.u4KJoWA5028092@repo.freebsd.org> <20160521081930.I1098@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=TuMb/2jh c=1 sm=1 tr=0 a=R/f3m204ZbWUO/0rwPSMPw==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=kXGavNC6CeC0bVARSMoA:9 a=E-ZPcJ4orlREQ8DK:21 a=cXrSxzgqgWSFdZ3D:21 a=CjuIK1q_8ugA:10 a=Oa0T6EYmKFNB-xRHvYM1:22 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 21 May 2016 01:11:01 -0000 On Fri, 20 May 2016, Conrad Meyer wrote: > On Fri, May 20, 2016 at 4:02 PM, Bruce Evans wrote: >> On Fri, 20 May 2016, Konstantin Belousov wrote: >> >>> --- head/sys/i386/i386/sys_machdep.c Fri May 20 19:46:25 2016 >>> (r300331) >>> +++ head/sys/i386/i386/sys_machdep.c Fri May 20 19:50:32 2016 >>> (r300332) >>> @@ -315,8 +315,9 @@ i386_set_ioperm(td, uap) >>> struct thread *td; >>> struct i386_ioperm_args *uap; >>> { >>> - int i, error; >>> char *iomap; >>> + u_int i; >>> + int error; >>> >>> if ((error = priv_check(td, PRIV_IO)) != 0) >>> return (error); >>> @@ -334,7 +335,8 @@ i386_set_ioperm(td, uap) >>> return (error); >>> iomap = (char *)td->td_pcb->pcb_ext->ext_iomap; >>> >>> - if (uap->start + uap->length > IOPAGES * PAGE_SIZE * NBBY) >>> + if (uap->start > uap->start + uap->length || >>> + uap->start + uap->length > IOPAGES * PAGE_SIZE * NBBY) >>> return (EINVAL); >>> >>> for (i = uap->start; i < uap->start + uap->length; i++) { >> >> I don't like using u_int for a small index. > > Why not? Indices are by definition non-negative so the fit seems natural. Signed integers are easier to understand provided calculations with them don't overflow. Unsigned integers are not easier to understand if calculations with them do overflow. That was the case here. Only indices relative to the base of an array are by definition non-negative. For an array a[], it is valid to do p = &a[i] and then use p[j] with negative j to get back before the i'th index. This is sometimes useful. i + j must be >= 0, but is hard write correctly and understand if either i or j is unsigned. (It can be arranged that the addition wraps correctly, but this is basically re-implementing signed arithmetic.) >> After the bounds checking >> fix, the range fits in a small signed integer. However, uap->start >> and uap->length already use bad type u_int, so it is natural to keep >> using that type. > > What's bad about it? Unsigned in the API gives unsigned poisoning when the API is used. The API would have needed unsigneds to cover the i386 i/o range of 64K using 16-bit ints, but since we never supported 16-bit ints and POSI now requires 32-bit ints, we shouldn't have the complications from that. Bruce