lseek/llseek allows the negative offset

lseek/llseek allows the negative offset

[H . J . Lu]

# gcc x.c
# ./a.out
lseek on -100000: -100000
write: File too large

Should kernel allow negative offsets for lseek/llseek?


-- 
H.J. Lu (hjl@valinux.com)
---
#include <fcntl.h>
#include <unistd.h>
#include <stdio.h>

extern loff_t llseek (int fd, loff_t offset, int whence);

int
main ()
{
  int fd = open ("/tmp/foo.out", O_CREAT | O_RDWR, 0600);
  off_t res, pos;
  loff_t lres, lpos;
  char buffer [] = "negative offset";

  if (fd < 0)
    {
      perror ("open");
      return 1;
    }

  pos = -100000;
  res = lseek (fd, pos, SEEK_SET);
  if (res == (off_t) -1L)
    {
      perror ("lseek");
      close (fd);
      return 1;
    }
  printf ("lseek on %ld: %ld\n", pos, res);

  if (write (fd, buffer, sizeof (buffer)) != sizeof (buffer))
    {
      perror ("write");
      close (fd);
      return 1;
    }

  lpos = -100000LL;
  lres = llseek (fd, lpos, SEEK_SET);
  if (lres == (loff_t) -1L)
    {
      perror ("llseek");
      close (fd);
      return 1;
    }

  
  printf ("llseek on %lld: %lld\n", lpos, lres);

  if (write (fd, buffer, sizeof (buffer)) != sizeof (buffer))
    {
      perror ("write");
      close (fd);
      return 1;
    }

  close (fd);

  return 0;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: lseek/llseek allows the negative offset

[H . J . Lu]

On Fri, Nov 17, 2000 at 03:59:13PM -0800, H . J . Lu wrote:
> # gcc x.c
> # ./a.out
> lseek on -100000: -100000
> write: File too large
> 
> Should kernel allow negative offsets for lseek/llseek?
> 
> 

Never mind. I was running the wrong kernel.

H.J.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: lseek/llseek allows the negative offset

[Andrea Arcangeli]

On Fri, Nov 17, 2000 at 04:09:00PM -0800, H . J . Lu wrote:
> On Fri, Nov 17, 2000 at 03:59:13PM -0800, H . J . Lu wrote:
> > # gcc x.c
> > # ./a.out
> > lseek on -100000: -100000
> > write: File too large
> > 
> > Should kernel allow negative offsets for lseek/llseek?
> > 
> > 
> 
> Never mind. I was running the wrong kernel.

With 2.2.18pre21aa2 this little proggy:

main()
{
	int fd = creat("x", 0600);
	lseek(fd, 0x80000000, 0);
}

get confused this way:

lseek(3, 2147483648, SEEK_SET)          = -1 ERRNO_0 (Success)
_exit(-2147483648)                      = ?

I fixed it this way:

diff -urN 2.2.18pre21/fs/read_write.c lseek/fs/read_write.c
--- 2.2.18pre21/fs/read_write.c	Tue Sep  5 02:28:49 2000
+++ lseek/fs/read_write.c	Sat Nov 18 18:42:55 2000
@@ -53,6 +53,10 @@
 	struct dentry * dentry;
 	struct inode * inode;
 
+	retval = -EINVAL;
+	if (offset < 0)
+		goto out_nolock;
+
 	lock_kernel();
 	retval = -EBADF;
 	file = fget(fd);
@@ -69,6 +73,7 @@
 	fput(file);
 bad:
 	unlock_kernel();
+out_nolock:
 	return retval;
 }
 
@@ -83,6 +88,11 @@
 	struct inode * inode;
 	loff_t offset;
 
+	retval = -EINVAL;
+	offset = ((loff_t) offset_high << 32) | offset_low;
+	if (offset < 0)
+		goto out_nolock;
+
 	lock_kernel();
 	retval = -EBADF;
 	file = fget(fd);
@@ -96,8 +106,7 @@
 	if (origin > 2)
 		goto out_putf;
 
-	offset = llseek(file, ((loff_t) offset_high << 32) | offset_low,
-			origin);
+	offset = llseek(file, offset, origin);
 
 	retval = (int)offset;
 	if (offset >= 0) {
@@ -109,6 +118,7 @@
 	fput(file);
 bad:
 	unlock_kernel();
+out_nolock:
 	return retval;
 }
 #endif


I've not tried yet in practice but by reading sources 2.4.x has the same bug
since doing the check internally to ext2 is too late to handle the 32bit
(non-lfs) lseek interface.  After moving the checks into the vfs (that seems
the right thing to do to me) the check internally to ext2 can be removed of
course (it was superflous anyways because ext2 file size is limited to 4T
that's not negative value in 64bit signed math)

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: lseek/llseek allows the negative offset

[H . J . Lu]

On Sat, Nov 18, 2000 at 07:25:42PM +0100, Andrea Arcangeli wrote:
> On Fri, Nov 17, 2000 at 04:09:00PM -0800, H . J . Lu wrote:
> > On Fri, Nov 17, 2000 at 03:59:13PM -0800, H . J . Lu wrote:
> > > # gcc x.c
> > > # ./a.out
> > > lseek on -100000: -100000
> > > write: File too large
> > > 
> > > Should kernel allow negative offsets for lseek/llseek?
> > > 
> > > 
> > 
> > Never mind. I was running the wrong kernel.
> 
> With 2.2.18pre21aa2 this little proggy:
> 
> main()
> {
> 	int fd = creat("x", 0600);
> 	lseek(fd, 0x80000000, 0);
> }
> 
> get confused this way:
> 
> lseek(3, 2147483648, SEEK_SET)          = -1 ERRNO_0 (Success)
> _exit(-2147483648)                      = ?
> 
> I fixed it this way:
> 
> diff -urN 2.2.18pre21/fs/read_write.c lseek/fs/read_write.c
> --- 2.2.18pre21/fs/read_write.c	Tue Sep  5 02:28:49 2000
> +++ lseek/fs/read_write.c	Sat Nov 18 18:42:55 2000
> @@ -53,6 +53,10 @@
>  	struct dentry * dentry;
>  	struct inode * inode;
>  
> +	retval = -EINVAL;
> +	if (offset < 0)
> +		goto out_nolock;
> +

offset shouldn't be < 0 to begin with. There may be a bug somewhere
else. In my case,

	if (offset < 0)
		return -EINVAL;

is missing from those FS lseek functions.

H.J.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: lseek/llseek allows the negative offset

[Andrea Arcangeli]

On Sat, Nov 18, 2000 at 07:25:42PM +0100, Andrea Arcangeli wrote:
> I fixed it this way:

fix is plain wrong, it's still possible to have lseek return -1 -2 -3 -4
even when it should return -EINVAL.

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: lseek/llseek allows the negative offset

[H . J . Lu]

On Sun, Nov 19, 2000 at 01:45:12AM +0100, Andrea Arcangeli wrote:
> On Sat, Nov 18, 2000 at 07:25:42PM +0100, Andrea Arcangeli wrote:
> > I fixed it this way:
> 
> fix is plain wrong, it's still possible to have lseek return -1 -2 -3 -4
> even when it should return -EINVAL.
> 

Try this again 2.2.18pre21. It works for me.


-- 
H.J. Lu (hjl@valinux.com)
---
--- linux/fs/ext2/file.c.lseek	Sat Nov 18 17:18:49 2000
+++ linux/fs/ext2/file.c	Sat Nov 18 17:19:28 2000
@@ -120,6 +120,8 @@ static long long ext2_file_lseek(
 		case 1:
 			offset += file->f_pos;
 	}
+	if (offset < 0)
+		return -EINVAL;
 	if (((unsigned long long) offset >> 32) != 0) {
 #if BITS_PER_LONG < 64
 		return -EINVAL;
--- linux/fs/proc/mem.c.lseek	Tue Jan  4 10:12:23 2000
+++ linux/fs/proc/mem.c	Sat Nov 18 17:19:28 2000
@@ -196,14 +196,17 @@ static long long mem_lseek(struct file *
 {
 	switch (orig) {
 		case 0:
-			file->f_pos = offset;
-			return file->f_pos;
+			break;
 		case 1:
-			file->f_pos += offset;
-			return file->f_pos;
+			offset += file->f_pos;
+			break;
 		default:
 			return -EINVAL;
 	}
+	if (offset < 0)
+			return -EINVAL;
+	file->f_pos = offset;
+	return offset;
 }
 
 /*
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: lseek/llseek allows the negative offset

[Andrea Arcangeli]

On Sat, Nov 18, 2000 at 05:20:34PM -0800, H . J . Lu wrote:
> Try this again 2.2.18pre21. It works for me.
> 
> 
> -- 
> H.J. Lu (hjl@valinux.com)
> ---
> --- linux/fs/ext2/file.c.lseek	Sat Nov 18 17:18:49 2000
> +++ linux/fs/ext2/file.c	Sat Nov 18 17:19:28 2000
> @@ -120,6 +120,8 @@ static long long ext2_file_lseek(
>  		case 1:
>  			offset += file->f_pos;
>  	}
> +	if (offset < 0)
> +		return -EINVAL;
>  	if (((unsigned long long) offset >> 32) != 0) {
>  #if BITS_PER_LONG < 64
>  		return -EINVAL;

It's not enough for 2.2.x (and you left the `>> 32' nosense check).

2.2.x vanilla (so non-lfs) is getting wrong both 32bit and 64bit:

1) 32bit can lseek over 2G and it can return bogus retval

main()
{
        int fd = creat("x", 0600);
        lseek(fd, 0x7fffffff, 1);
        lseek(fd, 0x7fffffff, 1);
}

lseek(3, 2147483647, SEEK_CUR)          = 2147483647
lseek(3, 2147483647, SEEK_CUR)          = -1 ENOENT (No such file or directory)

(this isn't really -ENOENT but it's just trying to return a succesful -2 value)

2) 64bit can lseek over ext2_max_sizes and it can return bogus retvals

main()
{
        int fd = creat("x", 0600);
        lseek(fd, -2, 0);
}

open("x", O_WRONLY|O_CREAT|O_TRUNC, 0600) = 3
lseek(3, 18446744073709551614, SEEK_SET) = -1 ENOENT (No such file or directory)

(this isn't really -ENOENT but it's just trying to return a succesful -2 value)

This will cure 2.2.x vanilla:

--- 2.2.18pre21/fs/ext2/file.c.~1~	Sun Nov 12 00:45:42 2000
+++ 2.2.18pre21/fs/ext2/file.c	Sun Nov 19 03:59:29 2000
@@ -120,14 +120,14 @@
 		case 1:
 			offset += file->f_pos;
 	}
-	if (((unsigned long long) offset >> 32) != 0) {
 #if BITS_PER_LONG < 64
+	if (offset >> 31)
 		return -EINVAL;
 #else
-		if (offset > ext2_max_sizes[EXT2_BLOCK_SIZE_BITS(inode->i_sb)])
-			return -EINVAL;
+	if (offset < 0 ||
+	    offset > ext2_max_sizes[EXT2_BLOCK_SIZE_BITS(inode->i_sb)])
+		return -EINVAL;
 #endif
-	} 
 	if (offset != file->f_pos) {
 		file->f_pos = offset;
 		file->f_reada = 0;

and this will remove the nosense stuff from 2.4.x:

--- 2.4.0-test11-pre6/fs/ext2/file.c.~1~	Thu Nov 16 15:37:32 2000
+++ 2.4.0-test11-pre6/fs/ext2/file.c	Sun Nov 19 04:03:27 2000
@@ -53,12 +53,9 @@
 		case 1:
 			offset += file->f_pos;
 	}
-	if (offset<0)
+	if (offset < 0 ||
+	    offset > ext2_max_sizes[EXT2_BLOCK_SIZE_BITS(inode->i_sb)])
 		return -EINVAL;
-	if (((unsigned long long) offset >> 32) != 0) {
-		if (offset > ext2_max_sizes[EXT2_BLOCK_SIZE_BITS(inode->i_sb)])
-			return -EINVAL;
-	} 
 	if (offset != file->f_pos) {
 		file->f_pos = offset;
 		file->f_reada = 0;

(the above is also ok for 2.2.x-lfs and I'll fix it that way)

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: lseek/llseek allows the negative offset

[H . J . Lu]

On Sun, Nov 19, 2000 at 04:07:04AM +0100, Andrea Arcangeli wrote:
> On Sat, Nov 18, 2000 at 05:20:34PM -0800, H . J . Lu wrote:
> > Try this again 2.2.18pre21. It works for me.
> > 
> > 
> > -- 
> > H.J. Lu (hjl@valinux.com)
> > ---
> > --- linux/fs/ext2/file.c.lseek	Sat Nov 18 17:18:49 2000
> > +++ linux/fs/ext2/file.c	Sat Nov 18 17:19:28 2000
> > @@ -120,6 +120,8 @@ static long long ext2_file_lseek(
> >  		case 1:
> >  			offset += file->f_pos;
> >  	}
> > +	if (offset < 0)
> > +		return -EINVAL;
> >  	if (((unsigned long long) offset >> 32) != 0) {
> >  #if BITS_PER_LONG < 64
> >  		return -EINVAL;
> 
> It's not enough for 2.2.x (and you left the `>> 32' nosense check).
> 

It works for me on ia32. offset is a long long in 2.2.18pre21 on ia32.
I don't see anything wrong for

	if (offset < 0)
		return -EINVAL;

I got:

lseek(3, 4294967295, SEEK_SET)          = -1 EINVAL (Invalid argument)
lseek(3, 4294967294, SEEK_SET)          = -1 EINVAL (Invalid argument)
lseek(3, 4294967293, SEEK_SET)          = -1 EINVAL (Invalid argument)
lseek(3, 4294967292, SEEK_SET)          = -1 EINVAL (Invalid argument)
lseek(3, 4294967291, SEEK_SET)          = -1 EINVAL (Invalid argument)
lseek(3, 2147483647, SEEK_CUR)          = -1 EOVERFLOW (Value too large for defined data type)
lseek(3, 2147483647, SEEK_CUR)          = -1 EOVERFLOW (Value too large for defined data type)
_llseek(3, 18446744073709551614, 0xbffff980, SEEK_SET) = -1 EINVAL (Invalid argument)
lseek(3, 4294867296, SEEK_SET)          = -1 EINVAL (Invalid argument)

My kernel has LFS patch.

H.J.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: lseek/llseek allows the negative offset

[Andrea Arcangeli]

On Sat, Nov 18, 2000 at 05:20:34PM -0800, H . J . Lu wrote:
> --- linux/fs/proc/mem.c.lseek	Tue Jan  4 10:12:23 2000
> +++ linux/fs/proc/mem.c	Sat Nov 18 17:19:28 2000
> @@ -196,14 +196,17 @@ static long long mem_lseek(struct file *
>  {
>  	switch (orig) {
>  		case 0:
> -			file->f_pos = offset;
> -			return file->f_pos;
> +			break;
>  		case 1:
> -			file->f_pos += offset;
> -			return file->f_pos;
> +			offset += file->f_pos;
> +			break;
>  		default:
>  			return -EINVAL;
>  	}
> +	if (offset < 0)
> +			return -EINVAL;
> +	file->f_pos = offset;
> +	return offset;
>  }

This looks fine to me.

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/