Date: Sun, 29 Jan 2012 23:37:30 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Kostik Belousov <kostikbel@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Gleb Smirnoff <glebius@freebsd.org>, src-committers@freebsd.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r230583 - head/sys/kern Message-ID: <20120129223232.F925@besplex.bde.org> In-Reply-To: <20120129062327.GK2726@deviant.kiev.zoral.com.ua> References: <201201261159.q0QBxma2086162@svn.freebsd.org> <20120126233110.C960@besplex.bde.org> <20120126153641.GA68112@FreeBSD.org> <20120127194612.H1547@besplex.bde.org> <20120127091244.GZ2726@deviant.kiev.zoral.com.ua> <20120127194221.GA25723@zim.MIT.EDU> <20120128123748.GD2726@deviant.kiev.zoral.com.ua> <20120129001225.GA32220@zim.MIT.EDU> <20120129062327.GK2726@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 29 Jan 2012, Kostik Belousov wrote:
> On Sat, Jan 28, 2012 at 07:12:25PM -0500, David Schultz wrote:
>> On Sat, Jan 28, 2012, Kostik Belousov wrote:
>>> On Fri, Jan 27, 2012 at 02:42:21PM -0500, David Schultz wrote:
>>>> On Fri, Jan 27, 2012, Kostik Belousov wrote:
>>>>> On Fri, Jan 27, 2012 at 07:50:30PM +1100, Bruce Evans wrote:
>>>>>> On Thu, 26 Jan 2012, Gleb Smirnoff wrote:
>>>>>>
>>>>>>> On Thu, Jan 26, 2012 at 11:53:57PM +1100, Bruce Evans wrote:
>>>>>>> B> > @@ -1552,6 +1552,12 @@ aio_aqueue(struct thread *td, struct aio
>>>>>>> B> > return (error);
>>>>>>> B> > }
>>>>>>> B> >
>>>>>>> B> > + /* XXX: aio_nbytes is later casted to signed types. */
>>>>>>> B> > + if ((int)aiocbe->uaiocb.aio_nbytes < 0) {
>>>>>>> B>
>>>>>>> B> This should avoid implementation-defined behaviour by checking if
>>>>>>> B>
>>>>>>> B> (uncast)aiocbe->uaiocb.aio_nbytes > INT_MAX.
>>>>>>
>>>>>>> Is the attached patch okay?
>>>>>>
>>>>>> Yes. It now matches the style used for read^Wsys_read() and friends.
>>>>>> This used to have to fit the count in "int uio_resid". uio_resid now
>>>>>> has type ssize_t, but for some reason the old INT_MAX limits remain.
>>>>>
>>>>> Well, I can revive the patch. I still think it is good to get rid of
>>>>> the limit.
>>>>
>>>> The correct limit on the maximum size of a single read/write is
>>>> SSIZE_MAX, but FreeBSD uses INT_MAX. It's not safe to raise the
>>>> limit yet, though, because of bugs in several filesystems. For
>>>> example, FFS copies uio_resid into a local variable of type int.
>>>> I have some old patches that fix some of these issues for FFS and
>>>> cd9660, but surely there are more places I didn't notice.
>>>>
>>> Absolutely agree.
>>>
>>> http://people.freebsd.org/~kib/misc/uio_resid.5.patch
>>
>> Nice. You found a lot more than I've got in my tree, and you even
>> fixed the return values. There are at least a few more places to
>> fix. For instance, cd9660 and the NFS client pass uio_resid or
>> iov_len to min(), which operates on ints. (Incidentally, C11
>> generics ought to make it possible to write type-generic min()
>> and max() functions.)
So does gnu typeof(), and much more portably in practice (we're still
waiting for a C99 compiler).
> Thank you, http://people.freebsd.org/~kib/misc/uio_resid.6.patch
> changed them to MIN().
Ugh, the existence of MIN() is API breakage (similarly for MAX()):
- in FreeBSD-1 (and probably in Net/2), MIN() was min() in the kernel,
where min() was an extern function with the same semantics as the
current inline min(). This was quite broken, since MIN() is almost
type-generic, while min() forces everything to u_int. min() was
implemented in kern/subr_xxx.c. FreeBSD-1 (and maybe Net/2) also
had imin/max(), lmin/max() and ulmin/max() there.
- 4.4BSD completed removing MIN() in the kernel. It was left undefined.
This was fragile, and probably still more broken than old code that
used MIN(), since the conversions given by prototypes combined with
no warnings for dangerous conversions tend to give even more sign
extension and overflow bugs that the implicit conversions in MIN().
- MIN() remained intentionally left out in the kernel in FreeBSD-[2-4].
Some contribed code that didn't know the BSD API rolled its own MIN()
and used that. There are no less than 38 home made definitions of
MIN() in FreeBSD-4 to replace the one that is intentionally left
out :-(. Some of these are ifdefed, but since the system doesn't
have MIN(), they are always used.
- the API was broken in FreeBSD-5 by removing the ifdef that left out
MIN().
- even more code that doesn't know the old BSD API now uses MIN().
Now there are only 16 home made definitions of MIN(). Most of these
are ifdefed.
I have had (but not used) the following fairly type-generic and safe
macros for min() since FreeBSD-~2.0:
% Index: libkern.h
% ===================================================================
% RCS file: /home/ncvs/src/sys/sys/libkern.h,v
% retrieving revision 1.45
% diff -u -2 -r1.45 libkern.h
% --- libkern.h 7 Apr 2004 04:19:49 -0000 1.45
% +++ libkern.h 7 Apr 2004 11:31:02 -0000
% @@ -49,4 +51,74 @@
% #define hex2ascii(hex) (hex2ascii_data[hex])
%
% +#if 0
% +#define __max(x, y) \
% +({ \
% + __typeof(x) __x = (x); \
% + __typeof(y) __y = (y); \
% + __x > __y ? __x : __y; \
% +})
% +
% +#define __min(x, y) \
% +({ \
% + __typeof(x) __x = (x); \
% + __typeof(y) __y = (y); \
% + __x < __y ? __x : __y; \
% +})
% +#endif
Normal use of gnu typeof() to write safe type-generic macros for things
like this. This is almost verbatime from gcc.info.
% +
% +#define __max(x, y) \
% +( \
% + (sizeof(x) == 8 || sizeof(y) == 8) ? \
% + ((__typeof(x))-1 == -1 && (__typeof(y))-1 == -1) ? \
% + _qmax((x), (y)) \
% + : \
% + _uqmax((x), (y)) \
% + : \
% + ((__typeof(x))-1 == -1 && (__typeof(y))-1 == -1) ? \
% + _imax((x), (y)) \
% + : \
% + _max((x), (y)) \
% +)
% +
% +#define __min(x, y) \
% +( \
% + (sizeof(x) == 8 || sizeof(y) == 8) ? \
% + ((__typeof(x))-1 == -1 && (__typeof(y))-1 == -1) ? \
% + _qmin((x), (y)) \
% + : \
% + _uqmin((x), (y)) \
% + : \
% + ((__typeof(x))-1 == -1 && (__typeof(y))-1 == -1) ? \
% + _imin((x), (y)) \
% + : \
% + _min((x), (y)) \
% +)
Since I didn't want to use gccisms back in FreeBSD-~2.0 before use of
gccisms became ubiquitous, I tried to use typeof() just to select
the correct inline function like the programmer should. Errors were
then detected by the above picking a different one than the one used
causing the object file to change (if I was lucky, the object file
didn't change due to just the extra complexity in the above). I
committed a few fixes for type errors found by this.
% +
% +#ifdef MACRO_MAX
% +static __inline int _imax(int a, int b) { return (a > b ? a : b); }
% +static __inline int _imin(int a, int b) { return (a < b ? a : b); }
% +static __inline long lmax(long a, long b) { return (a > b ? a : b); }
% +static __inline long lmin(long a, long b) { return (a < b ? a : b); }
% +static __inline u_int _max(u_int a, u_int b) { return (a > b ? a : b); }
% +static __inline u_int _min(u_int a, u_int b) { return (a < b ? a : b); }
% +static __inline quad_t _qmax(quad_t a, quad_t b) { return (a > b ? a : b); }
% +static __inline quad_t _qmin(quad_t a, quad_t b) { return (a < b ? a : b); }
% +static __inline u_long ulmax(u_long a, u_long b) { return (a > b ? a : b); }
% +static __inline u_long ulmin(u_long a, u_long b) { return (a < b ? a : b); }
% +static __inline u_quad_t _uqmax(u_quad_t a, u_quad_t b) { return (a > b ? a : b); }
% +static __inline u_quad_t _uqmin(u_quad_t a, u_quad_t b) { return (a < b ? a : b); }
Rename the inlines so that the top level API can use either them or the
ones that do the selection.
% +
% +#define imax(a, b) __max((a), (b))
% +#define imin(a, b) __min((a), (b))
% +#define lmax(a, b) __max((a), (b))
% +#define lmin(a, b) __min((a), (b))
% +#define max(a, b) __max((a), (b))
% +#define min(a, b) __min((a), (b))
% +#define qmax(a, b) __max((a), (b))
% +#define qmin(a, b) __min((a), (b))
% +#define ulmax(a, b) __max((a), (b))
% +#define ulmin(a, b) __min((a), (b))
% +#else
But normally, don't use any of the above additions.
% +#ifdef __GNUC__
% static __inline int imax(int a, int b) { return (a > b ? a : b); }
% static __inline int imin(int a, int b) { return (a < b ? a : b); }
% @@ -63,4 +135,6 @@
% static __inline long labs(long a) { return (a < 0 ? -a : a); }
% static __inline quad_t qabs(quad_t a) { return (a < 0 ? -a : a); }
% +#endif
% +#endif
%
% /* Prototypes for non-quad routines. */
What should happen is that everyone should use a type-generic safe macro
named min() and not provide home made versions of it or MIN(), but the
namespace has been horribly messed up by using min() for the u_int function.
MIN() is not so broken, but since its name says that it is unsafe, careful
users of it should be adding extra code in some cases to avoid multiple
evaluations of its args, either for correctness or efficiency. I haven't
noticed any problems from multiple evaluations for MIN(), but I noticed
one for an mtx macro (most mtx APIs are macros, and are unsafe despite
their name, and despite them copying a parameter to a local variable in
a few places. Since they mostly return void, they can use the do-while
hack to get a block with local variables, and don't require the extra
unportability given by the gcc statement-expressions used in the above).
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120129223232.F925>