Re: [PATCH] Excessive memory use with "SVNPathAuthz short_circuit"

On Wed, Sep 19, 2012 at 3:29 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> On Wed, Sep 19, 2012 at 3:22 PM, Philip Martin
> <philip.martin_at_wandisco.com> wrote:
>> Roderich Schupp <roderich.schupp_at_gmail.com> 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