svn commit: r1664927 - /subversion/trunk/subversion/libsvn_ra_serf/serf.c

[rh...@apache.org]

Author: rhuijben
Date: Sat Mar  7 22:13:26 2015
New Revision: 1664927

URL: http://svn.apache.org/r1664927
Log:
Following up on r1664078, reinstate a subpool requirement for serf to avoid a
segfault in basic tests caused by a bug in serf 1.3.x... by converting the code
to behave like good dual pool code.

This problem is fixed on serf trunk, but the fix is

* subversion/libsvn_ra_serf/serf.c
  (svn_ra_serf__open): Remove private session pool that breaks the cleanup
    rules (making scratch pool a sibling to result pool). Update documentation.
    Add assertion.

Modified:
    subversion/trunk/subversion/libsvn_ra_serf/serf.c

Modified: subversion/trunk/subversion/libsvn_ra_serf/serf.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/serf.c?rev=1664927&r1=1664926&r2=1664927&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/serf.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/serf.c Sat Mar  7 22:13:26 2015
@@ -484,15 +484,14 @@ svn_ra_serf__open(svn_ra_session_t *sess
   apr_uri_t url;
   const char *client_string = NULL;
   svn_error_t *err;
-  apr_pool_t *pool = result_pool;
 
   if (corrected_url)
     *corrected_url = NULL;
 
-  serf_sess = apr_pcalloc(pool, sizeof(*serf_sess));
-  serf_sess->pool = svn_pool_create(pool);
+  serf_sess = apr_pcalloc(result_pool, sizeof(*serf_sess));
+  serf_sess->pool = result_pool;
   if (config)
-    SVN_ERR(svn_config_copy_config(&serf_sess->config, config, pool));
+    SVN_ERR(svn_config_copy_config(&serf_sess->config, config, result_pool));
   else
     serf_sess->config = NULL;
   serf_sess->wc_callbacks = callbacks;
@@ -554,13 +553,16 @@ svn_ra_serf__open(svn_ra_session_t *sess
 
   /* create the user agent string */
   if (callbacks->get_client_string)
-    SVN_ERR(callbacks->get_client_string(callback_baton, &client_string, pool));
+    SVN_ERR(callbacks->get_client_string(callback_baton, &client_string,
+                                         scratch_pool));
 
   if (client_string)
-    serf_sess->useragent = apr_pstrcat(pool, get_user_agent_string(pool), " ",
+    serf_sess->useragent = apr_pstrcat(result_pool,
+                                       get_user_agent_string(scratch_pool),
+                                       " ",
                                        client_string, SVN_VA_NULL);
   else
-    serf_sess->useragent = get_user_agent_string(pool);
+    serf_sess->useragent = get_user_agent_string(result_pool);
 
   /* go ahead and tell serf about the connection. */
   status =
@@ -581,16 +583,24 @@ svn_ra_serf__open(svn_ra_session_t *sess
 
   session->priv = serf_sess;
 
-  /* This subpool not only avoids having a lot of temporary state in the long
-     living session pool, but it also works around a bug in serf
-     <= r2319 / 1.3.4 where serf doesn't report the request as failed/cancelled
-     when the authorization request handler fails to handle the request.
-
-     In this specific case the serf connection is cleaned up by the pool
-     handlers before our handler is cleaned up (via subpools). Using a
-     subpool here cleans up our handler before the connection is cleaned. */
+  /* The following code explicitly works around a bug in serf <= r2319 / 1.3.8
+     where serf doesn't report the request as failed/cancelled when the
+     authorization request handler fails to handle the request.
+
+     As long as we allocate the request in a subpool of the serf connection
+     pool, we know that the handler is always cleaned before the connection.
+
+     Luckily our caller now passes us two pools which handle this case.
+   */
+#if defined(SVN_DEBUG) && !SERF_VERSION_AT_LEAST(1,4,0)
+  /* Currently ensured by svn_ra_open4().
+     If failing causes segfault in basic_tests.py 48, "basic auth test" */
+  SVN_ERR_ASSERT((serf_sess->pool != scratch_pool)
+                 && apr_pool_is_ancestor(serf_sess->pool, scratch_pool));
+#endif
+
   err = svn_ra_serf__exchange_capabilities(serf_sess, corrected_url,
-                                           pool, scratch_pool);
+                                            result_pool, scratch_pool);
 
   /* serf should produce a usable error code instead of APR_EGENERAL */
   if (err && err->apr_err == APR_EGENERAL)