Patch: CVS can infinitely recurse on assertion failure during cleanup

[Jonathan Kamens]

If an assertion fails inside a CVS process while it's cleaning up, it
can recurse forever (well, until it grows so large that the kernel
decides to kill it because there's no more memory) inside the cleanup
function.  The assertion failure calls abort, which sends SIGABRT to
the process, which causes the cleanup handler to be called, which
causes the assertion failure, which causes abort, etc.

I duplicated this problem on Linux (both client and server) as follows
in cvs 1.11.2 with logmsg.c (do_verify) modified to allow empty log
messages but with no other changes:

* Using a remote repository with CVS_RSH set to ssh, run "cvs commit".
* Specify an empty log message.
* When it asks what to do, say "c" for continue.
* It'll start an editor on the server.
* Send a TERM signal to the parent "cvs server" process.
* Send a TERM signal to the client "cvs commit" process.

The parent "cvs server" process will start growing as soon as you send
the TERM signal to the "cvs commit" process.  (More on the particular
failure mode in a minute.)

The patch below avoids this problem by deregistering SIGABRT at the
beginning of each of the cleanup handlers.  This means that in the
unlikely even that an assertion failure occurs, cleanup will be
incomplete, but this is preferable to infinite recursion hosing the
machine.

Now, about the failure mode.... After you send the TERM signal to the
parent "cvs server" process, it get stuck inside getc inside
stdio_buffer_shutdown at line 1396 of buffer.c.  It's trying to read a
character from the client, but the client has nothing to say to it
(it's waiting for the commit that it submitted to finish).  I'm not
sure what the correct fix here is; perhaps stdio_buffer_shutdown should
set the buffer to nonblocking mode before checking to see if there is
data pending on it?

When you subsequently send the TERM signal to the "cvs client", the
assertion that fails (over and over, without my fix) is the one right
at the beginning of stdio_buffer_shutdown.  It's getting called again
for some reason I do not know.

I believe that the patch below for preventing infinite recursion in
cleanup procedures should be applied independent of whether or how the
stdio_buffer_shutdown problem described above is fixed.

The diff to patch.c may apply with an offset because of other changes
(previously submitted to you) I have in the file.

By the way, should I be filing my bug reports and patches directly
into the Issuezilla database on cvshome.org?

Thanks,

  jik

2002-06-04  Jonathan Kamens  <jik@kamens.brookline.ma.us>

        * main.c (main_cleanup): Deregister the SIGABRT handle, so that if
        we do something while cleaning up that causes an abort (e.g., a
        failed assertion), we won't infinitely recurse in the cleanup
        function.
        * patch.c (patch_cleanup): Ditto.
        * rcs.c (rcs_cleanup): Ditto.
        * server.c (server_cleanup): Ditto.

Index: main.c
===================================================================
RCS file: /cvsroot/ccvs/src/main.c,v
retrieving revision 1.171
diff -u -r1.171 main.c
--- main.c      20 May 2002 17:33:55 -0000      1.171
+++ main.c      4 Jun 2002 14:16:37 -0000
@@ -346,6 +346,13 @@
     const char *name;
     char temp[10];
 
+#ifdef SIGABRT
+    /* Need to deregister the SIGABRT handler so that if an assertion
+       fails and calls abort while we're cleaning up, we won't
+       infinitely recurse in the cleanup function. */
+    SIG_deregister(SIGABRT, main_cleanup);
+#endif
+
     switch (sig)
     {
 #ifdef SIGABRT
Index: patch.c
===================================================================
RCS file: /cvsroot/ccvs/src/patch.c,v
retrieving revision 1.78
diff -u -r1.78 patch.c
--- patch.c     7 Aug 2001 15:35:32 -0000       1.78
+++ patch.c     4 Jun 2002 14:16:37 -0000
@@ -774,6 +791,15 @@
     /* Note that the checks for existence_error are because we are
        called from a signal handler, without SIG_begincrsect, so
        we don't know whether the files got created.  */
+
+#ifndef DONT_USE_SIGNALS
+#ifdef SIGABRT
+    /* Need to deregister the SIGABRT handler so that if an assertion
+       fails and calls abort while we're cleaning up, we won't
+       infinitely recurse in the cleanup function. */
+    SIG_deregister(SIGABRT, patch_cleanup);
+#endif
+#endif /* !DONT_USE_SIGNALS */
 
     if (tmpfile1 != NULL)
     {
Index: rcs.c
===================================================================
RCS file: /cvsroot/ccvs/src/rcs.c,v
retrieving revision 1.260
diff -u -r1.260 rcs.c
--- rcs.c       31 May 2002 19:12:45 -0000      1.260
+++ rcs.c       4 Jun 2002 14:16:38 -0000
@@ -8234,6 +8234,15 @@
        called from a signal handler, so we don't know whether the
        files got created.  */
 
+#ifndef DONT_USE_SIGNALS
+#ifdef SIGABRT
+    /* Need to deregister the SIGABRT handler so that if an assertion
+       fails and calls abort while we're cleaning up, we won't
+       infinitely recurse in the cleanup function. */
+    SIG_deregister(SIGABRT, rcs_cleanup);
+#endif
+#endif /* !DONT_USE_SIGNALS */
+
     /* FIXME: Do not perform buffered I/O from an interrupt handler like
        this (via error).  However, I'm leaving the error-calling code there
        in the hope that on the rare occasion the error call is actually made
Index: server.c
===================================================================
RCS file: /cvsroot/ccvs/src/server.c,v
retrieving revision 1.274
diff -u -r1.274 server.c
--- server.c    3 May 2002 17:20:48 -0000       1.274
+++ server.c    4 Jun 2002 14:16:39 -0000
@@ -4871,6 +4871,15 @@
     int status;
     int save_noexec;
 
+#ifndef DONT_USE_SIGNALS
+#ifdef SIGABRT
+    /* Need to deregister the SIGABRT handler so that if an assertion
+       fails and calls abort while we're cleaning up, we won't
+       infinitely recurse in the cleanup function. */
+    SIG_deregister(SIGABRT, server_cleanup);
+#endif
+#endif /* !DONT_USE_SIGNALS */
+
     if (buf_to_net != NULL)
     {
        /* Since we're done, go ahead and put BUF_TO_NET back into blocking