Fix race in pipe read code whereby a blocked lock can allow another

    process to sneak in and write to or close the pipe.  The read code
    enters a 'piperd' state after doing the lock operation without
    checking to see if the state changed, which can cause the process
    to wait forever.

    The code has also been documented more.

1 files changed, 39 insertions, 20 deletions

diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c
index 66ddd50..396538a 100644
--- a/sys/kern/sys_pipe.c
+++ b/sys/kern/sys_pipe.c
@@ -16,7 +16,7 @@
  * 4. Modifications may be freely made to this file if the above conditions
  *    are met.
  *
- * $Id: sys_pipe.c,v 1.48 1999/01/27 21:49:57 dillon Exp $
+ * $Id: sys_pipe.c,v 1.49 1999/01/28 00:57:47 dillon Exp $
  */
 
 /*
@@ -381,12 +381,39 @@ pipe_read(fp, uio, cred)
 #endif
 		} else {
 			/*
+			 * If there is no more to read in the pipe, reset
+			 * its pointers to the beginning.  This improves
+			 * cache hit stats.
+			 *
+			 * We get this over with now because it may block
+			 * and cause the state to change out from under us,
+			 * rather then have to re-test the state both before
+			 * and after this fragment.
+			 */
+		
+			if ((error = pipelock(rpipe,1)) == 0) {
+				if (rpipe->pipe_buffer.cnt == 0) {
+					rpipe->pipe_buffer.in = 0;
+					rpipe->pipe_buffer.out = 0;
+				}
+				pipeunlock(rpipe);
+
+				/*
+				 * If pipe filled up due to pipelock
+				 * blocking, loop back up.
+				 */
+				if (rpipe->pipe_buffer.cnt > 0)
+					continue;
+			}
+
+			/*
 			 * detect EOF condition
 			 */
 			if (rpipe->pipe_state & PIPE_EOF) {
 				/* XXX error = ? */
 				break;
 			}
+
 			/*
 			 * If the "write-side" has been blocked, wake it up now.
 			 */
@@ -394,34 +421,26 @@ pipe_read(fp, uio, cred)
 				rpipe->pipe_state &= ~PIPE_WANTW;
 				wakeup(rpipe);
 			}
-			if (nread > 0)
+
+			/*
+			 * break if error (signal via pipelock), or if some 
+			 * data was read
+			 */
+			if (error || nread > 0)
 				break;
 
+			/*
+			 * Handle non-blocking mode operation
+			 */
+
 			if (fp->f_flag & FNONBLOCK) {
 				error = EAGAIN;
 				break;
 			}
 
 			/*
-			 * If there is no more to read in the pipe, reset
-			 * its pointers to the beginning.  This improves
-			 * cache hit stats.
+			 * Wait for more data
 			 */
-		
-			if ((error = pipelock(rpipe,1)) == 0) {
-				if (rpipe->pipe_buffer.cnt == 0) {
-					rpipe->pipe_buffer.in = 0;
-					rpipe->pipe_buffer.out = 0;
-				}
-				pipeunlock(rpipe);
-			} else {
-				break;
-			}
-
-			if (rpipe->pipe_state & PIPE_WANTW) {
-				rpipe->pipe_state &= ~PIPE_WANTW;
-				wakeup(rpipe);
-			}
 
 			rpipe->pipe_state |= PIPE_WANTR;
 			if ((error = tsleep(rpipe, PRIBIO|PCATCH, "piperd", 0)) != 0) {