>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) { _______________________________________________ Bug-cvs mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/bug-cvs