> -----Original Message-----
> From: Hyrum K. Wright [mailto:hyrum_at_hyrumwright.org]
> Sent: woensdag 6 mei 2009 18:31
> To: svn_at_subversion.tigris.org
> Subject: svn commit: r37615 - trunk/subversion/libsvn_ra_svn
>
> Author: hwright
> Date: Wed May 6 09:31:28 2009
> New Revision: 37615
>
> Log:
> A better fix to issue #2580.
>
> Fix ssh zombie problem introduced with revision 35533
>
> * subversion/libsvn_ra_svn/client.c
> (detach_child_cleanup): New.
> (make_tunnel): Fully detach tunnel process to avoid having it receive
> signals
> while restoring the original apr_pool_note_subprocess to avoid
> creating
> zombies.
>
> Patch by: Kyle McKay <mackyle_at_gmail.com>
>
> Modified:
> trunk/subversion/libsvn_ra_svn/client.c
>
> Modified: trunk/subversion/libsvn_ra_svn/client.c
> URL:
> http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_ra_svn/client.
> c?pathrev=37615&r1=37614&r2=37615
> =======================================================================
> =======
> --- trunk/subversion/libsvn_ra_svn/client.c Wed May 6 09:02:16 2009
> (r37614)
> +++ trunk/subversion/libsvn_ra_svn/client.c Wed May 6 09:31:28 2009
> (r37615)
> @@ -26,6 +26,12 @@
> #include <apr_strings.h>
> #include <apr_network_io.h>
> #include <apr_uri.h>
> +#if APR_HAVE_STDLIB_H
> +#include <stdlib.h>
> +#endif
> +#if APR_HAVE_UNISTD_H
> +#include <unistd.h>
> +#endif
>
> #include "svn_types.h"
> #include "svn_string.h"
> @@ -426,6 +432,43 @@ static void handle_child_process_error(a
> svn_error_clear(svn_ra_svn_flush(conn, pool));
> }
>
> +static apr_status_t detach_child_cleanup(void *data)
> +{
> +#if APR_HAVE_STDLIB_H
> +#if APR_HAVE_UNISTD_H
> +#if APR_HAS_FORK
I don't think we need the two outer #if's. (We only need the check for
fork()).
These defines check whether a specific header file exist and not if
functionality exist.
Using these checks is most likely a dumb guess on whether this is running on
a Unix system. (stdlib.h is available on any operating system I know and
probably part of recent C standards).
Bert
> + /* Using apr_procattr_detach_set has a number of undesirable
> + * side effects:
> + * - Redirecting stdin/stdout/stderr to /dev/null
> + * - Detaching from the controlling terminal
> + *
> + * We can workaround the redirections with some calls to
> + * dup/dup2/fclose in a child cleanup function. However
> + * that ends up leaving the child with 3 extra open file
> + * handles to /dev/null -- they're not used, but it's not
> + * clean.
> + *
> + * Unfortunately the side effect of detaching from the
> + * controlling terminal is that any tunnel processes that
> + * may need to prompt for a password will get confused
> + * (i.e. ssh). So instead we just do the detach ourselves
> + * here and avoid using apr_procattr_detach_set.
> + *
> + * Since child_cleanup functions only run on systems with
> + * fork this code is surrounded with the appropriate
> + * #ifdefs. On systems without fork, the code here
> + * isn't needed anyway.
> + */
> + int x;
> + if ((x = fork()) > 0) {
> + exit(0);
> + }
> +#endif
> +#endif
> +#endif
> + return APR_SUCCESS;
> +}
> +
> /* (Note: *CONN is an output parameter.) */
> static svn_error_t *make_tunnel(const char **args, svn_ra_svn_conn_t
> **conn,
> apr_pool_t *pool)
> @@ -433,6 +476,7 @@ static svn_error_t *make_tunnel(const ch
> apr_status_t status;
> apr_proc_t *proc;
> apr_procattr_t *attr;
> + apr_pool_t *temp_pool = NULL;
>
> status = apr_procattr_create(&attr, pool);
> if (status == APR_SUCCESS)
> @@ -441,12 +485,43 @@ static svn_error_t *make_tunnel(const ch
> status = apr_procattr_cmdtype_set(attr, APR_PROGRAM_PATH);
> if (status == APR_SUCCESS)
> status = apr_procattr_child_errfn_set(attr,
> handle_child_process_error);
> + if (status == APR_SUCCESS)
> + status = apr_pool_create(&temp_pool, NULL);
> + if (status == APR_SUCCESS)
> + apr_pool_cleanup_register(temp_pool, NULL, apr_pool_cleanup_null,
> + detach_child_cleanup);
> proc = apr_palloc(pool, sizeof(*proc));
> if (status == APR_SUCCESS)
> status = apr_proc_create(proc, *args, args, NULL, attr, pool);
> + if (temp_pool)
> + apr_pool_destroy(temp_pool);
> if (status != APR_SUCCESS)
> return svn_error_wrap_apr(status, _("Can't create tunnel"));
>
> + /* Arrange for the tunnel agent to get a SIGKILL on pool
> + * cleanup. This is a little extreme, but the alternatives
> + * weren't working out:
> + * - Closing the pipes and waiting for the process to die
> + * was prone to mysterious hangs which are difficult to
> + * diagnose (e.g. svnserve dumps core due to unrelated bug;
> + * sshd goes into zombie state; ssh connection is never
> + * closed; ssh never terminates).
> + * - Killing the tunnel agent with SIGTERM leads to unsightly
> + * stderr output from ssh.
> + * Since we now run the tunnel detached, this is the best way
> + * to avoid zombies without adding any wait delays. The actual
> + * tunnel itself won't get the signal because it will be detached
> + * by then.
> + * Don't call apr_pool_note_subprocess if WIN32 is defined as
> + * there is no zombie problem there but we DO want the child
> + * to still be allowed to "die in piece, in its own time, on
> + * its own terms." See:
> + * http://subversion.tigris.org/issues/show_bug.cgi?id=2580
> + */
> +#ifndef WIN32
> + apr_pool_note_subprocess(pool, proc, APR_KILL_ALWAYS);
> +#endif
> +
> /* APR pipe objects inherit by default. But we don't want the
> * tunnel agent's pipes held open by future child processes
> * (such as other ra_svn sessions), so turn that off. */
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=495&dsMessageI
> d=2082454
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2085521
Received on 2009-05-07 00:16:36 CEST