On Sun, Feb 28, 2016 at 04:40:21PM +0100, Stefan Kempf wrote:
> Stefan Kempf wrote:
> > Martin Pieuchot wrote:
> > > I'm also wondering when you say "an offset that's at or paste the
> > > EOF" does that include ``uio_resid''? I mean shouldn't you check
> > > for:
> > >
> > > if ((uio->uio_offset + uio->uio_resid) > fs->fs_maxfilesize)
> > > ...
> >
> > I think if uio->uio_offset < fs->fs_maxfilesize, then
> > min(uio->uio_resid, fs->fs_maxfilesize - uio->uio_offset)
> > bytes should be read.
>
> Ehh. Not quite true. It can't read more than the size of the file.
Yes, correct. uio_resid is not included, because instead of returning
zero, the function should transfer the available bytes (even if less
than has been requested).
> The read-loops of ffs_read and ext2_ind_read already check
> whether uio_offset is beyond the size of the file:
>
> for (error = 0, bp = NULL; uio->uio_resid > 0; bp = NULL) {
> if ((bytesinfile = DIP(ip, size) - uio->uio_offset) <= 0)
> break;
>
> If we are assured that DIP(ip, size) cannot return a bogus size
> (> fs->fs_maxfilesize), the check before the loop could be
>
> if (uio->uio_offset >= DIP(ip, size) || uio->uio_resid == 0)
> return (0);
>
> > Maybe the check should be if (uio_offset >= fs->fs_maxfilesize)
> > though. Otherwise access flags of the inode are changed at the end
> > if ext2_ind_read() and ffs_read() even though there was no real access.
Your comment got me investigate what the supposed behaviour is. It turns
out POSIX is pretty clear about the intended behaviour: "Upon successful
completion, where nbyte is greater than 0, read() will mark for update
the st_atime field of the file, and return the number of bytes read."
Note, that nbytes is the requested read size, not the actual read size,
so even if the actual read is zero bytes, atime has to be updated when
more has been requested.
I think what this means is, that we shouldn't check for (uio_offset >
filesize) before the loop at all and let the respecitve read loop do
it's magic. All the involved loops seem to handle this case correctly
already.
Does that make sense?
natano
Index: ufs/ext2fs/ext2fs_readwrite.c
===================================================================
RCS file: /cvs/src/sys/ufs/ext2fs/ext2fs_readwrite.c,v
retrieving revision 1.39
diff -u -p -r1.39 ext2fs_readwrite.c
--- ufs/ext2fs/ext2fs_readwrite.c 27 Feb 2016 18:50:38 -0000 1.39
+++ ufs/ext2fs/ext2fs_readwrite.c 28 Feb 2016 17:57:08 -0000
@@ -100,8 +100,8 @@ ext2_ind_read(struct vnode *vp, struct i
} else if (vp->v_type != VREG && vp->v_type != VDIR)
panic("%s: type %d", "ext2fs_read", vp->v_type);
#endif
- if (e2fs_overflow(fs, 0, uio->uio_offset))
- return (EFBIG);
+ if (uio->uio_offset < 0)
+ return (EINVAL);
if (uio->uio_resid == 0)
return (0);
@@ -163,7 +163,6 @@ ext4_ext_read(struct vnode *vp, struct i
struct ext4_extent_path path;
struct ext4_extent nex, *ep;
struct buf *bp;
- size_t orig_resid;
daddr_t lbn, pos;
off_t bytesinfile;
int size, xfersize, blkoffset;
@@ -171,12 +170,10 @@ ext4_ext_read(struct vnode *vp, struct i
memset(&path, 0, sizeof path);
- orig_resid = uio->uio_resid;
- if (orig_resid == 0)
+ if (uio->uio_offset < 0)
+ return (EINVAL);
+ if (uio->uio_resid == 0)
return (0);
-
- if (e2fs_overflow(fs, 0, uio->uio_offset))
- return (EFBIG);
while (uio->uio_resid > 0) {
if ((bytesinfile = ext2fs_size(ip) - uio->uio_offset) <= 0)
Index: ufs/ffs/ffs_vnops.c
===================================================================
RCS file: /cvs/src/sys/ufs/ffs/ffs_vnops.c,v
retrieving revision 1.84
diff -u -p -r1.84 ffs_vnops.c
--- ufs/ffs/ffs_vnops.c 27 Feb 2016 18:50:38 -0000 1.84
+++ ufs/ffs/ffs_vnops.c 28 Feb 2016 17:57:08 -0000
@@ -214,9 +214,8 @@ ffs_read(void *v)
panic("ffs_read: type %d", vp->v_type);
#endif
fs = ip->i_fs;
- if ((u_int64_t)uio->uio_offset > fs->fs_maxfilesize)
- return (EFBIG);
-
+ if (uio->uio_offset < 0)
+ return (EINVAL);
if (uio->uio_resid == 0)
return (0);
> > > > Index: ufs/ext2fs/ext2fs_readwrite.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/ufs/ext2fs/ext2fs_readwrite.c,v
> > > > retrieving revision 1.39
> > > > diff -u -p -r1.39 ext2fs_readwrite.c
> > > > --- ufs/ext2fs/ext2fs_readwrite.c 27 Feb 2016 18:50:38 -0000
> > > > 1.39
> > > > +++ ufs/ext2fs/ext2fs_readwrite.c 28 Feb 2016 09:10:16 -0000
> > > > @@ -100,9 +100,9 @@ ext2_ind_read(struct vnode *vp, struct i
> > > > } else if (vp->v_type != VREG && vp->v_type != VDIR)
> > > > panic("%s: type %d", "ext2fs_read", vp->v_type);
> > > > #endif
> > > > - if (e2fs_overflow(fs, 0, uio->uio_offset))
> > > > - return (EFBIG);
> > > > - if (uio->uio_resid == 0)
> > > > + if (uio->uio_offset < 0)
> > > > + return (EINVAL);
> > > > + if (uio->uio_offset > fs->e2fs_maxfilesize || uio->uio_resid ==
> > > > 0)
> > > > return (0);
> > > >
> > > > for (error = 0, bp = NULL; uio->uio_resid > 0; bp = NULL) {
> > > > @@ -163,7 +163,6 @@ ext4_ext_read(struct vnode *vp, struct i
> > > > struct ext4_extent_path path;
> > > > struct ext4_extent nex, *ep;
> > > > struct buf *bp;
> > > > - size_t orig_resid;
> > > > daddr_t lbn, pos;
> > > > off_t bytesinfile;
> > > > int size, xfersize, blkoffset;
> > > > @@ -171,12 +170,10 @@ ext4_ext_read(struct vnode *vp, struct i
> > > >
> > > > memset(&path, 0, sizeof path);
> > > >
> > > > - orig_resid = uio->uio_resid;
> > > > - if (orig_resid == 0)
> > > > + if (uio->uio_offset < 0)
> > > > + return (EINVAL);
> > > > + if (uio->uio_offset > fs->e2fs_maxfilesize || uio->uio_resid ==
> > > > 0)
> > > > return (0);
> > > > -
> > > > - if (e2fs_overflow(fs, 0, uio->uio_offset))
> > > > - return (EFBIG);
> > > >
> > > > while (uio->uio_resid > 0) {
> > > > if ((bytesinfile = ext2fs_size(ip) - uio->uio_offset)
> > > > <= 0)
> > > > Index: ufs/ffs/ffs_vnops.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/ufs/ffs/ffs_vnops.c,v
> > > > retrieving revision 1.84
> > > > diff -u -p -r1.84 ffs_vnops.c
> > > > --- ufs/ffs/ffs_vnops.c 27 Feb 2016 18:50:38 -0000 1.84
> > > > +++ ufs/ffs/ffs_vnops.c 28 Feb 2016 09:10:16 -0000
> > > > @@ -214,10 +214,9 @@ ffs_read(void *v)
> > > > panic("ffs_read: type %d", vp->v_type);
> > > > #endif
> > > > fs = ip->i_fs;
> > > > - if ((u_int64_t)uio->uio_offset > fs->fs_maxfilesize)
> > > > - return (EFBIG);
> > > > -
> > > > - if (uio->uio_resid == 0)
> > > > + if (uio->uio_offset < 0)
> > > > + return (EINVAL);
> > > > + if (uio->uio_offset > fs->fs_maxfilesize || uio->uio_resid == 0)
> > > > return (0);
> > > >
> > > > for (error = 0, bp = NULL; uio->uio_resid > 0; bp = NULL) {
> > > >
> > >
>
