Implementation defined behaviour in read_write.c

[Rainer Weikusat]

The following code is in the function do_readv_writev in the file
fs/read_write.c (2.6.8.1):


	/*
	 * Single unix specification:
	 * We should -EINVAL if an element length is not >= 0 and fitting an
	 * ssize_t.  The total length is fitting an ssize_t
	 *
	 * Be careful here because iov_len is a size_t not an ssize_t
	 */
	tot_len = 0;
	ret = -EINVAL;
	for (seg = 0; seg < nr_segs; seg++) {
		ssize_t len = (ssize_t)iov[seg].iov_len;

		if (len < 0)	/* size_t not fitting an ssize_t .. */
			goto out;
		tot_len += len;
		if ((ssize_t)tot_len < 0) /* maths overflow on the ssize_t */
			goto out;
	}
	if (tot_len == 0) {
		ret = 0;
		goto out;
	}

There is a potential problem here, namely, that the result of the
conversion to ssize_t is implementation defined (ISO-C
6.3.1.3|3). This is not a problem with current gcc versions, but might
become a future one, because len need not be below zero after
attempting to convert a value too large for a ssize_t to that type. The
following patch would get rid off this (and corrects to errors in the
comment text ;-).

--- read_write.c.orig	2004-09-20 22:26:43.000000000 +0800
+++ read_write.c	2004-09-20 23:45:51.000000000 +0800
@@ -386,10 +386,10 @@
 	typedef ssize_t (*io_fn_t)(struct file *, char __user *, size_t, loff_t *);
 	typedef ssize_t (*iov_fn_t)(struct file *, const struct iovec *, unsigned long, loff_t *);
 
-	size_t tot_len;
+	size_t tot_len, cur_len;
 	struct iovec iovstack[UIO_FASTIOV];
 	struct iovec *iov=iovstack, *vector;
-	ssize_t ret;
+	ssize_t ret, x;
 	int seg;
 	io_fn_t fn;
 	iov_fn_t fnv;
@@ -425,22 +425,32 @@
 
 	/*
 	 * Single unix specification:
-	 * We should -EINVAL if an element length is not >= 0 and fitting an
-	 * ssize_t.  The total length is fitting an ssize_t
+	 * We should -EINVAL if an element length is not >= 0 and fitting a
+	 * ssize_t.  The total length is fitting a ssize_t
 	 *
-	 * Be careful here because iov_len is a size_t not an ssize_t
+	 * Be careful here because iov_len is a size_t not an ssize_t.
 	 */
 	tot_len = 0;
 	ret = -EINVAL;
 	for (seg = 0; seg < nr_segs; seg++) {
-		ssize_t len = (ssize_t)iov[seg].iov_len;
-
-		if (len < 0)	/* size_t not fitting an ssize_t .. */
-			goto out;
-		tot_len += len;
-		if ((ssize_t)tot_len < 0) /* maths overflow on the ssize_t */
+		cur_len = iov[seg].iov_len;
+		/* guard against overflows during the addition */
+		if (cur_len + tot_len < tot_len) goto out;
+		tot_len += cur_len;
 	}
+
+	/*
+	  This is as ISO-compliant as possible.
+	  If the value in tot_len changes when
+	  converted to a ssize_t and back,
+	  something 'implementation defined' happened
+	  because of an overflow.
+	*/
+	x = tot_len;
+	cur_len = x;
+	if (tot_len != cur_len) goto out;
+	
 	if (tot_len == 0) {
 		ret = 0;
 		goto out;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/