Bug 21768

Comment 1 Robert Watson 2000-10-05 16:35:22 UTC

Responsible Changed
From-To: freebsd-bugs->rwatson

I'll take this.

On Thu, 5 Oct 2000 pb@hert.org wrote:

> >Description:
> 
>    Trailing slash after a symlink which points to a regular file resolves
>    the link, where i think it should return EISDIR.

I think you mean ENOTDIR.  If so, I agree (see the log message for rev.1.8
of vfs_lookup.c).

> --- vfs_lookup.c        Thu Oct  5 16:25:22 2000
> +++ vfs_lookup.c.new    Thu Oct  5 16:24:44 2000
> @@ -504,6 +504,14 @@
>         }
>  
>         /*
> +        * Check for bogus trailing slashes.
> +        */
> +       if (trailing_slash && dp->v_type != VDIR) {
> +               error = ENOTDIR;
> +               goto bad2;
> +       }
> +
> +       /*
>          * Check for symbolic link
>          */
>         if ((dp->v_type == VLNK) &&
> @@ -515,14 +523,6 @@
>                         goto bad2;
>                 }
>                 return (0);
> -       }
> -
> -       /*
> -        * Check for bogus trailing slashes.
> -        */
> -       if (trailing_slash && dp->v_type != VDIR) {
> -               error = ENOTDIR;
> -               goto bad2;
>         }
>  
>  nextname:

I think this breaks the case of a symlink to a directory.  Symlinks to
directories must be followed and turned into directories before the
check for bogus trailing slashes.

Bruce

Comment 3 Rebecca Cran 2008-03-07 23:35:31 UTC

This issue is still present on FreeBSD 7.0.

--
Bruce

Please, note that there is a discuission and some patches for this
issue at freebsd-hackers@:
  http://lists.freebsd.org/pipermail/freebsd-hackers/2009-May/028691.html
-- 
Eygene
 _                ___       _.--.   #
 \`.|\..----...-'`   `-._.-'_.-'`   #  Remember that it is hard
 /  ' `         ,       __.--'      #  to read the on-line manual
 )/' _/     \   `-_,   /            #  while single-stepping the kernel.
 `-'" `"\_  ,_.-;_.-\_ ',  fsc/as   #
     _.-'_./   {_.'   ; /           #    -- FreeBSD Developers handbook
    {_.-``-'         {_/            #

Comment 5 Robert Watson 2009-05-27 14:21:07 UTC

Responsible Changed
From-To: rwatson->des

Assign to des@ since he is working on a patch.

Comment 6 dfilter service 2009-05-29 11:02:53 UTC

Author: des
Date: Fri May 29 10:02:44 2009
New Revision: 193028
URL: http://svn.freebsd.org/changeset/base/193028

Log:
  Let vfs_lookup() return ENOTDIR if the path has a trailing slash and
  the last component is a symlink to something that isn't a directory.
  
  We introduce a new namei flag, TRAILINGSLASH, which is set by lookup()
  if the last component is followed by a slash.  The trailing slash is
  then stripped, as before.  If the final component is a symlink,
  lookup() will return to namei(), which will expand the symlink and
  call lookup() with the new path.  When all symlinks have been
  resolved, lookup() checks if the TRAILINGSLASH flag is set, and if it
  is, and the vnode it ended up with is not a directory, it returns
  ENOTDIR.
  
  PR:		kern/21768
  Submitted by:	Eygene Ryabinkin <rea-fbsd@codelabs.ru>
  MFC after:	3 weeks

Modified:
  head/sys/kern/vfs_lookup.c
  head/sys/sys/namei.h

Modified: head/sys/kern/vfs_lookup.c
==============================================================================
--- head/sys/kern/vfs_lookup.c	Fri May 29 09:52:13 2009	(r193027)
+++ head/sys/kern/vfs_lookup.c	Fri May 29 10:02:44 2009	(r193028)
@@ -147,6 +147,9 @@ namei(struct nameidata *ndp)
 		cnp->cn_flags &= ~LOCKSHARED;
 	fdp = p->p_fd;
 
+	/* We will set this ourselves if we need it. */
+	cnp->cn_flags &= ~TRAILINGSLASH;
+
 	/*
 	 * Get a buffer for the name to be translated, and copy the
 	 * name into the buffer.
@@ -533,6 +536,7 @@ dirloop:
 		if (*cp == '\0') {
 			trailing_slash = 1;
 			*ndp->ni_next = '\0';	/* XXX for direnter() ... */
+			cnp->cn_flags |= TRAILINGSLASH;
 		}
 	}
 	ndp->ni_next = cp;
@@ -807,14 +811,6 @@ unionlookup:
 		goto success;
 	}
 
-	/*
-	 * Check for bogus trailing slashes.
-	 */
-	if (trailing_slash && dp->v_type != VDIR) {
-		error = ENOTDIR;
-		goto bad2;
-	}
-
 nextname:
 	/*
 	 * Not a symbolic link.  If more pathname,
@@ -838,6 +834,14 @@ nextname:
 		goto dirloop;
 	}
 	/*
+	 * If we're processing a path with a trailing slash,
+	 * check that the end result is a directory.
+	 */
+	if ((cnp->cn_flags & TRAILINGSLASH) && dp->v_type != VDIR) {
+		error = ENOTDIR;
+		goto bad2;
+	}
+	/*
 	 * Disallow directory write attempts on read-only filesystems.
 	 */
 	if (rdonly &&

Modified: head/sys/sys/namei.h
==============================================================================
--- head/sys/sys/namei.h	Fri May 29 09:52:13 2009	(r193027)
+++ head/sys/sys/namei.h	Fri May 29 10:02:44 2009	(r193028)
@@ -142,7 +142,8 @@ struct nameidata {
 #define	GIANTHELD	0x02000000 /* namei() is holding giant. */
 #define	AUDITVNODE1	0x04000000 /* audit the looked up vnode information */
 #define	AUDITVNODE2 	0x08000000 /* audit the looked up vnode information */
-#define	PARAMASK	0x0ffffe00 /* mask of parameter descriptors */
+#define	TRAILINGSLASH	0x10000000 /* path ended in a slash */
+#define	PARAMASK	0x1ffffe00 /* mask of parameter descriptors */
 
 #define	NDHASGIANT(NDP)	(((NDP)->ni_cnd.cn_flags & GIANTHELD) != 0)
 
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"

Comment 7 Dag-Erling Smørgrav 2009-05-29 17:50:44 UTC

State Changed
From-To: open->patched

Fixed in head

Comment 8 Dag-Erling Smørgrav 2009-06-21 20:39:42 UTC

State Changed
From-To: patched->closed

Fixed, thanks

Comment 9 dfilter service 2009-06-21 20:39:48 UTC

Author: des
Date: Sun Jun 21 19:39:34 2009
New Revision: 194604
URL: http://svn.freebsd.org/changeset/base/194604

Log:
  merge r193027, r193557, r192900, r193028: fix trailing-slash symlink bug
  
  PR:		kern/21768

Modified:
  stable/7/sys/   (props changed)
  stable/7/sys/contrib/pf/   (props changed)
  stable/7/sys/kern/vfs_lookup.c
  stable/7/sys/sys/namei.h

Modified: stable/7/sys/kern/vfs_lookup.c
==============================================================================
--- stable/7/sys/kern/vfs_lookup.c	Sun Jun 21 19:31:41 2009	(r194603)
+++ stable/7/sys/kern/vfs_lookup.c	Sun Jun 21 19:39:34 2009	(r194604)
@@ -138,6 +138,9 @@ namei(struct nameidata *ndp)
 		cnp->cn_flags &= ~LOCKSHARED;
 	fdp = p->p_fd;
 
+	/* We will set this ourselves if we need it. */
+	cnp->cn_flags &= ~TRAILINGSLASH;
+
 	/*
 	 * Get a buffer for the name to be translated, and copy the
 	 * name into the buffer.
@@ -224,7 +227,7 @@ namei(struct nameidata *ndp)
 		vfslocked = (ndp->ni_cnd.cn_flags & GIANTHELD) != 0;
 		ndp->ni_cnd.cn_flags &= ~GIANTHELD;
 		/*
-		 * Check for symbolic link
+		 * If not a symbolic link, we're done.
 		 */
 		if ((cnp->cn_flags & ISSYMLINK) == 0) {
 			if ((cnp->cn_flags & (SAVENAME | SAVESTART)) == 0) {
@@ -367,7 +370,6 @@ lookup(struct nameidata *ndp)
 	int docache;			/* == 0 do not cache last component */
 	int wantparent;			/* 1 => wantparent or lockparent flag */
 	int rdonly;			/* lookup read-only flag bit */
-	int trailing_slash;
 	int error = 0;
 	int dpunlocked = 0;		/* dp has already been unlocked */
 	struct componentname *cnp = &ndp->ni_cnd;
@@ -439,13 +441,12 @@ dirloop:
 	 * trailing slashes to handle symlinks, existing non-directories
 	 * and non-existing files that won't be directories specially later.
 	 */
-	trailing_slash = 0;
 	while (*cp == '/' && (cp[1] == '/' || cp[1] == '\0')) {
 		cp++;
 		ndp->ni_pathlen--;
 		if (*cp == '\0') {
-			trailing_slash = 1;
-			*ndp->ni_next = '\0';	/* XXX for direnter() ... */
+			*ndp->ni_next = '\0';
+			cnp->cn_flags |= TRAILINGSLASH;
 		}
 	}
 	ndp->ni_next = cp;
@@ -604,27 +605,24 @@ unionlookup:
 		if (error != EJUSTRETURN)
 			goto bad;
 		/*
-		 * If creating and at end of pathname, then can consider
-		 * allowing file to be created.
+		 * At this point, we know we're at the end of the
+		 * pathname.  If creating / renaming, we can consider
+		 * allowing the file or directory to be created / renamed,
+		 * provided we're not on a read-only filesystem.
 		 */
 		if (rdonly) {
 			error = EROFS;
 			goto bad;
 		}
-		if (*cp == '\0' && trailing_slash &&
-		     !(cnp->cn_flags & WILLBEDIR)) {
+		/* trailing slash only allowed for directories */
+		if ((cnp->cn_flags & TRAILINGSLASH) &&
+		    !(cnp->cn_flags & WILLBEDIR)) {
 			error = ENOENT;
 			goto bad;
 		}
 		if ((cnp->cn_flags & LOCKPARENT) == 0)
 			VOP_UNLOCK(dp, 0, td);
 		/*
-		 * This is a temporary assert to make sure I know what the
-		 * behavior here was.
-		 */
-		KASSERT((cnp->cn_flags & (WANTPARENT|LOCKPARENT)) != 0,
-		   ("lookup: Unhandled case."));
-		/*
 		 * We return with ni_vp NULL to indicate that the entry
 		 * doesn't currently exist, leaving a pointer to the
 		 * (possibly locked) directory vnode in ndp->ni_dvp.
@@ -687,7 +685,7 @@ unionlookup:
 	 * Check for symbolic link
 	 */
 	if ((dp->v_type == VLNK) &&
-	    ((cnp->cn_flags & FOLLOW) || trailing_slash ||
+	    ((cnp->cn_flags & FOLLOW) || (cnp->cn_flags & TRAILINGSLASH) ||
 	     *ndp->ni_next == '/')) {
 		cnp->cn_flags |= ISSYMLINK;
 		if (dp->v_iflag & VI_DOOMED) {
@@ -710,18 +708,10 @@ unionlookup:
 		goto success;
 	}
 
-	/*
-	 * Check for bogus trailing slashes.
-	 */
-	if (trailing_slash && dp->v_type != VDIR) {
-		error = ENOTDIR;
-		goto bad2;
-	}
-
 nextname:
 	/*
-	 * Not a symbolic link.  If more pathname,
-	 * continue at next component, else return.
+	 * Not a symbolic link that we will follow.  Continue with the
+	 * next component if there is any; otherwise, we're done.
 	 */
 	KASSERT((cnp->cn_flags & ISLASTCN) || *ndp->ni_next == '/',
 	    ("lookup: invalid path state."));
@@ -741,6 +731,14 @@ nextname:
 		goto dirloop;
 	}
 	/*
+	 * If we're processing a path with a trailing slash,
+	 * check that the end result is a directory.
+	 */
+	if ((cnp->cn_flags & TRAILINGSLASH) && dp->v_type != VDIR) {
+		error = ENOTDIR;
+		goto bad2;
+	}
+	/*
 	 * Disallow directory write attempts on read-only filesystems.
 	 */
 	if (rdonly &&
@@ -891,12 +889,6 @@ relookup(struct vnode *dvp, struct vnode
 		if ((cnp->cn_flags & LOCKPARENT) == 0)
 			VOP_UNLOCK(dp, 0, td);
 		/*
-		 * This is a temporary assert to make sure I know what the
-		 * behavior here was.
-		 */
-		KASSERT((cnp->cn_flags & (WANTPARENT|LOCKPARENT)) != 0,
-		   ("relookup: Unhandled case."));
-		/*
 		 * We return with ni_vp NULL to indicate that the entry
 		 * doesn't currently exist, leaving a pointer to the
 		 * (possibly locked) directory vnode in ndp->ni_dvp.

Modified: stable/7/sys/sys/namei.h
==============================================================================
--- stable/7/sys/sys/namei.h	Sun Jun 21 19:31:41 2009	(r194603)
+++ stable/7/sys/sys/namei.h	Sun Jun 21 19:39:34 2009	(r194604)
@@ -127,26 +127,27 @@ struct nameidata {
  * name being sought. The caller is responsible for releasing the
  * buffer and for vrele'ing ni_startdir.
  */
-#define	RDONLY		0x0000200 /* lookup with read-only semantics */
-#define	HASBUF		0x0000400 /* has allocated pathname buffer */
-#define	SAVENAME	0x0000800 /* save pathname buffer */
-#define	SAVESTART	0x0001000 /* save starting directory */
-#define ISDOTDOT	0x0002000 /* current component name is .. */
-#define MAKEENTRY	0x0004000 /* entry is to be added to name cache */
-#define ISLASTCN	0x0008000 /* this is last component of pathname */
-#define ISSYMLINK	0x0010000 /* symlink needs interpretation */
-#define	ISWHITEOUT	0x0020000 /* found whiteout */
-#define	DOWHITEOUT	0x0040000 /* do whiteouts */
-#define	WILLBEDIR	0x0080000 /* new files will be dirs; allow trailing / */
-#define	ISUNICODE	0x0100000 /* current component name is unicode*/
-#define	ISOPEN		0x0200000 /* caller is opening; return a real vnode. */
-#define	NOCROSSMOUNT	0x0400000 /* do not cross mount points */
-#define	NOMACCHECK	0x0800000 /* do not perform MAC checks */
-#define	MPSAFE		0x1000000 /* namei() must acquire Giant if needed. */
-#define	GIANTHELD	0x2000000 /* namei() is holding giant. */
-#define	AUDITVNODE1	0x4000000 /* audit the looked up vnode information */
-#define	AUDITVNODE2 	0x8000000 /* audit the looked up vnode information */
-#define	PARAMASK	0xffffe00 /* mask of parameter descriptors */
+#define	RDONLY		0x00000200 /* lookup with read-only semantics */
+#define	HASBUF		0x00000400 /* has allocated pathname buffer */
+#define	SAVENAME	0x00000800 /* save pathname buffer */
+#define	SAVESTART	0x00001000 /* save starting directory */
+#define	ISDOTDOT	0x00002000 /* current component name is .. */
+#define	MAKEENTRY	0x00004000 /* entry is to be added to name cache */
+#define	ISLASTCN	0x00008000 /* this is last component of pathname */
+#define	ISSYMLINK	0x00010000 /* symlink needs interpretation */
+#define	ISWHITEOUT	0x00020000 /* found whiteout */
+#define	DOWHITEOUT	0x00040000 /* do whiteouts */
+#define	WILLBEDIR	0x00080000 /* new files will be dirs; allow trailing / */
+#define	ISUNICODE	0x00100000 /* current component name is unicode*/
+#define	ISOPEN		0x00200000 /* caller is opening; return a real vnode. */
+#define	NOCROSSMOUNT	0x00400000 /* do not cross mount points */
+#define	NOMACCHECK	0x00800000 /* do not perform MAC checks */
+#define	MPSAFE		0x01000000 /* namei() must acquire Giant if needed. */
+#define	GIANTHELD	0x02000000 /* namei() is holding giant. */
+#define	AUDITVNODE1	0x04000000 /* audit the looked up vnode information */
+#define	AUDITVNODE2 	0x08000000 /* audit the looked up vnode information */
+#define	TRAILINGSLASH	0x10000000 /* path ended in a slash */
+#define	PARAMASK	0x1ffffe00 /* mask of parameter descriptors */
 
 #define	NDHASGIANT(NDP)	(((NDP)->ni_cnd.cn_flags & GIANTHELD) != 0)
 
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"