Date: Wed, 30 Jan 2002 20:48:00 +0100 From: Thomas Moestl <tmoestl@gmx.net> To: hackers@FreeBSD.org, jhb@FreeBSD.org Subject: Re: uiomove busted for the past 3 years Message-ID: <20020130194800.GB324@crow.dom2ip.de> In-Reply-To: <Pine.BSF.4.21.0201300655270.85159-100000@toxic.magnesium.net> References: <Pine.BSF.4.21.0201300655270.85159-100000@toxic.magnesium.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 2002/01/30 at 07:02:25 -0800, User JHB wrote:
> When the DEADLKTREAT flag was added, uiomove() was broken. :) The
> problem is that a return() inside of a switch nested inside of a while
> loop was converted to a break leading to the following rather silly code:
>
> if (error)
> break;
> break;
>
> What is supposed to happen is that if there is an error, we break out of
> the while loop, but all we do is break out of the switch ignoring the
> error and continuing to loop. Thus, in the best case, if copyin or
> copyout failed on the last iteration of the loop, we would return an
> error, but would bogusly update the counts in the iovec and uio
> structures. In the worst case, if we kept looping and later copyin's or
> copyout's succeeded, then we wouldn't return an error at all. A quick fix
> would be to add a goto to jump to the error return code at the end of the
> loop rather than the bogus break if (error). However, I can't test this
> at the moment. Someone please verify and fix this.
The attached test program triggers this bug (if the test file is
located on a file system that does not split up the uiomove() in that
case, e.g. FFS with a block size >= 2 * page size). When NIOV is changed
from 2 to 1, the writev() will result in EFAULT, however with two
iovecs the write returns the sum of the sizes of both iovecs although
the buffer supplied to the first is unaccessible.
The patch you proposed:
--- kern_subr.c~ Fri Jan 25 02:14:45 2002
+++ kern_subr.c Wed Jan 30 20:39:07 2002
@@ -104,7 +104,7 @@
else
error = copyin(iov->iov_base, cp,
cnt);
if (error)
- break;
+ goto out;
break;
case UIO_SYSSPACE:
@@ -123,6 +123,7 @@
cp += cnt;
n -= cnt;
}
+out:
if (td != curthread) printf("uiomove: IT CHANGED!");
td = curthread; /* Might things have changed in
copyin/copyout? */
if (td) {
--
fixes it for me.
- thomas
#include <sys/types.h>
#include <sys/fcntl.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <sys/uio.h>
#include <err.h>
#include <errno.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#define NIOV 2
int
main()
{
struct iovec iov[2];
size_t psz;
char *a, *b;
int fd, rv, en;
psz = getpagesize();
a = mmap(NULL, psz, PROT_NONE, MAP_ANON, -1, 0);
if (a == MAP_FAILED)
err(1, "mmap 1");
b = mmap(a + psz, psz, PROT_READ, MAP_ANON | MAP_FIXED, -1, 0);
if (b == MAP_FAILED)
err(1, "mmap 2");
fd = open("uiob.foo", O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
if (fd == -1)
err(1, "open");
iov[0].iov_base = a;
iov[0].iov_len = psz;
iov[1].iov_base = b;
iov[1].iov_len = psz;
rv = writev(fd, iov, NIOV);
en = errno;
printf("writev returned %d\n", rv);
if (rv == -1)
printf("error: %s\n", strerror(en));
close(fd);
return (0);
}
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-hackers" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020130194800.GB324>