From owner-svn-src-head@FreeBSD.ORG Wed Feb 17 20:31:56 2010 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C12731065679; Wed, 17 Feb 2010 20:31:56 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail09.syd.optusnet.com.au (mail09.syd.optusnet.com.au [211.29.132.190]) by mx1.freebsd.org (Postfix) with ESMTP id 597488FC18; Wed, 17 Feb 2010 20:31:56 +0000 (UTC) Received: from besplex.bde.org (c122-106-163-215.carlnfd1.nsw.optusnet.com.au [122.106.163.215]) by mail09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id o1HKVpPi012911 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 18 Feb 2010 07:31:53 +1100 Date: Thu, 18 Feb 2010 07:31:51 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Poul-Henning Kamp In-Reply-To: <6413.1266433105@critter.freebsd.dk> Message-ID: <20100218064545.J2074@besplex.bde.org> References: <6413.1266433105@critter.freebsd.dk> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans Subject: Re: svn commit: r203990 - head/lib/libc/sys X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Feb 2010 20:31:56 -0000 On Wed, 17 Feb 2010, Poul-Henning Kamp wrote: > In message <20100218044931.S95007@delplex.bde.org>, Bruce Evans writes: >> On Wed, 17 Feb 2010, Poul-Henning Kamp wrote: >> >>> Log: >>> Mention EISDIR as a possible errno. >> >> It's not a possible error. > > critter phk> cat > a.c > #include > #include > > int > main(int argc, char **argv) > { > if (unlink("/")) > err(1, "Told you so"); > return (0); > } > critter phk> cc a.c > critter phk> ./a.out > a.out: Told you so: Is a directory > critter phk> Better fix the kernel bug than break the documentation then. The bug is very old -- it happens in ~5.2, where the code is a bit easier to understand: % int % kern_unlink(struct thread *td, char *path, enum uio_seg pathseg) % { % struct mount *mp; % struct vnode *vp; % int error; % struct nameidata nd; % % restart: % bwillwrite(); % NDINIT(&nd, DELETE, NOFOLLOW | LOCKPARENT | LOCKLEAF, pathseg, path, % td); % if ((error = namei(&nd)) != 0) % return (error); namei() returns EISDIR for "/" (due to DELETE and and the special handling of the degenerate case which includes "/" and not much else, else the bug would affect more cases). Then we return the wrong errno before we test VDIR. % vp = nd.ni_vp; % if (vp->v_type == VDIR) % error = EPERM; % else { Untested fix for ~5.2. Also fixes some style bugs. % Index: vfs_syscalls.c % =================================================================== % RCS file: /home/ncvs/src/sys/kern/vfs_syscalls.c,v % retrieving revision 1.354 % diff -u -2 -r1.354 vfs_syscalls.c % --- vfs_syscalls.c 24 Jun 2004 17:22:29 -0000 1.354 % +++ vfs_syscalls.c 17 Feb 2010 20:01:09 -0000 % @@ -1614,10 +1605,11 @@ % restart: % bwillwrite(); % - NDINIT(&nd, DELETE, LOCKPARENT|LOCKLEAF, pathseg, path, td); % + NDINIT(&nd, DELETE, NOFOLLOW | LOCKPARENT | LOCKLEAF, pathseg, path, % + td); Style fixes: - spell out NOFOLLOW (NOFOLLOW is 0, so omitting it is just confusing). This bug is in about 3 other vfs syscalls. - there are spaces around binary operators in KNF. These spaces are especially important for the "|" operator since this operator looks more like an alphanumeric character than most operator symbols, but they are most often omitted for this operator :-(. % if ((error = namei(&nd)) != 0) % - return (error); % + return (error == EISDIR ? EPERM : error); Fix the EISDIR bug. % vp = nd.ni_vp; % if (vp->v_type == VDIR) % - error = EPERM; /* POSIX */ % + error = EPERM; Remove banal/misleading comment. The new fixup needs a comment more than this, but I don't want to add one. % else { % /* ISTR trying to avoid the special handling for the degenerate case in namei() and lookup() (2 almost identical copies of it). The correct fix may be there. From lookup() in ~5.2: % /* % * Check for degenerate name (e.g. / or "") % * which is a way of talking about a directory, % * e.g. like "/." or ".". % */ % if (cnp->cn_nameptr[0] == '\0') { % if (dp->v_type != VDIR) { % error = ENOTDIR; % goto bad; % } % if (cnp->cn_nameiop != LOOKUP) { % error = EISDIR; % goto bad; % } Cases with trailing slashes are handled by removing the slash, but this doesn't work for "/" since we don't want to end up with the fully degenerate name of "". Cases starting with this fully degnerate name haven't been permitted since ~1988 when POSIX disallowed it, but the comment in the code hasn't caught up with this. The comment gives these cases as examples only, but I think that is another bug and that this is a complete list of degenerate names so the comment should say "i.e.,". After catching up with 1988 and removing "" from the list, only "/" remains. This is not really degenerate and can hopefully be handled more directly and simply. The != VDIR case in the above might already be unreachable. Bruce