Date: Thu, 18 Feb 2010 07:31:51 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Poul-Henning Kamp <phk@phk.freebsd.dk> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r203990 - head/lib/libc/sys Message-ID: <20100218064545.J2074@besplex.bde.org> In-Reply-To: <6413.1266433105@critter.freebsd.dk> References: <6413.1266433105@critter.freebsd.dk>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <stdio.h> > #include <err.h> > > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100218064545.J2074>