On Wed, Sep 19, 2012 at 3:29 PM, Ivan Zhakov <[email protected]> wrote:
> On Wed, Sep 19, 2012 at 3:22 PM, Philip Martin
> <[email protected]> wrote:
>> Roderich Schupp <[email protected]> writes:
>>
>>>> Since this definition is in include/mod_authz_svn.h (i.e. it's a public
>>>> interface)
>>>
>>> Perhaps one should also bump #define AUTHZ_SVN__SUBREQ_BYPASS_PROV_VER
>>> in include/mod_authz_svn.h
>>>
>>> Cheers, Roderich
>>>
>>> --- subversion-1.7.5.orig/subversion/include/mod_authz_svn.h 2009-11-16
>>> 20:07:17.000000000 +0100
>>> +++ subversion-1.7.5/subversion/include/mod_authz_svn.h 2012-09-19
>>> 10:48:27.662049000 +0200
>>> @@ -41,7 +41,8 @@
>>> #define AUTHZ_SVN__SUBREQ_BYPASS_PROV_VER "00.00a"
>>> typedef int (*authz_svn__subreq_bypass_func_t)(request_rec *r,
>>> const char *repos_path,
>>> - const char *repos_name);
>>> + const char *repos_name,
>>> + apr_pool_t *pool);
>>>
>>> #ifdef __cplusplus
>>> }
>>
>> I'm not sure what our policy is here. I think we do have to bump
>> AUTHZ_SVN__SUBREQ_BYPASS_PROV_VER but I don't know whether we would have
>> to provide the both the old and new interfaces in parallel. I suppose
>> it is possible to provide the old interface and implement it by calling
>> the new interface and passing r->pool.
>>
>> The pool parameter should probably be called scratch_pool to indicate
>> that it will not persist.
>>
> I suggest following solution for this issue:
> 1. Rename subreq_bypass to subreq_bypass2 and add scratch_pool parameter.
> 2. Call subreq_bypass2 from subreq_bypass with temporary pool
> 3. Use scratch_pool in subreq_bypass2 instead of r->pool in all places.
> 4. Bump AUTHZ_SVN__SUBREQ_BYPASS_PROV_VER and export subreq_bypass2
> 5. Call AUTHZ_SVN__SUBREQ_BYPASS_PROV_VER2 if available in mod_dav_svn.
>
> Backport changes 1-3 to svn 1.7.x, 4-5 leave on trunk.
>
Here is the patch to address 1-3:
[[[
Fix unbounded memory usage in mod_authz_svn with short_circuit enabled.
* subversion/mod_authz_svn/mod_authz_svn.c
(): Include svn_pools.h.
(get_access_conf, get_username_to_authorize): Add scratch_pool parameter
and use it for temporary data.
(req_check_access): Pass r->pool as scratch_pool to
get_username_to_authorize() and get_access_conf().
(subreq_bypass2): New. Implementation extracted from subreq_bypass.
(subreq_bypass): Create scratch_pool and call subreq_bypass2.
]]]
I'm ready to commit it, but additional review would be helpful.
--
Ivan Zhakov
Index: subversion/mod_authz_svn/mod_authz_svn.c
===================================================================
--- subversion/mod_authz_svn/mod_authz_svn.c (revision 1383873)
+++ subversion/mod_authz_svn/mod_authz_svn.c (working copy)
@@ -44,6 +44,7 @@
#include "svn_config.h"
#include "svn_string.h"
#include "svn_repos.h"
+#include "svn_pools.h"
#include "svn_dirent_uri.h"
#include "private/svn_fspath.h"
@@ -164,7 +165,7 @@
* Get the, possibly cached, svn_authz_t for this request.
*/
static svn_authz_t *
-get_access_conf(request_rec *r, authz_svn_config_rec *conf)
+get_access_conf(request_rec *r, authz_svn_config_rec *conf, apr_pool_t
*scratch_pool)
{
const char *cache_key = NULL;
const char *access_file;
@@ -182,7 +183,7 @@
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, "%s", dav_err->desc);
return NULL;
}
- access_file = svn_dirent_join_many(r->pool, repos_path, "conf",
+ access_file = svn_dirent_join_many(scratch_pool, repos_path, "conf",
conf->repo_relative_access_file,
NULL);
}
@@ -194,7 +195,7 @@
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
"Path to authz file is %s", access_file);
- cache_key = apr_pstrcat(r->pool, "mod_authz_svn:",
+ cache_key = apr_pstrcat(scratch_pool, "mod_authz_svn:",
access_file, (char *)NULL);
apr_pool_userdata_get(&user_data, cache_key, r->connection->pool);
access_conf = user_data;
@@ -243,12 +244,13 @@
/* Return the username to authorize, with case-conversion performed if
CONF->force_username_case is set. */
static char *
-get_username_to_authorize(request_rec *r, authz_svn_config_rec *conf)
+get_username_to_authorize(request_rec *r, authz_svn_config_rec *conf,
+ apr_pool_t *pool)
{
char *username_to_authorize = r->user;
if (username_to_authorize && conf->force_username_case)
{
- username_to_authorize = apr_pstrdup(r->pool, r->user);
+ username_to_authorize = apr_pstrdup(pool, r->user);
convert_case(username_to_authorize,
strcasecmp(conf->force_username_case, "upper") == 0);
}
@@ -283,7 +285,8 @@
svn_authz_t *access_conf = NULL;
svn_error_t *svn_err;
char errbuf[256];
- const char *username_to_authorize = get_username_to_authorize(r, conf);
+ const char *username_to_authorize = get_username_to_authorize(r, conf,
+ r->pool);
switch (r->method_number)
{
@@ -419,7 +422,7 @@
}
/* Retrieve/cache authorization file */
- access_conf = get_access_conf(r,conf);
+ access_conf = get_access_conf(r,conf, r->pool);
if (access_conf == NULL)
return DECLINED;
@@ -577,14 +580,13 @@
}
/*
- * This function is used as a provider to allow mod_dav_svn to bypass the
- * generation of an apache request when checking GET access from
- * "mod_dav_svn/authz.c" .
+ * Implementation of subreq_bypass with scratch_pool parameter.
*/
static int
-subreq_bypass(request_rec *r,
- const char *repos_path,
- const char *repos_name)
+subreq_bypass2(request_rec *r,
+ const char *repos_path,
+ const char *repos_name,
+ apr_pool_t *scratch_pool)
{
svn_error_t *svn_err = NULL;
svn_authz_t *access_conf = NULL;
@@ -595,7 +597,7 @@
conf = ap_get_module_config(r->per_dir_config,
&authz_svn_module);
- username_to_authorize = get_username_to_authorize(r, conf);
+ username_to_authorize = get_username_to_authorize(r, conf, scratch_pool);
/* If configured properly, this should never be true, but just in case. */
if (!conf->anonymous
@@ -606,7 +608,7 @@
}
/* Retrieve authorization file */
- access_conf = get_access_conf(r, conf);
+ access_conf = get_access_conf(r, conf, scratch_pool);
if (access_conf == NULL)
return HTTP_FORBIDDEN;
@@ -650,6 +652,25 @@
}
/*
+ * This function is used as a provider to allow mod_dav_svn to bypass the
+ * generation of an apache request when checking GET access from
+ * "mod_dav_svn/authz.c" .
+ */
+static int
+subreq_bypass(request_rec *r,
+ const char *repos_path,
+ const char *repos_name)
+{
+ int status;
+ apr_pool_t *scratch_pool;
+
+ scratch_pool = svn_pool_create(r->pool);
+ status = subreq_bypass2(r, repos_path, repos_name, scratch_pool);
+ svn_pool_destroy(scratch_pool);
+
+ return status;
+}
+/*
* Hooks
*/
