This patchset fixes a large number of file descriptor race conditions.

    Pre-rfork code assumed inherent locking of a process's file descriptor
    array.  However, with the advent of rfork() the file descriptor table
    could be shared between processes.  This patch closes over a dozen
    serious race conditions related to one thread manipulating the table
    (e.g. closing or dup()ing a descriptor) while another is blocked in
    an open(), close(), fcntl(), read(), write(), etc...

PR: kern/11629
Discussed with: Alexander Viro <viro@math.psu.edu>

1 files changed, 75 insertions, 10 deletions

diff --git a/sys/kern/vfs_extattr.c b/sys/kern/vfs_extattr.c
index c24d227..7cf4663 100644
--- a/sys/kern/vfs_extattr.c
+++ b/sys/kern/vfs_extattr.c
@@ -71,6 +71,7 @@
 #include <vm/vm.h>
 #include <vm/vm_object.h>
 #include <vm/vm_zone.h>
+#include <vm/vm_page.h>
 
 static int change_dir __P((struct nameidata *ndp, struct proc *p));
 static void checkdirs __P((struct vnode *olddp));
@@ -996,25 +997,65 @@ open(p, uap)
 	cmode = ((SCARG(uap, mode) &~ fdp->fd_cmask) & ALLPERMS) &~ S_ISTXT;
 	NDINIT(&nd, LOOKUP, FOLLOW, UIO_USERSPACE, SCARG(uap, path), p);
 	p->p_dupfd = -indx - 1;			/* XXX check for fdopen */
+	/*
+	 * Bump the ref count to prevent another process from closing
+	 * the descriptor while we are blocked in vn_open()
+	 */
+	fhold(fp);
 	error = vn_open(&nd, &flags, cmode);
 	if (error) {
-		ffree(fp);
+		/*
+		 * release our own reference
+		 */
+		fdrop(fp, p);
+
+		/*
+		 * handle special fdopen() case.  bleh.  dupfdopen() is
+		 * responsible for dropping the old contents of ofiles[indx]
+		 * if it succeeds.
+		 */
 		if ((error == ENODEV || error == ENXIO) &&
 		    p->p_dupfd >= 0 &&			/* XXX from fdopen */
 		    (error =
-			dupfdopen(fdp, indx, p->p_dupfd, flags, error)) == 0) {
+			dupfdopen(p, fdp, indx, p->p_dupfd, flags, error)) == 0) {
 			p->p_retval[0] = indx;
 			return (0);
 		}
+		/*
+		 * Clean up the descriptor, but only if another thread hadn't
+		 * replaced or closed it.
+		 */
+		if (fdp->fd_ofiles[indx] == fp) {
+			fdp->fd_ofiles[indx] = NULL;
+			fdrop(fp, p);
+		}
+
 		if (error == ERESTART)
 			error = EINTR;
-		fdp->fd_ofiles[indx] = NULL;
 		return (error);
 	}
 	p->p_dupfd = 0;
 	NDFREE(&nd, NDF_ONLY_PNBUF);
 	vp = nd.ni_vp;
 
+	/*
+	 * There should be 2 references on the file, one from the descriptor
+	 * table, and one for us.
+	 *
+	 * Handle the case where someone closed the file (via its file
+	 * descriptor) while we were blocked.  The end result should look
+	 * like opening the file succeeded but it was immediately closed.
+	 */
+	if (fp->f_count == 1) {
+		KASSERT(fdp->fd_ofiles[indx] != fp,
+		    ("Open file descriptor lost all refs"));
+		VOP_UNLOCK(vp, 0, p);
+		vn_close(vp, flags & FMASK, fp->f_cred, p);
+		fdrop(fp, p);
+		p->p_retval[0] = indx;
+		return 0;
+	}
+
 	fp->f_data = (caddr_t)vp;
 	fp->f_flag = flags & FMASK;
 	fp->f_ops = &vnops;
@@ -1051,12 +1092,19 @@ open(p, uap)
 	/* assert that vn_open created a backing object if one is needed */
 	KASSERT(!vn_canvmio(vp) || VOP_GETVOBJECT(vp, NULL) == 0,
 		("open: vmio vnode has no backing object after vn_open"));
+	/*
+	 * Release our private reference, leaving the one associated with
+	 * the descriptor table intact.
+	 */
+	fdrop(fp, p);
 	p->p_retval[0] = indx;
 	return (0);
 bad:
-	(void) vn_close(vp, fp->f_flag, fp->f_cred, p);
-	ffree(fp);
-	fdp->fd_ofiles[indx] = NULL;
+	if (fdp->fd_ofiles[indx] == fp) {
+		fdp->fd_ofiles[indx] = NULL;
+		fdrop(fp, p);
+	}
+	fdrop(fp, p);
 	return (error);
 }
 
@@ -3394,6 +3442,12 @@ fhopen(p, uap)
 	if ((error = falloc(p, &nfp, &indx)) != 0)
 		goto bad;
 	fp = nfp;	
+
+	/*
+	 * Hold an extra reference to avoid having fp ripped out 
+	 * from under us while we block in the lock op
+	 */
+	fhold(fp);
 	nfp->f_data = (caddr_t)vp;
 	nfp->f_flag = fmode & FMASK;
 	nfp->f_ops = &vnops;
@@ -3411,10 +3465,20 @@ fhopen(p, uap)
 			type |= F_WAIT;
 		VOP_UNLOCK(vp, 0, p);
 		if ((error = VOP_ADVLOCK(vp, (caddr_t)fp, F_SETLK, &lf, type)) != 0) {
-			(void) vn_close(vp, fp->f_flag, fp->f_cred, p);
-			ffree(fp);
-			fdp->fd_ofiles[indx] = NULL;
-			return (error);
+			/*
+			 * The lock request failed.  Normally close the
+			 * descriptor but handle the case where someone might
+			 * have dup()d or close()d it when we weren't looking.
+			 */
+			if (fdp->fd_ofiles[indx] == fp) {
+				fdp->fd_ofiles[indx] = NULL;
+				fdrop(fp, p);
+			}
+			/*
+			 * release our private reference
+			 */
+			fdrop(fp, p);
+			return(error);
 		}
 		vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, p);
 		fp->f_flag |= FHASLOCK;
@@ -3423,6 +3487,7 @@ fhopen(p, uap)
 		vfs_object_create(vp, p, p->p_ucred);
 
 	VOP_UNLOCK(vp, 0, p);
+	fdrop(fp, p);
 	p->p_retval[0] = indx;
 	return (0);