On Sun, Aug 11, 2013 at 11:17 PM, Philip Martin
wrote:
> To see the difference consider an authz file with just one block for /.
> Given two paths, /foo/bar and /zig/zag, your patch produces two cache
> entries. Repeat requests for one of the paths pull the result directly
> from the cache. The
Roderich Schupp writes:
> On Friday, August 9, 2013, Philip Martin wrote:
>
>>
>> We could cache the results of authz_parse_line. That would limit the
>> cache size as it would only grow to the number of sections in the file.
>> Also authz_parse_line is often the expensive bit as it involves ch
On Friday, August 9, 2013, Philip Martin wrote:
>
> We could cache the results of authz_parse_line. That would limit the
> cache size as it would only grow to the number of sections in the file.
> Also authz_parse_line is often the expensive bit as it involves checking
> the user against all the
Ivan Zhakov writes:
> As far I understand cache will be very inefficient if authorization
> file will be bigger than configured limit. I real world cases cache
> will hold data for every configured path if user run svn log on
> repository root or something.
We could cache the results of authz_pa
On Thu, Aug 8, 2013 at 11:04 PM, Ben Reser wrote:
> On Thu, Aug 8, 2013 at 5:41 AM, Ivan Zhakov wrote:
>> I don't see problem here: in worst scenario cache size would same as
>> authorization file. So even for large authorization files memory usage
>> will be limited.
>
> Yes I realize it's not e
On Wed, Aug 7, 2013 at 4:15 PM, Roderich Schupp
wrote:
> Actually I think that your worst case scenario should be illegal
> and that Apache should close a connection when the user changes
> between requests.
I disagree that this sort of scenario should be disallowed. It's a
fundamental feature o
On Thu, Aug 8, 2013 at 1:08 AM, Ben Reser wrote:
> On Wed, Aug 7, 2013 at 1:03 PM, Roderich Schupp
> wrote:
>> Err... the cache apr_hash_t by construction cannot contain keys (i.e.paths)
>> that are not in the authz file also, so is bounded by the size of the
>> corresponding svn_config_t. In fac
On Wed, Aug 7, 2013 at 11:08 PM, Ben Reser wrote:
> Yes that's true. I know there are people out there with very large
> authz files though. Your cache isn't going to use much extra memory
> for most connections.
>
We could shave off some more if we integrated the cache into svn_config_t.
But
On Wed, Aug 7, 2013 at 1:03 PM, Roderich Schupp
wrote:
> Err... the cache apr_hash_t by construction cannot contain keys (i.e.paths)
> that are not in the authz file also, so is bounded by the size of the
> corresponding svn_config_t. In fact, one could precompute the maximal
> cache on the first
On Wed, Aug 7, 2013 at 9:04 PM, Ben Reser wrote:
> An attacker can open a connection and starting making requests
> to fill up a servers memory. As long as they keep making authz
> requests as the same user on the same connection they will be able to
> increase the server usage everytime we do a
First of all I'd like to say thanks for doing this. I'd thought
briefly about doing something like this yesterday. This is a good
start.
The biggest issue I see with this is that you're adding an unbounded
cache. An attacker can open a connection and starting making requests
to fill up a server
On Wed, Aug 7, 2013 at 3:59 PM, Ivan Zhakov wrote:
> Could you please provide log message for your patch submission [1]. It
> makes reviewing patch much easier.
>
See below.
> Did you consider make separate cache_pool and clear if we invoked with
> different user?
>
I revised the patch so tha
On Wed, Aug 7, 2013 at 5:43 PM, Roderich Schupp
wrote:
> Hi,
>
> revised patch attached, hopefully addressing all concerns.
>
> No more breaking encapsulation. We simply allocate svn_authz_t from the same
> pool
> that svn_config_t is allocated from and remember this pool in a new
> svn_authz_t
>
Hi,
revised patch attached, hopefully addressing all concerns.
No more breaking encapsulation. We simply allocate svn_authz_t from the
same pool
that svn_config_t is allocated from and remember this pool in a new
svn_authz_t
member "pool".l Create a function authz_create() for that; it uses
apr_p
On Wed, Aug 7, 2013 at 11:02 AM, Daniel Shahaf wrote:
> > The patch needs to #include libsvn_subr/config_impl.h
> > in order to gain access to svn_config_t.pool:
> > the cache (apr_hash_t itself, keys and values) must be allocated from
> > the same pool as svn_config_t so that they have the same
On Wed, Aug 7, 2013 at 11:05 AM, Philip Martin
wrote:
> > +svn_boolean_t authz_use_cache = TRUE; /* FIXME devel only */
>
> I assume that is temporary, what is the long-term plan? Remove it
> completely?
>
Yes, this will go away. It's just there for benchmarking, so that I can
turn caching
On Wed, Aug 7, 2013 at 11:07 AM, Ivan Zhakov wrote:
> > For example, when a client uses a long-lived keep-alive connection and
> > the authz file changes during the life of that connection, will your
> > change cause the changes to be picked up later than they would before
> > your patch?
> >
> W
On Wed, Aug 7, 2013 at 11:05 AM, Philip Martin
wrote:
> It would probably better to introduce a new API something like:
>
> apr_pool_t *svn_config__pool(svn_config_t *);
>
> declared in subversion/include/private/.
>
> However I don't fully understand the logic governing the lifetime. The
> cache
Ivan Zhakov wrote on Wed, Aug 07, 2013 at 13:07:27 +0400:
> On Wed, Aug 7, 2013 at 1:02 PM, Daniel Shahaf wrote:
> > Please use text/* MIME type. The .c file was so but the .patch wasn't,
> > and that gets in the way of reviewing it. More below...
> >
> > Roderich Schupp wrote on Wed, Aug 07, 20
On Wed, Aug 7, 2013 at 1:02 PM, Daniel Shahaf wrote:
> Please use text/* MIME type. The .c file was so but the .patch wasn't,
> and that gets in the way of reviewing it. More below...
>
> Roderich Schupp wrote on Wed, Aug 07, 2013 at 08:27:43 +0200:
>> This patch reduces the cumulative time for
Roderich Schupp writes:
> The patch needs to #include libsvn_subr/config_impl.h
> in order to gain access to svn_config_t.pool:
> the cache (apr_hash_t itself, keys and values) must be allocated from
> the same pool as svn_config_t so that they have the same lifespan.
It would probably better to
Please use text/* MIME type. The .c file was so but the .patch wasn't,
and that gets in the way of reviewing it. More below...
Roderich Schupp wrote on Wed, Aug 07, 2013 at 08:27:43 +0200:
> This patch reduces the cumulative time for svn_repos_authz_check_access
> (when called repeteadly in the
Hi,
this patch attempts to speed up svn_repos_authz_check_access, esp. when it
is called
repeatedly during the same HTTP request (or on the same connection).
Subversion issues
many HTTP requests that result only in a single call to
svn_repos_authz_check_access
(i.e. just for the path given in the
23 matches
Mail list logo
