Martin Natano wrote:
> 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.
Thanks for checking. Looks good.
> 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) {
> > > > >
> > > >
> >
