- Fix dangling symlink regression

- Make open(2) more POSIX compliant
- Add a test case for dangling symlinks and open() syscall with O_CREAT and
  O_EXCL on a symlink.
- Update open(2) man page to reflect change.

include/minix/vfsif.h

@@ -58,11 +58,11 @@
 #define REQ_RDONLY		001
 #define REQ_ISROOT		002
 #define PATH_NOFLAGS		000
-#define PATH_RET_SYMLINK	001	/* Return a symlink object (i.e.
+#define PATH_RET_SYMLINK	010	/* Return a symlink object (i.e.
 					 * do not continue with the contents
 					 * of the symlink if it is the last
 					 * component in a path). */
-#define PATH_GET_UCRED		002	/* Request provides a grant ID in m9_l1
+#define PATH_GET_UCRED		020	/* Request provides a grant ID in m9_l1
 					 * and struct ucred size in m9_s4 (as
 					 * opposed to a REQ_UID). */
 

man/man2/open.2

@@ -174,7 +174,10 @@ allocating the inode for O_CREAT.
 points outside the process's allocated address space.
 .TP 15
 [EEXIST]
-O_CREAT and O_EXCL were specified and the file exists.
+O_CREAT and O_EXCL were specified and the file exists or
+.I
+path
+names a symbolic link (regardless of contents of the link).
 .SH "SEE ALSO"
 .BR chmod (2),
 .BR close (2),

servers/mfs/read.c

@@ -584,7 +584,7 @@ PUBLIC int fs_getdents(void)
 	  else
 		  dp = &bp->b_dir[0];
 	  for (; dp < &bp->b_dir[NR_DIR_ENTRIES(block_size)]; dp++) {
-		  if (dp->d_ino == 0)
+		  if (dp->d_ino == 0) 
 			  continue;	/* Entry is not in use */
 
 		  /* Compute the length of the name */
@@ -600,7 +600,7 @@ PUBLIC int fs_getdents(void)
 		  if (o != 0)
 			  reclen += sizeof(long) - o;
 
-		  /* Need the postition of this entry in the directory */
+		  /* Need the position of this entry in the directory */
 		  ent_pos = block_pos + ((char *)dp - bp->b_data);
 
 		  if(tmpbuf_off + reclen > GETDENTS_BUFSIZ) {
@@ -620,7 +620,7 @@ PUBLIC int fs_getdents(void)
 			  /* The user has no space for one more record */
 			  done = TRUE;
 			  
-			  /* Record the postion of this entry, it is the
+			  /* Record the position of this entry, it is the
 			   * starting point of the next request (unless the
 			   * postion is modified with lseek).
 			   */

servers/vfs/open.c

@@ -31,7 +31,7 @@
 PRIVATE char mode_map[] = {R_BIT, W_BIT, R_BIT|W_BIT, 0};
 
 FORWARD _PROTOTYPE( int common_open, (int oflags, mode_t omode)		);
-FORWARD _PROTOTYPE( struct vnode *new_node, (mode_t bits)		);
+FORWARD _PROTOTYPE( struct vnode *new_node, (int oflags, mode_t bits)		);
 FORWARD _PROTOTYPE( int pipe_open, (struct vnode *vp,mode_t bits,int oflags));
 
 
@@ -96,7 +96,7 @@ PRIVATE int common_open(register int oflags, mode_t omode)
   /* If O_CREATE is set, try to make the file. */
   if (oflags & O_CREAT) {
         omode = I_REGULAR | (omode & ALL_MODES & fp->fp_umask);
-	vp = new_node(omode);
+	vp = new_node(oflags, omode);
 	r = err_code;
 	if (r == OK) exist = FALSE; /* We just created the file */
 	else if (r != EEXIST) return(r);  /* other error */ 
@@ -114,7 +114,7 @@ PRIVATE int common_open(register int oflags, mode_t omode)
   fil_ptr->filp_vno = vp;
   fil_ptr->filp_flags = oflags;
 
-  /* Only do the normal open code if didn't just create the file. */
+  /* Only do the normal open code if we didn't just create the file. */
   if(exist) {
 	/* Check protections. */
 	if ((r = forbidden(vp, bits)) == OK) {
@@ -236,18 +236,33 @@ PRIVATE int common_open(register int oflags, mode_t omode)
 /*===========================================================================*
  *				new_node				     *
  *===========================================================================*/
-PRIVATE struct vnode *new_node(mode_t bits)
+PRIVATE struct vnode *new_node(int oflags, mode_t bits)
 {
+/* Try to create a new inode and return a pointer to it. If the inode already
+   exists, return a pointer to it as well, but set err_code accordingly.
+   NIL_VNODE is returned if the path cannot be resolved up to the last
+   directory, or when the inode cannot be created due to permissions or
+   otherwise. */ 
   struct vnode *dirp, *vp;
-  int r;
+  int r, flags;
   struct node_details res;
   struct vnode *rest;
 
+  /* When O_CREAT and O_EXCL flags are set, the path may not be named by a
+   * symbolic link. */
+  flags = PATH_NOFLAGS;
+  if (oflags & O_EXCL) flags |= PATH_RET_SYMLINK;
+
   /* See if the path can be opened down to the last directory. */
   if ((dirp = last_dir()) == NIL_VNODE) return(NIL_VNODE);
 
   /* The final directory is accessible. Get final component of the path. */
-  vp = advance(dirp, 0);
+  vp = advance(dirp, flags);
+
+  /* The combination of a symlink with absolute path followed by a danglink
+   * symlink results in a new path that needs to be re-resolved entirely. */
+  if (user_fullpath[0] == '/') return new_node(oflags, bits);
+
   if (vp == NIL_VNODE && err_code == ENOENT) {
 	/* Last path component does not exist. Make a new directory entry. */
 	if ((vp = get_free_vnode()) == NIL_VNODE) {
@@ -258,10 +273,61 @@ PRIVATE struct vnode *new_node(mode_t bits)
 	if ((r = forbidden(dirp, W_BIT|X_BIT)) != OK ||
 	    (r = req_create(dirp->v_fs_e, dirp->v_inode_nr,bits, fp->fp_effuid,
 			    fp->fp_effgid, user_fullpath, &res)) != OK ) {
-		/* Can't create new directory entry: either no permission or
+		/* Can't create inode either due to permissions or some other
-		   something else is wrong. */
+		 * problem. In case r is EEXIST, we might be dealing with a
+		 * dangling symlink.*/
+		if (r == EEXIST) {
+			struct vnode *slp, *old_wd;
+
+			/* Resolve path up to symlink */
+			slp = advance(dirp, PATH_RET_SYMLINK);
+			if (slp != NIL_VNODE) {
+				if (S_ISLNK(slp->v_mode)) {
+					/* Get contents of link */
+
+					int max_linklen;
+					max_linklen = sizeof(user_fullpath)-1;
+					r = req_rdlink(slp->v_fs_e,
+						       slp->v_inode_nr,
+					   	       VFS_PROC_NR,
+					   	       user_fullpath,
+					   	       max_linklen);
+					if (r < 0) {
+						/* Failed to read link */
+						put_vnode(slp);
+						put_vnode(dirp);
+						err_code = r;
+						return(NIL_VNODE);
+					}
+					user_fullpath[r] = '\0';/* Term. path*/
+				} 
+				put_vnode(slp);
+			}
+
+			/* Try to create the inode the dangling symlink was
+			 * pointing to. We have to use dirp as starting point
+			 * as there might be multiple successive symlinks
+			 * crossing multiple mountpoints. */
+			old_wd = fp->fp_wd; /* Save orig. working dirp */
+			fp->fp_wd = dirp;
+			vp = new_node(oflags, bits);
+			fp->fp_wd = old_wd; /* Restore */
+
+			if (vp != NIL_VNODE) {
+				put_vnode(dirp);
+				return(vp);
+			}
+			r = err_code;
+		} 
+
+		if (r == EEXIST) 
+			err_code = EIO; /* Impossible, we have verified that
+					 * the last component doesn't exist and
+					 * is not a dangling symlink. */
+		else
+			err_code = r;
+
 		put_vnode(dirp);
-		err_code = r;
 		return(NIL_VNODE);
 	}
 	
@@ -280,9 +346,10 @@ PRIVATE struct vnode *new_node(mode_t bits)
   } else {
   	/* Either last component exists, or there is some other problem. */
   	if (vp != NIL_VNODE)
-  		r = EEXIST;
+  		r = EEXIST;	/* File exists or a symlink names a file while
+  				 * O_EXCL is set. */ 
   	else
-		r = err_code; 
+		r = err_code;	/* Other problem. */
   }
 
   err_code = r;

servers/vfs/request.c

@@ -149,10 +149,13 @@ node_details_t *res;
   size_t len;
   message m;
 
+  if (path[0] == '/')
+  	panic(__FILE__, "req_create: filename starts with '/'", NO_NUM);
+
   len = strlen(path) + 1;
   grant_id = cpf_grant_direct(fs_e, (vir_bytes) path, len, CPF_READ);
   if (grant_id == -1)
-	  panic(__FILE__, "req_create: cpf_grant_direct failed", NO_NUM);
+	panic(__FILE__, "req_create: cpf_grant_direct failed", NO_NUM);
 
   /* Fill in request message */
   m.m_type	= REQ_CREATE;

servers/vfs/request.h

@@ -15,9 +15,6 @@ typedef struct node_details {
 	uid_t uid;
 	gid_t gid;
 
-	/* For faster access */
-	unsigned short inode_index;
-
 	/* For char/block special files */
 	dev_t dev;
 } node_details_t;

test/test25.c

@@ -665,14 +665,25 @@ void test25e()
   if (fd != -1) e(40);
   if (errno != EEXIST) e(41);
 
+  /* open should fail when O_CREAT|O_EXCL are set and a symbolic link names
+     a file with EEXIST (regardless of link actually works or not) */
+  if (symlink("exists", "slinktoexists") == -1) e(42);
+  if (open("slinktoexists", O_RDWR | O_CREAT | O_EXCL, 0777) != -1) e(43);
+  if (unlink("exists") == -1) e(44);
+  /* "slinktoexists has become a dangling symlink. open(2) should still fail
+     with EEXIST */
+  if (open("slinktoexists", O_RDWR | O_CREAT | O_EXCL, 0777) != -1) e(45);
+  if (errno != EEXIST) e(46);
+  
+
   /* Test ToLongName and ToLongPath */
-  if ((fd = open(ToLongName, O_RDWR | O_CREAT, 0777)) != 3) e(45);
+  if ((fd = open(ToLongName, O_RDWR | O_CREAT, 0777)) != 3) e(47);
-  if (close(fd) != 0) e(46);
+  if (close(fd) != 0) e(48);
   ToLongPath[PATH_MAX - 2] = '/';
   ToLongPath[PATH_MAX - 1] = 'a';
-  if ((fd = open(ToLongPath, O_RDWR | O_CREAT, 0777)) != -1) e(47);
+  if ((fd = open(ToLongPath, O_RDWR | O_CREAT, 0777)) != -1) e(49);
-  if (errno != ENAMETOOLONG) e(48);
+  if (errno != ENAMETOOLONG) e(50);
-  if (close(fd) != -1) e(49);
+  if (close(fd) != -1) e(51);
   ToLongPath[PATH_MAX - 1] = '/';
 }
 

test/test48.c

@@ -537,7 +537,7 @@ static int can_use_network(void)
 	int status;
 
 	/* try to ping minix3.org */
-	status = system("ping www.minix3.org > /dev/nul 2>&1");
+	status = system("ping www.minix3.org > /dev/null 2>&1");
 	if (status == 127)
 	{
 		printf("cannot execute ping\n");