Re: Server recurses and cores on unexpected client connection loss

[David Sainty]

Some more notes:

It's probably a mistake to NULL the global pointers and not use buf_free() in 
my patch.  On the other hand, I don't think historically buf_free() has ever 
been called in the cleanup path.  My patch is consistent with this :)

It isn't entirely clear what the interface to the buffers is intended to be, 
but if a buffer is still meant to be "valid" after a buf_shutdown(), another 
approach would be to change buf_shutdown() so that it can be called multiple 
times without crashing.  An obvious way to do this is to NULL all the virtual 
function pointers once buf_shutdown() has completed.

If the above approach is taken (change the buffering API to be more forgiving), 
then my patch below doesn't need to be applied.  On reflection, I think I 
prefer this approach, because the buf_shutdown() method is not intended to 
destroy the object - so the object should remain "valid", and therefore it 
doesn't seem unreasonable to me that repeated buf_shutdown() calls should 
silently succeed.

Cheers,

Dave

>>> <david.sainty@ird.govt.nz> 29/04/2002 22:52:45 >>>

>Submitter-Id:   net
>Originator:     David Sainty
>Organization:
>Confidential:  no
>Synopsis:      Server recurses and cores on unexpected client connection loss
>Severity:      serious
>Priority:      medium
>Category:      cvs
>Class:         sw-bug
>Release:       1.11.2
>Environment:
System: SunOS dotp01 5.8 Generic_108528-03 sun4u sparc SUNW,Ultra-5_10
Architecture: sun4

>Description:

        CVS (questionably?) traps SIGPIPE and uses this to run various
        shutdown operations.  One of those operations is to call
        server_cleanup() [association added in server.c].  Another operation
        is to call main_cleanup() [association added in main.c].  However,
        main_cleanup() calls error(), which calls error_exit(), which calls
        server_cleanup().

        The (amusing, but painful) sequence of events is:

        1. client drops connection unexpectedly
        2. server receives SIGPIPE
        3. server_cleanup() is called, calls
             stdio_buffer_shutdown(buf_from_net)
        4. main_cleanup() is called, server_cleanup() is called, calls
             stdio_buffer_shutdown(buf_from_net).  'stdin' is closed.
        5. stdio_buffer_shutdown() does an assert [this is also a bug, asserts
           can be compiled out] on a positive result from fstat('stdin').
        6. fstat() fails on closed file descriptor, assert generates a
           SIGABORT.
        7. SIGABORT is trapped, and also calls server_cleanup() and
           main_cleanup().
        8. Process repeats 4554 times, at which point the stack is exhausted.
           Process cores.

>How-To-Repeat:
        It appears to be consistently repeatable simply by doing a 'cvs -d
        :pserver:user@host:/cvsroot/repository update -dP', and hitting ctrl-C
        in the middle of processing.

>Fix:
        The following patch avoids the core, and hasn't really been tested.
        However it looks closer to intention, I think that it can't really be
        any "worse" than current...

        Some points:

        - I only caught the cvs_outerr() change because it causes a core if
          omitted.  There may be other places where cvs is (erroneously)
          continuing to use buffers after they are potentially closed.

        - The assert() in buffer.c should be broken out to (at the VERY least)
          an if/abort.

        - Perhaps the SIGPIPE stuff could die entirely, it isn't the nicest
          way to handle a network connection shutdown cleanup :)

--- src/server.c.orig   Mon Apr 29 21:37:39 2002
+++ src/server.c        Mon Apr 29 22:43:07 2002
@@ -4885,11 +4885,17 @@
         * generated when the client shuts down its buffer.  Then, after we
         * have generated any final output, we shut down BUF_TO_NET.
         */
-
-       status = buf_shutdown (buf_from_net);
-       if (status != 0)
+       if (buf_from_net != NULL)
        {
-           error (0, status, "shutting down buffer from client");
+           /* Temporary used to protect against potential reentry */
+           struct buffer *tmp_buf_from_net = buf_from_net;
+           buf_from_net = NULL;
+           status = buf_shutdown (tmp_buf_from_net);
+
+           if (status != 0)
+           {
+               error (0, status, "shutting down buffer from client");
+           }
        }
     }
 
@@ -4898,7 +4904,13 @@
        if (buf_to_net != NULL)
        {
            (void) buf_flush (buf_to_net, 1);
-           (void) buf_shutdown (buf_to_net);
+
+           {
+               /* Temporary used to protect against potential reentry */
+               struct buffer *tmp_buf_to_net = buf_to_net;
+               buf_to_net = NULL;
+               (void) buf_shutdown (tmp_buf_to_net);
+           }
        }
        return;
     }
@@ -4998,8 +5010,13 @@
 
     if (buf_to_net != NULL)
     {
+       /* Temporary used to protect against potential reentry */
+       struct buffer *tmp_buf_to_net;
        (void) buf_flush (buf_to_net, 1);
-       (void) buf_shutdown (buf_to_net);
+
+       tmp_buf_to_net = buf_to_net;
+       buf_to_net = NULL;
+       (void) buf_shutdown (tmp_buf_to_net);
     }
 }
 
@@ -6484,8 +6501,13 @@
 #ifdef SERVER_SUPPORT
     if (error_use_protocol)
     {
-       buf_output (saved_outerr, str, len);
-       buf_copy_lines (buf_to_net, saved_outerr, 'E');
+       /* We expect this channel to be available.  If it isn't, just
+           drop the error... */
+       if (buf_to_net != NULL)
+       {
+           buf_output (saved_outerr, str, len);
+           buf_copy_lines (buf_to_net, saved_outerr, 'E');
+       }
     }
     else if (server_active)
     {