[PATCH 1/2]segmentation fault in xfs_io mread/mwrite command

[PATCH 1/2]segmentation fault in xfs_io mread/mwrite command

[Utako Kusaka]

Hi,

I found the following issues in xfs_io.
 mread command:
  a) Causes a segmentation fault.
     Because "length"+1 bytes data is copied to buffer in read_mapping(),
     but buffer size is "length".
  b) Reads from wrong offset.
  c) The first byte of dump data is incorrect when length > page size.
 mwrite command:
  d) Data placement is incorrect when -r option is specified
     because of wrong for-loop counter.

This patch fixes them.

Signed-off-by: Utako Kusaka <utako@tnes.nec.co.jp>
---

--- xfsprogs-2.8.11-orgn/io/mmap.c	2006-06-26 14:01:15.000000000 +0900
+++ xfsprogs-2.8.11/io/mmap.c	2006-11-14 16:46:18.651458839 +0900
@@ -323,26 +323,6 @@ msync_f(
 	return 0;
 }
 
-static int
-read_mapping(
-	char		*dest,
-	off64_t		offset,
-	int		dump,
-	off64_t		dumpoffset,
-	size_t		dumplength)
-{
-	*dest = *(((char *)mapping->addr) + offset);
-
-	if (offset % pagesize == 0) {
-		if (dump == 2)
-			dumpoffset += mapping->offset;
-		if (dump)
-			dump_buffer(dumpoffset, dumplength);
-		return 1;
-	}
-	return 0;
-}
-
 static void
 mread_help(void)
 {
@@ -373,9 +353,9 @@ mread_f(
 	int		argc,
 	char		**argv)
 {
-	off64_t		offset, tmp;
+	off64_t		offset, tmp, dumpoffset, printoffset;
 	ssize_t		length;
-	size_t		dumplen;
+	size_t		dumplen, cnt = 0;
 	char		*bp;
 	void		*start;
 	int		dump = 0, rflag = 0, c;
@@ -422,6 +402,11 @@ mread_f(
 	start = check_mapping_range(mapping, offset, length, 0);
 	if (!start)
 		return 0;
+	dumpoffset = offset - mapping->offset;
+	if (dump == 2)
+		printoffset = offset;
+	else
+		printoffset = dumpoffset;
 
 	if (alloc_buffer(pagesize, 0, 0) < 0)
 		return 0;
@@ -432,28 +417,35 @@ mread_f(
 		dumplen = pagesize;
 
 	if (rflag) {
-		for (tmp = length, c = 0; tmp > 0; tmp--, bp++, c = 1)
-			if (read_mapping(bp, tmp, c? dump:0, offset, dumplen)) {
+		for (tmp = length - 1, c = 0; tmp >= 0; tmp--, c = 1) {
+			*bp = *(((char *)mapping->addr) + dumpoffset + tmp);
+			cnt++;
+			if (c && cnt == dumplen) {
+				if (dump) {
+					dump_buffer(printoffset, dumplen);
+					printoffset += dumplen;
+				}
 				bp = (char *)buffer;
 				dumplen = pagesize;
+				cnt = 0;
+			} else {
+				bp++;
 			}
+		}
 	} else {
-		for (tmp = 0, c = 0; tmp < length; tmp++, bp++, c = 1)
-			if (read_mapping(bp, tmp, c? dump:0, offset, dumplen)) {
+		for (tmp = 0, c = 0; tmp < length; tmp++, c = 1) {
+			*bp = *(((char *)mapping->addr) + dumpoffset + tmp);
+			cnt++;
+			if (c && cnt == dumplen) {
+				if (dump)
+					dump_buffer(printoffset + tmp -
+						(dumplen - 1), dumplen);
 				bp = (char *)buffer;
 				dumplen = pagesize;
+				cnt = 0;
+			} else {
+				bp++;
 			}
-	}
-	/* dump the remaining (partial page) part of the read buffer */
-	if (dump) {
-		if (rflag)
-			dumplen = length % pagesize;
-		else
-			dumplen = tmp % pagesize;
-		if (dumplen) {
-			if (dump == 2)
-				tmp += mapping->offset;
-			dump_buffer(tmp, dumplen);
 		}
 	}
 	return 0;
@@ -571,7 +563,7 @@ mwrite_f(
 		return 0;
 
 	if (rflag) {
-		for (tmp = offset + length; tmp > offset; tmp--)
+		for (tmp = offset + length -1; tmp >= offset; tmp--)
 			((char *)mapping->addr)[tmp] = seed;
 	} else {
 		for (tmp = offset; tmp < offset + length; tmp++)

Re: [PATCH 1/2]segmentation fault in xfs_io mread/mwrite command

[Nathan Scott]

On Wed, 2006-11-29 at 09:26 +0900, Utako Kusaka wrote:
> Hi,
> 
> I found the following issues in xfs_io.
>  mread command:
>   a) Causes a segmentation fault.
>      Because "length"+1 bytes data is copied to buffer in read_mapping(),
>      but buffer size is "length".
>   b) Reads from wrong offset.
>   c) The first byte of dump data is incorrect when length > page size.
>  mwrite command:
>   d) Data placement is incorrect when -r option is specified
>      because of wrong for-loop counter.
> 
> This patch fixes them.
> 

Looks OK - could you send explicit test cases that demonstrate each
problem please?  (i.e. actual xfs_io invocations).  Particularly the
segfault should be easy to show, something like:
xfs_io -f -c 'mmap ...' -c 'mread ...' /tmp/foo)

That way they can be added to the regression test suite to ensure these
things don't spontaneously break themselves in the future.

thanks!

-- 
Nathan

Re: [PATCH 1/2]segmentation fault in xfs_io mread/mwrite command

[Utako Kusaka]

Thanks for your response.
This is my test case.

$ ./xfs_io -f mmap -c "pwrite 0 16384" -c "mmap 4096 4096" -c "mread -r"
wrote 16384/16384 bytes at offset 0
16 KiB, 4 ops; 0.0000 sec (625 MiB/sec and 160000.0000 ops/sec)
Segmentation fault

$ cat /etc/SuSE-release
SUSE LINUX 10.0 (X86-64)
VERSION = 10.0

Thu, 30 Nov 2006 09:22:41 +1100 Nathan Scott wrote：
>On Wed, 2006-11-29 at 09:26 +0900, Utako Kusaka wrote:
>> Hi,
>> 
>> I found the following issues in xfs_io.
>>  mread command:
>>   a) Causes a segmentation fault.
>>      Because "length"+1 bytes data is copied to buffer in read_mapping(),
>>      but buffer size is "length".
>>   b) Reads from wrong offset.
>>   c) The first byte of dump data is incorrect when length > page size.
>>  mwrite command:
>>   d) Data placement is incorrect when -r option is specified
>>      because of wrong for-loop counter.
>> 
>> This patch fixes them.
>> 
>
>Looks OK - could you send explicit test cases that demonstrate each
>problem please?  (i.e. actual xfs_io invocations).  Particularly the
>segfault should be easy to show, something like:
>xfs_io -f -c 'mmap ...' -c 'mread ...' /tmp/foo)
>
>That way they can be added to the regression test suite to ensure these
>things don't spontaneously break themselves in the future.
>
>thanks!
>
>-- 
>Nathan