open(O_NOFOLLOW) error when encountered symlink

[Kostik Belousov]

On Sat, Mar 12, 2011 at 08:31:32PM +0100, Jilles Tjoelker wrote:
> On Sat, Mar 12, 2011 at 07:01:23PM +0200, Kostik Belousov wrote:
> > Hello,
> > I noted the following discussion and commits in the gnu tar repository:
> 
> > http://lists.gnu.org/archive/html/bug-tar/2010-11/msg00080.html
> > 
> > http://git.savannah.gnu.org/cgit/tar.git/commit/?id=1584b72ff271e7f826dd64d7a1c7cd2f66504acb
> > http://git.savannah.gnu.org/cgit/tar.git/commit/?id=649b747913d2b289e904b5f1d222af886acd209c
> 
> > The issue is that in case of open(path, O_NOFOLLOW), when path is naming
> > a symlink, FreeBSD returns EMLINK error. On the other hand, the POSIX
> > requirement is absolutely clear that it shall be ELOOP.
> 
> > I found FreeBSD commit r35088 that specifically changed the error code
> > from the required ELOOP to EMLINK. I doubt that somebody can remember
> > a reason for the change done more then 12 years ago.
> 
> In fact that change was done hours after the new ELOOP error.
Do you mean r35083, r35084 and r35085 ?

> 
> > Anybody have strong objections against the patch below ?
> 
> Although it loses information (ELOOP may also be caused by the directory
> prefix), I think we should make the change.
> 
> Please move the error condition in open.2 below the other [ELOOP] error.
> 
> usr.bin/cmp relies on the EMLINK error for the -h option and needs some
> adjustment. If ELOOP is returned and O_NOFOLLOW is in use, it needs to
> check using lstat() if the file is a symlink.
This is quite serious argument against the change, IMO.
Also, after your comment, I found the similar code in contrib/xz,
and somewhat doubtful resolve_symlink() in rcs sources.

I am recalling my proposal. Just for record, below is the updated patch

diff --git a/lib/libc/sys/open.2 b/lib/libc/sys/open.2
index deca8bc..1c9095d 100644
--- a/lib/libc/sys/open.2
+++ b/lib/libc/sys/open.2
@@ -304,6 +304,9 @@ is specified or
 .Dv O_APPEND
 is not specified.
 .It Bq Er ELOOP
+.Dv O_NOFOLLOW
+was specified and the target is a symbolic link.
+.It Bq Er ELOOP
 Too many symbolic links were encountered in translating the pathname.
 .It Bq Er EISDIR
 The named file is a directory, and the arguments specify
@@ -318,9 +321,6 @@ is specified and the named file would reside on a read-only file system.
 The process has already reached its limit for open file descriptors.
 .It Bq Er ENFILE
 The system file table is full.
-.It Bq Er EMLINK
-.Dv O_NOFOLLOW
-was specified and the target is a symbolic link.
 .It Bq Er ENXIO
 The named file is a character special or block
 special file, and the device associated with this special file
diff --git a/sys/kern/vfs_vnops.c b/sys/kern/vfs_vnops.c
index 7b5cad1..c7985ef 100644
--- a/sys/kern/vfs_vnops.c
+++ b/sys/kern/vfs_vnops.c
@@ -194,7 +194,7 @@ restart:
 		vp = ndp->ni_vp;
 	}
 	if (vp->v_type == VLNK) {
-		error = EMLINK;
+		error = ELOOP;
 		goto bad;
 	}
 	if (vp->v_type == VSOCK) {
diff --git a/usr.bin/cmp/cmp.c b/usr.bin/cmp/cmp.c
index f3ac717..84514d0 100644
--- a/usr.bin/cmp/cmp.c
+++ b/usr.bin/cmp/cmp.c
@@ -107,11 +107,14 @@ main(int argc, char *argv[])
 		fd1 = 0;
 		file1 = "stdin";
 	}
-	else if ((fd1 = open(file1, oflag, 0)) < 0 && errno != EMLINK) {
-		if (!sflag)
-			err(ERR_EXIT, "%s", file1);
-		else
-			exit(ERR_EXIT);
+	else if ((fd1 = open(file1, oflag, 0)) < 0) {
+		if (errno != ELOOP || stat(file1, &sb1) == -1 ||
+		    !S_ISLNK(sb1.st_mode)) {
+			if (!sflag)
+				err(ERR_EXIT, "%s", file1);
+			else
+				exit(ERR_EXIT);
+		}
 	}
 	if (strcmp(file2 = argv[1], "-") == 0) {
 		if (special)
@@ -121,11 +124,14 @@ main(int argc, char *argv[])
 		fd2 = 0;
 		file2 = "stdin";
 	}
-	else if ((fd2 = open(file2, oflag, 0)) < 0 && errno != EMLINK) {
-		if (!sflag)
-			err(ERR_EXIT, "%s", file2);
-		else
-			exit(ERR_EXIT);
+	else if ((fd2 = open(file2, oflag, 0)) < 0) {
+		if (errno != ELOOP || stat(file2, &sb2) == -1 ||
+		    !S_ISLNK(sb2.st_mode)) {
+			if (!sflag)
+				err(ERR_EXIT, "%s", file2);
+			else
+				exit(ERR_EXIT);
+		}
 	}
 
 	skip1 = argc > 2 ? strtol(argv[2], NULL, 0) : 0;

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-fs/attachments/20110312/46ea3dd5/attachment.pgp