From owner-freebsd-current Wed Aug 15 3:18:23 2001 Delivered-To: freebsd-current@freebsd.org Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by hub.freebsd.org (Postfix) with ESMTP id 45D8637B407; Wed, 15 Aug 2001 03:18:11 -0700 (PDT) (envelope-from bde@zeta.org.au) Received: from bde.zeta.org.au (bde.zeta.org.au [203.2.228.102]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id UAA27875; Wed, 15 Aug 2001 20:17:57 +1000 Date: Wed, 15 Aug 2001 20:15:21 +1000 (EST) From: Bruce Evans X-X-Sender: To: "Andrey A. Chernov" Cc: , Subject: Re: CFR: lseek() POSIXed patch In-Reply-To: <20010815125248.A2588@nagual.pp.ru> Message-ID: <20010815190108.J19482-100000@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG 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-current" in the body of the message