Date: Wed, 15 Aug 2001 20:15:21 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: "Andrey A. Chernov" <ache@nagual.pp.ru> Cc: <arch@FreeBSD.ORG>, <current@FreeBSD.ORG> Subject: Re: CFR: lseek() POSIXed patch Message-ID: <20010815190108.J19482-100000@besplex.bde.org> In-Reply-To: <20010815125248.A2588@nagual.pp.ru>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 15 Aug 2001, Andrey A. Chernov wrote:
> The patch below adds both cases, i.e. disallow negative seeks for VREG,
> VDIR, VBLK and add off_t overflow checks.
>
> I plan to commit this, please review.
>
> --- vfs_syscalls.c.old Wed Aug 15 04:45:30 2001
> +++ vfs_syscalls.c Wed Aug 15 12:46:12 2001
> @@ -1614,29 +1614,44 @@
> register struct filedesc *fdp = p->p_fd;
> register struct file *fp;
> struct vattr vattr;
> - int error;
> + off_t offset;
> + int error, no_neg_seek;
>
> if ((u_int)SCARG(uap, fd) >= fdp->fd_nfiles ||
> (fp = fdp->fd_ofiles[SCARG(uap, fd)]) == NULL)
> return (EBADF);
> if (fp->f_type != DTYPE_VNODE)
> return (ESPIPE);
> + error = VOP_GETATTR((struct vnode *)fp->f_data, &vattr, cred, p);
> + no_neg_seek = (!error &&
> + (vattr.va_type == VREG ||
> + vattr.va_type == VDIR ||
> + vattr.va_type == VBLK));
The type is in vp->v_type (where vp = (struct vnode *)fp->f_data), so
the VOP_GETTATTR() call can be avoided except in the L_XTND case, as
in the old code.
I think permitting negative offsets for the VCHR case only is enough.
> + offset = SCARG(uap, offset);
> switch (SCARG(uap, whence)) {
> case L_INCR:
> - fp->f_offset += SCARG(uap, offset);
> + if ((fp->f_offset > 0 && offset > 0 &&
> + offset + fp->f_offset < 0) ||
> + (fp->f_offset < 0 && offset < 0 &&
> + offset + fp->f_offset > 0))
> + return (EOVERFLOW);
You used a slightly simpler, slightly less ugly overflow checks in
fseek.c. This seems to be a bug in fseek.c. Since the checks are so
messy, maybe they should be messy enough to not depend on undefined
behaviour (they check for overflow after overflow may have occurred).
Something like:
#define OFF_T_MAX 0x7FFFFFFFFFFFFFFF /* XXX */
#define OFF_T_MIN (-0x7FFFFFFFFFFFFFFF - 1) /* XXX */
start = fp->f_offset;
if (offset > 0 && start > OFF_T_MAX - offset ||
offset < 0 && start < OFF_T_MIN - offset)
return (EOVERFLOW);
> + if ((fp->f_offset > 0 && offset > 0 &&
> + offset + fp->f_offset < 0) ||
> + (fp->f_offset < 0 && offset < 0 &&
> + offset + fp->f_offset > 0))
> + offset += fp->f_offset;
> break;
> case L_XTND:
> - error=VOP_GETATTR((struct vnode *)fp->f_data, &vattr, cred, p);
> if (error)
> return (error);
> - fp->f_offset = SCARG(uap, offset) + vattr.va_size;
> + if (offset > 0 && (off_t)(offset + vattr.va_size) < 0)
> + return (EOVERFLOW);
> + offset += vattr.va_size;
This assumes that vattr.va_size is representable as a (non-negative) off_t.
E.g., if offset is 1 and vattr.va_size is (uquad_t)-1, then the sum overflows
but the overflow is not detected.
> break;
> case L_SET:
> - fp->f_offset = SCARG(uap, offset);
> break;
> default:
> return (EINVAL);
> }
> + if (no_neg_seek && offset < 0)
> + return (EINVAL);
Some no_neg_seek conditionals are needed for the overflow checks too.
Otherwise seeking from offset OFF_T_MAX to 1 more than that is disallowed
for all types of files. The offset overflows, but it only reaches the
half way mark in a 64-bit /dev/kmem.
> + fp->f_offset = offset;
> *(off_t *)(p->p_retval) = fp->f_offset;
> return (0);
> }
kern_lockf.c has some related problems. It doesn't distinguish the
EOVERFLOW case from the EINVAL case as is now required by POSIX. POSIX
now specifies the behaviour for weird cases like negative lengths (it
gives an interval extending from the top down), but I made the overflow
checking too strong to permit this.
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-arch" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010815190108.J19482-100000>