Re: svn commit: r230583 - head/sys/kern
On Tue, Jan 31, 2012 at 12:48:49PM -0500, David Schultz wrote:
On Tue, Jan 31, 2012, Konstantin Belousov wrote:
On Mon, Jan 30, 2012 at 02:07:03PM -0500, David Schultz wrote:
On Mon, Jan 30, 2012, Kostik Belousov wrote:
On Sun, Jan 29, 2012 at 05:39:04PM -0500, David Schultz wrote:
On Sun, Jan 29, 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:
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.)
Thank you, http://people.freebsd.org/~kib/misc/uio_resid.6.patch
changed them to MIN().
This looks good to me. I tried to think of other places that you
might have missed, and the only one that occurred to me is the
Might ? I think this is a blatant understate.
pipe code. sys_pipe.c has an `int orig_resid' and lots of bogus
casts of iov_len and uio_resid to type u_int. Some look harmless,
although it appears that writing a multiple of 2^32 bytes might
result in pipe_build_write_buffer() allocating a 0-length buffer.
My only reservation is that raising the limit could unmask a
kernel buffer overflow if we missed something, but I guess we have
to cross that bridge some day anyway.
Yes, and it is an obvious reason why I am chicken to commit this for
so long time. One more place, if this is reasonable to count as 'one'
place, are the cdevsw methods. devfs passes uio down to the drivers.
That's why I'm glad I'm not committing it. :) A more conservative
change (also known as kicking the can down the road) would be to
add a VFS flag, e.g., VFCF_LONGIO, and only set it on file systems
that have been thoroughly reviewed. The VFS layer could cap the size
at INT_MAX for file systems without the flag.
At least I will get more mail after the commit, I hope.
I disagree with the VFCF_LONGIO approach. It will cause much head-scratching
for unsuspecting user who would try to use 4GB transfers.
What I can do, is to commit all changes except removals of the checks
for INT_MAX. After type changes settle, I can try to gather enough
bravery to flip the checks in HEAD, possibly with temporary sysctl
to return to old behaviour for emergency (AKA hole).
That sounds like a good plan to me.
As an aside, I wonder if we could convince the clang folks to add
a warning similar to `lint -a', which complains about every long-int
narrowing conversion that doesn't have an explicit cast. Modern
languages such as Java and C# require casts for narrowing
conversions, and I'd be a lot more confident about this change if
we did the same for the FreeBSD kernel.
diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c
index 9edcb74..332ec37 100644
--- a/sys/kern/sys_pipe.c
+++ b/sys/kern/sys_pipe.c
[...]
@@ -757,14 +757,14 @@ pipe_build_write_buffer(wpipe, uio)
struct pipe *wpipe;
struct uio *uio;
{
- u_int size;
+ size_t size;
int i;
PIPE_LOCK_ASSERT(wpipe, MA_NOTOWNED);
KASSERT(wpipe-pipe_state PIPE_DIRECTW,
(Clone attempt on non-direct write
pipe!));
- size = (u_int) uio-uio_iov-iov_len;
+ size = uio-uio_iov-iov_len;
if (size wpipe-pipe_buffer.size)
size = wpipe-pipe_buffer.size;
The transfer can't be bigger than the max pipe buffer size (64k),
so `size = (int)MIN(uio-uio_iov-iov_len, wpipe-pipe_buffer.size)'
should suffice. The same comment applies elsewhere in the file.
True. If you much prefer this version, I will change the patch. But I do
think that my changes are cleaner.
I don't mind either way. I haven't touched anything remotely
close to that code in years.
I did the changes along the way suggested by Bruce.
Also, I put the patch under the real test for UFS and new
Re: svn commit: r230583 - head/sys/kern
On Mon, Jan 30, 2012 at 02:07:03PM -0500, David Schultz wrote:
On Mon, Jan 30, 2012, Kostik Belousov wrote:
On Sun, Jan 29, 2012 at 05:39:04PM -0500, David Schultz wrote:
On Sun, Jan 29, 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:
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.)
Thank you, http://people.freebsd.org/~kib/misc/uio_resid.6.patch
changed them to MIN().
This looks good to me. I tried to think of other places that you
might have missed, and the only one that occurred to me is the
Might ? I think this is a blatant understate.
pipe code. sys_pipe.c has an `int orig_resid' and lots of bogus
casts of iov_len and uio_resid to type u_int. Some look harmless,
although it appears that writing a multiple of 2^32 bytes might
result in pipe_build_write_buffer() allocating a 0-length buffer.
My only reservation is that raising the limit could unmask a
kernel buffer overflow if we missed something, but I guess we have
to cross that bridge some day anyway.
Yes, and it is an obvious reason why I am chicken to commit this for
so long time. One more place, if this is reasonable to count as 'one'
place, are the cdevsw methods. devfs passes uio down to the drivers.
That's why I'm glad I'm not committing it. :) A more conservative
change (also known as kicking the can down the road) would be to
add a VFS flag, e.g., VFCF_LONGIO, and only set it on file systems
that have been thoroughly reviewed. The VFS layer could cap the size
at INT_MAX for file systems without the flag.
At least I will get more mail after the commit, I hope.
I disagree with the VFCF_LONGIO approach. It will cause much head-scratching
for unsuspecting user who would try to use 4GB transfers.
What I can do, is to commit all changes except removals of the checks
for INT_MAX. After type changes settle, I can try to gather enough
bravery to flip the checks in HEAD, possibly with temporary sysctl
to return to old behaviour for emergency (AKA hole).
diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c
index 9edcb74..332ec37 100644
--- a/sys/kern/sys_pipe.c
+++ b/sys/kern/sys_pipe.c
[...]
@@ -757,14 +757,14 @@ pipe_build_write_buffer(wpipe, uio)
struct pipe *wpipe;
struct uio *uio;
{
- u_int size;
+ size_t size;
int i;
PIPE_LOCK_ASSERT(wpipe, MA_NOTOWNED);
KASSERT(wpipe-pipe_state PIPE_DIRECTW,
(Clone attempt on non-direct write pipe!));
- size = (u_int) uio-uio_iov-iov_len;
+ size = uio-uio_iov-iov_len;
if (size wpipe-pipe_buffer.size)
size = wpipe-pipe_buffer.size;
The transfer can't be bigger than the max pipe buffer size (64k),
so `size = (int)MIN(uio-uio_iov-iov_len, wpipe-pipe_buffer.size)'
should suffice. The same comment applies elsewhere in the file.
True. If you much prefer this version, I will change the patch. But I do
think that my changes are cleaner.
pgpfSMIx7snmI.pgp
Description: PGP signature
Re: svn commit: r230583 - head/sys/kern
On Tue, 31 Jan 2012, Konstantin Belousov wrote:
On Mon, Jan 30, 2012 at 02:07:03PM -0500, David Schultz wrote:
That's why I'm glad I'm not committing it. :) A more conservative
change (also known as kicking the can down the road) would be to
add a VFS flag, e.g., VFCF_LONGIO, and only set it on file systems
that have been thoroughly reviewed. The VFS layer could cap the size
at INT_MAX for file systems without the flag.
At least I will get more mail after the commit, I hope.
I disagree with the VFCF_LONGIO approach. It will cause much head-scratching
for unsuspecting user who would try to use 4GB transfers.
No one cared when this was broken for almost 10 years by 4.4BSD changing
the type of iov_len from int to size_t. You can almost equally safely
break it again :(. Previously (in FreeBSD-1 and presumably in Net/2)
iov_len had the same type as uio_resid, and the code in readv(), etc.,
depended on this and also on benign undefined behaviour on overflow
to detect overflow (after it gave undefined behaviour by occurring)
when adding up iov_len's to get uio_resid. The bug didn't require
passing a buffer of size INT_MAX to reach -- it just required a
single or multiple iov_len whose total size exceeded INT_MAX. FreeBSD
fixed this in 2003.
size_t for iov_len is bogus even if the limit is SSIZE max. It allows
a single iov to have a size that cannot work, and n iov's to have a
size almost 2*n times too large to work. Unfortunately, POSIX
standardizes the type of iov_len as size_t. It is interesting that
POSIX says shall fail for readv() when the sum of the iov_len values
overflowed [sic] an ssize_t. For read(), the behaviour when the count
exceeds {SSIZE_MAX} is implementation-defined. This gives the silly
possibility that read() can work for a size that is almost twice as
large as readv(), using a single count instead of an array of counts.
It is also strange that readv() refers to ssize_t while read() refers
to {SSIZE_MAX}. POSIX standardizes the bug that {SSIZE_MAX} is the
limit of the type, although that may be unrelated to sizes that can
work. If these APIs had been correctly designed, then the limit would
be {READ_MAX} and unrelated to any size type (except that the type
must be large enough to represent {READ_MAX}). It would normally be
INT_MAX or much smaller. readv() also has the bogus specification
that if iovcnt is = 0 or {IOV_MAX}, then failure is optional.
So failure is optional for a hard error like iovcnt {IOV_MAX},
but is required for a condition that is not required to be an error
for a similar API (count {SSIZE_MAX}). I only checked a 2001
draft for this. Maybe some of these bugs have been fixed.
What I can do, is to commit all changes except removals of the checks
for INT_MAX. After type changes settle, I can try to gather enough
bravery to flip the checks in HEAD, possibly with temporary sysctl
to return to old behaviour for emergency (AKA hole).
diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c
index 9edcb74..332ec37 100644
--- a/sys/kern/sys_pipe.c
+++ b/sys/kern/sys_pipe.c
[...]
@@ -757,14 +757,14 @@ pipe_build_write_buffer(wpipe, uio)
struct pipe *wpipe;
struct uio *uio;
{
- u_int size;
+ size_t size;
int i;
PIPE_LOCK_ASSERT(wpipe, MA_NOTOWNED);
KASSERT(wpipe-pipe_state PIPE_DIRECTW,
(Clone attempt on non-direct write pipe!));
- size = (u_int) uio-uio_iov-iov_len;
+ size = uio-uio_iov-iov_len;
if (size wpipe-pipe_buffer.size)
size = wpipe-pipe_buffer.size;
The transfer can't be bigger than the max pipe buffer size (64k),
so `size = (int)MIN(uio-uio_iov-iov_len, wpipe-pipe_buffer.size)'
should suffice. The same comment applies elsewhere in the file.
True. If you much prefer this version, I will change the patch. But I do
think that my changes are cleaner.
size_t is a bogus type for `size', since it must still be a u_int to
work. For example, when it is assigned to the u_int wpipe-pipe_map.cnt.
This shouldn't be fixed by bloating the type to size_t throughout
(any global change should be to int to give suitably undefined behaviour
on overflow). There must be an implicit or explicit check on it
somewhere that it fits. This check is provided by the comparison with
wpipe-pipe_buffer.size. But the above is a bad way to write it --
it should be checked before assignment, to avoid any possibility of
overflow or sign extension of either operand before the check:
u_int size;
if (uio-uio_iov-iov_len wpipe-pipe_buffer.size)
size = wpipe-pipe_buffer.size;
else
size = uio-uio_iov-iov_len;
This is the same as the MIN() expression (without a bogus cast). I
think I want to write it out verbosely like this and not use MIN(),
even if MIN() were not a style bug, to make it clear that this is
a correct bounds check.
Bruce
___
Re: svn commit: r230583 - head/sys/kern
On Tue, Jan 31, 2012, Konstantin Belousov wrote:
On Mon, Jan 30, 2012 at 02:07:03PM -0500, David Schultz wrote:
On Mon, Jan 30, 2012, Kostik Belousov wrote:
On Sun, Jan 29, 2012 at 05:39:04PM -0500, David Schultz wrote:
On Sun, Jan 29, 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:
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.)
Thank you, http://people.freebsd.org/~kib/misc/uio_resid.6.patch
changed them to MIN().
This looks good to me. I tried to think of other places that you
might have missed, and the only one that occurred to me is the
Might ? I think this is a blatant understate.
pipe code. sys_pipe.c has an `int orig_resid' and lots of bogus
casts of iov_len and uio_resid to type u_int. Some look harmless,
although it appears that writing a multiple of 2^32 bytes might
result in pipe_build_write_buffer() allocating a 0-length buffer.
My only reservation is that raising the limit could unmask a
kernel buffer overflow if we missed something, but I guess we have
to cross that bridge some day anyway.
Yes, and it is an obvious reason why I am chicken to commit this for
so long time. One more place, if this is reasonable to count as 'one'
place, are the cdevsw methods. devfs passes uio down to the drivers.
That's why I'm glad I'm not committing it. :) A more conservative
change (also known as kicking the can down the road) would be to
add a VFS flag, e.g., VFCF_LONGIO, and only set it on file systems
that have been thoroughly reviewed. The VFS layer could cap the size
at INT_MAX for file systems without the flag.
At least I will get more mail after the commit, I hope.
I disagree with the VFCF_LONGIO approach. It will cause much head-scratching
for unsuspecting user who would try to use 4GB transfers.
What I can do, is to commit all changes except removals of the checks
for INT_MAX. After type changes settle, I can try to gather enough
bravery to flip the checks in HEAD, possibly with temporary sysctl
to return to old behaviour for emergency (AKA hole).
That sounds like a good plan to me.
As an aside, I wonder if we could convince the clang folks to add
a warning similar to `lint -a', which complains about every long-int
narrowing conversion that doesn't have an explicit cast. Modern
languages such as Java and C# require casts for narrowing
conversions, and I'd be a lot more confident about this change if
we did the same for the FreeBSD kernel.
diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c
index 9edcb74..332ec37 100644
--- a/sys/kern/sys_pipe.c
+++ b/sys/kern/sys_pipe.c
[...]
@@ -757,14 +757,14 @@ pipe_build_write_buffer(wpipe, uio)
struct pipe *wpipe;
struct uio *uio;
{
- u_int size;
+ size_t size;
int i;
PIPE_LOCK_ASSERT(wpipe, MA_NOTOWNED);
KASSERT(wpipe-pipe_state PIPE_DIRECTW,
(Clone attempt on non-direct write pipe!));
- size = (u_int) uio-uio_iov-iov_len;
+ size = uio-uio_iov-iov_len;
if (size wpipe-pipe_buffer.size)
size = wpipe-pipe_buffer.size;
The transfer can't be bigger than the max pipe buffer size (64k),
so `size = (int)MIN(uio-uio_iov-iov_len, wpipe-pipe_buffer.size)'
should suffice. The same comment applies elsewhere in the file.
True. If you much prefer this version, I will change the patch. But I do
think that my changes are cleaner.
I don't mind either way. I haven't touched anything remotely
close to that code in years.
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r230583 - head/sys/kern
On Mon, Jan 30, 2012, Kostik Belousov wrote:
On Sun, Jan 29, 2012 at 05:39:04PM -0500, David Schultz wrote:
On Sun, Jan 29, 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:
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.)
Thank you, http://people.freebsd.org/~kib/misc/uio_resid.6.patch
changed them to MIN().
This looks good to me. I tried to think of other places that you
might have missed, and the only one that occurred to me is the
Might ? I think this is a blatant understate.
pipe code. sys_pipe.c has an `int orig_resid' and lots of bogus
casts of iov_len and uio_resid to type u_int. Some look harmless,
although it appears that writing a multiple of 2^32 bytes might
result in pipe_build_write_buffer() allocating a 0-length buffer.
My only reservation is that raising the limit could unmask a
kernel buffer overflow if we missed something, but I guess we have
to cross that bridge some day anyway.
Yes, and it is an obvious reason why I am chicken to commit this for
so long time. One more place, if this is reasonable to count as 'one'
place, are the cdevsw methods. devfs passes uio down to the drivers.
That's why I'm glad I'm not committing it. :) A more conservative
change (also known as kicking the can down the road) would be to
add a VFS flag, e.g., VFCF_LONGIO, and only set it on file systems
that have been thoroughly reviewed. The VFS layer could cap the size
at INT_MAX for file systems without the flag.
diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c
index 9edcb74..332ec37 100644
--- a/sys/kern/sys_pipe.c
+++ b/sys/kern/sys_pipe.c
[...]
@@ -757,14 +757,14 @@ pipe_build_write_buffer(wpipe, uio)
struct pipe *wpipe;
struct uio *uio;
{
- u_int size;
+ size_t size;
int i;
PIPE_LOCK_ASSERT(wpipe, MA_NOTOWNED);
KASSERT(wpipe-pipe_state PIPE_DIRECTW,
(Clone attempt on non-direct write pipe!));
- size = (u_int) uio-uio_iov-iov_len;
+ size = uio-uio_iov-iov_len;
if (size wpipe-pipe_buffer.size)
size = wpipe-pipe_buffer.size;
The transfer can't be bigger than the max pipe buffer size (64k),
so `size = (int)MIN(uio-uio_iov-iov_len, wpipe-pipe_buffer.size)'
should suffice. The same comment applies elsewhere in the file.
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r230583 - head/sys/kern
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 - 1.45
% +++ libkern.h 7 Apr 2004 11:31:02 -
% @@ -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))
Re: svn commit: r230583 - head/sys/kern
On Sun, Jan 29, 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
Breturn (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.)
Thank you, http://people.freebsd.org/~kib/misc/uio_resid.6.patch
changed them to MIN().
This looks good to me. I tried to think of other places that you
might have missed, and the only one that occurred to me is the
pipe code. sys_pipe.c has an `int orig_resid' and lots of bogus
casts of iov_len and uio_resid to type u_int. Some look harmless,
although it appears that writing a multiple of 2^32 bytes might
result in pipe_build_write_buffer() allocating a 0-length buffer.
My only reservation is that raising the limit could unmask a
kernel buffer overflow if we missed something, but I guess we have
to cross that bridge some day anyway.
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r230583 - head/sys/kern
On Sun, Jan 29, 2012 at 05:39:04PM -0500, David Schultz wrote:
On Sun, Jan 29, 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.)
Thank you, http://people.freebsd.org/~kib/misc/uio_resid.6.patch
changed them to MIN().
This looks good to me. I tried to think of other places that you
might have missed, and the only one that occurred to me is the
Might ? I think this is a blatant understate.
pipe code. sys_pipe.c has an `int orig_resid' and lots of bogus
casts of iov_len and uio_resid to type u_int. Some look harmless,
although it appears that writing a multiple of 2^32 bytes might
result in pipe_build_write_buffer() allocating a 0-length buffer.
My only reservation is that raising the limit could unmask a
kernel buffer overflow if we missed something, but I guess we have
to cross that bridge some day anyway.
Yes, and it is an obvious reason why I am chicken to commit this for
so long time. One more place, if this is reasonable to count as 'one'
place, are the cdevsw methods. devfs passes uio down to the drivers.
http://people.freebsd.org/~kib/misc/uio_resid.7.patch
contains the sys_pipe.c changes.
pgpOSaHOEp5Rh.pgp
Description: PGP signature
Re: svn commit: r230583 - head/sys/kern
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
By the way, PR 147226 is about this.
pgpcFsmpLEZVe.pgp
Description: PGP signature
Re: svn commit: r230583 - head/sys/kern
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
Breturn (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.)
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r230583 - head/sys/kern
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.
Bruce
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r230583 - head/sys/kern
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.
pgpl04w0rAyFf.pgp
Description: PGP signature
Re: svn commit: r230583 - head/sys/kern
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
Breturn (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.
By the way, PR 147226 is about this.
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r230583 - head/sys/kern
On Thu, 26 Jan 2012, Gleb Smirnoff wrote:
Log:
Although aio_nbytes is size_t, later is is signed to
casted types: to ssize_t in filesystem code and to
int in buf code, thus supplying a negative argument
leads to kernel panic later.
And supplying a large positive argument leads to undefined
behaviour later in the buf case.
To fix that check user
supplied argument in the beginning of syscall.
Now, supplying a large positive argument gives implementation-
defined behaviour immediately. Sometimes the implementation-
defined behaviour is to have no effect immediately and allow
a panic later.
Modified: head/sys/kern/vfs_aio.c
==
--- head/sys/kern/vfs_aio.c Thu Jan 26 11:15:12 2012(r230582)
+++ head/sys/kern/vfs_aio.c Thu Jan 26 11:59:48 2012(r230583)
@@ -1552,6 +1552,12 @@ aio_aqueue(struct thread *td, struct aio
return (error);
}
+ /* XXX: aio_nbytes is later casted to signed types. */
+ if ((int)aiocbe-uaiocb.aio_nbytes 0) {
This should avoid implementation-defined behaviour by checking if
(uncast)aiocbe-uaiocb.aio_nbytes INT_MAX.
It isn't clear what the effects of the implementation-defined behaviour
are even when you know what it is. It certainly results in the
check passing values that exceed INT_MAX. For example, with 32-bit
int and 64-bit size_t, and everything 2's complement, and no perverse
implementation-defined behaviour like the result of converting to
int when the argment exceeds INT_MAX is always 42, then
(int)0x1000 is 0.
+ uma_zfree(aiocb_zone, aiocbe);
+ return (EINVAL);
+ }
+
if (aiocbe-uaiocb.aio_sigevent.sigev_notify != SIGEV_KEVENT
aiocbe-uaiocb.aio_sigevent.sigev_notify != SIGEV_SIGNAL
aiocbe-uaiocb.aio_sigevent.sigev_notify != SIGEV_THREAD_ID
Bruce
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r230583 - head/sys/kern
Bruce,
On Thu, Jan 26, 2012 at 11:53:57PM +1100, Bruce Evans wrote:
B Log:
B Although aio_nbytes is size_t, later is is signed to
B casted types: to ssize_t in filesystem code and to
B int in buf code, thus supplying a negative argument
B leads to kernel panic later.
B
B And supplying a large positive argument leads to undefined
B behaviour later in the buf case.
B
B To fix that check user
B supplied argument in the beginning of syscall.
B
B Now, supplying a large positive argument gives implementation-
B defined behaviour immediately. Sometimes the implementation-
B defined behaviour is to have no effect immediately and allow
B a panic later.
B
B Modified: head/sys/kern/vfs_aio.c
B
==
B --- head/sys/kern/vfs_aio.cThu Jan 26 11:15:12 2012
(r230582)
B +++ head/sys/kern/vfs_aio.cThu Jan 26 11:59:48 2012
(r230583)
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.
B
B It isn't clear what the effects of the implementation-defined behaviour
B are even when you know what it is. It certainly results in the
B check passing values that exceed INT_MAX. For example, with 32-bit
B int and 64-bit size_t, and everything 2's complement, and no perverse
B implementation-defined behaviour like the result of converting to
B int when the argment exceeds INT_MAX is always 42, then
B (int)0x1000 is 0.
Is the attached patch okay?
--
Totus tuus, Glebius.
Index: vfs_aio.c
===
--- vfs_aio.c (revision 230585)
+++ vfs_aio.c (working copy)
@@ -1553,7 +1553,7 @@
}
/* XXX: aio_nbytes is later casted to signed types. */
- if ((int)aiocbe-uaiocb.aio_nbytes 0) {
+ if (aiocbe-uaiocb.aio_nbytes INT_MAX) {
uma_zfree(aiocb_zone, aiocbe);
return (EINVAL);
}
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
