Date: Sat, 21 May 2016 11:10:52 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Conrad Meyer <cem@freebsd.org> Cc: Konstantin Belousov <kib@freebsd.org>, 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 Message-ID: <20160521103528.I1539@besplex.bde.org> In-Reply-To: <CAG6CVpUtz49L0VWfPcCXFvEMiV41AwxhJ8tGjenLqgPJt_kpyA@mail.gmail.com> References: <201605201950.u4KJoWA5028092@repo.freebsd.org> <20160521081930.I1098@besplex.bde.org> <CAG6CVpUtz49L0VWfPcCXFvEMiV41AwxhJ8tGjenLqgPJt_kpyA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 20 May 2016, Conrad Meyer wrote:
> On Fri, May 20, 2016 at 4:02 PM, Bruce Evans <brde@optusnet.com.au> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160521103528.I1539>