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)