Re: Possible race in pserver leading to broken pipe error?

[Derek Robert Price]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Derek Robert Price wrote:

> I've attempted to rewrite this patch, and attached it, still broken,
> only because I may have to leave the office in a few minutes.
> remotecheck is curerntly failing with this patch because STDOUT isn't
> flushing, it looks like.


Yep, it just needed fflushes.  Working patch attached.  Comments before
I commit?

Derek

Index: src/ChangeLog
===================================================================
RCS file: /cvs/ccvs/src/ChangeLog,v
retrieving revision 1.2336.2.176
diff -u -r1.2336.2.176 ChangeLog
- --- src/ChangeLog    10 Feb 2004 19:54:59 -0000    1.2336.2.176
+++ src/ChangeLog    10 Feb 2004 21:56:50 -0000
@@ -1,5 +1,15 @@
 2004-02-10  Derek Price  <address@hidden>
 
+    * server.c (do_cvs_command): Have the server child close all the pipes
+    but the flow control pipe and wait on an EOF on the flow control pipe
+    from the parent when done to avoid a race condition that could
+    otherwise generate a SIGPIPE for the parent before the SIGCHILD when
+    the other pipes were so full after a child exited that the parent
+    attempted to write a stop byte to the flow control pipe.
+    (Original report from <address@hidden>.)
+
+2004-02-10  Derek Price  <address@hidden>
+
     * buffer.c (stdio_buffer_shutdown): Add a helpful comment.
 
 2004-02-09  Derek Price  <address@hidden>
Index: src/server.c
===================================================================
RCS file: /cvs/ccvs/src/server.c,v
retrieving revision 1.284.2.15
diff -u -r1.284.2.15 server.c
- --- src/server.c    2 Feb 2004 15:12:11 -0000    1.284.2.15
+++ src/server.c    10 Feb 2004 21:56:54 -0000
@@ -2293,13 +2293,18 @@
 #  define SERVER_LO_WATER (1 * 1024 * 1024)
 # endif /* SERVER_LO_WATER */
 
+
+
+/* Protos */
 static int set_nonblock_fd PROTO((int));
+static int set_block_fd PROTO((int));
+
+
 
 /*
- - * Set buffer BUF to non-blocking I/O.  Returns 0 for success or errno
+ * Set buffer FD to non-blocking I/O.  Returns 0 for success or errno
  * code.
  */
- -
 static int
 set_nonblock_fd (fd)
      int fd;
@@ -2314,8 +2319,29 @@
     return 0;
 }
 
+
+
+/*
+ * Set buffer FD to non-blocking I/O.  Returns 0 for success or errno
+ * code.
+ */
+static int
+set_block_fd (fd)
+     int fd;
+{
+    int flags;
+
+    flags = fcntl (fd, F_GETFL, 0);
+    if (flags < 0)
+    return errno;
+    if (fcntl (fd, F_SETFL, flags & ~O_NONBLOCK) < 0)
+    return errno;
+    return 0;
+}
 #endif /* SERVER_FLOWCONTROL */
- -
+
+
+
 static void serve_questionable PROTO((char *));
 
 static void
@@ -2792,11 +2818,36 @@
     /* For now we just discard partial lines on stderr.  I suspect
        that CVS can't write such lines unless there is a bug.  */
 
- -    /*
- -     * When we exit, that will close the pipes, giving an EOF to
- -     * the parent.
- -     */
     buf_free (protocol);
+
+    /* Close the pipes explicitly in order to send an EOF to the parent,
+     * then wait for the parent to close the flow control pipe.  This
+     * avoids a race condition where a child which dumped more than the
+     * high water mark into the pipes could complete its job and exit,
+     * leaving the parent process to attempt to write a stop byte to the
+     * closed flow control pipe, which earned the parent a SIGPIPE, which
+     * it normally only expects on the network pipe and that causes it to
+     * exit with an error message, rather than the SIGCHILD that it knows
+     * how to handle correctly.
+     */
+    /* Let exit() close STDIN - it's from /dev/null anyhow.  */
+    fflush (stderr);
+    close (STDERR_FILENO);
+    fflush (stdout);
+    close (STDOUT_FILENO);
+    fflush (fdopen (protocol_pipe[1], "w"));
+    close (protocol_pipe[1]);
+#ifdef SERVER_FLOWCONTROL
+    if (set_block_fd (flowcontrol_pipe[0]) == 0)
+    {
+      char junk;
+      while (read (flowcontrol_pipe[0], &junk, 1) != 0);
+    }
+    /* FIXCVS: No point in printing an error message with error(),
+     * as STDERR is already closed, but perhaps this could be syslogged?
+     */
+#endif
+
     exit (exitstatus);
     }


- --
                *8^)

Email: address@hidden

Get CVS support at <http://ximbiot.com>!
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)
Comment: Using GnuPG with Netscape - http://enigmail.mozdev.org

iD8DBQFAKVQ2LD1OTBfyMaQRAiHuAKC9AQc7kkqaVLHB/X1xEle0dZDAeQCggj6/
Lh8JPuu2Gr4UV6NN2GaytAg=
=5rDP
-----END PGP SIGNATURE-----