[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...

From: Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com>
Date: Tue, 23 Aug 2016 18:58:29 +0300

Joe Orton <jorton_at_redhat.com> writes:

> Can you work around the bug in mod_dav_svn by writing to
> output->r->output_filters each time rather than the passed-in "output"
> directly? Or alternatively use a request_rec stashed in resource->info
> and simply reference r->output_filters from there?
>
> Obviously that doesn't fix the underlying API issue, but it'd be worth
> validating as a (relatively simple?) alternative.

Thank you for taking a look at the issue.

It might be possible to rework mod_dav_svn, although it's going to take
some time. Currently, every top-level handler receives an `ap_filter_t *`
and passes it further, and all these places would have to be updated so
that the actual writes would target r->output_filters.

There also is the function in the core part of mod_dav that shares the same
kind of problem and requires a separate fix. Please see mod_dav.h:554:

  DAV_DECLARE(void) dav_send_one_response(dav_response *response,
                                          apr_bucket_brigade *bb,
                                          ap_filter_t *output,
                                          apr_pool_t *pool);

For a start, I prepared two patches for mod_dav:

 - The first patch fixes the problem in dav_send_one_response(). Please note
   that while dav_send_one_response() is public API, it's not yet released as
   of 2.4.23. So, this patch changes it directly, without introducing a new
   dav_send_one_response2() function for compatibility.

 - The second patch notes the API issue in the docs for the mentioned DAV
   hooks, deliver(), deliver_report() and merge() — for the sake of anyone
   who might be implementing a DAV provider.

The patches are attached (log messages are in the beginning of each file).

What do you think?

> BTW, looking at mod_dav_svn:repos.c's deliver(), the lack of brigade
> reuse is potentially going to consume a lot of RAM too; abbreviated code
> like:
>
> block = apr_palloc(resource->pool, SVN__STREAM_CHUNK_SIZE);
> while (1) {
> serr = svn_stream_read_full(stream, block, &bufsize);
> bb = apr_brigade_create(resource->pool, output->c->bucket_alloc);
> if ((status = ap_pass_brigade(output, bb)) != APR_SUCCESS) {
>
> ... that brigade should be created before entering the loop; call
> apr_brigade_cleanup() after calling ap_pass_brigade() at the end of each
> iteration to ensure it's empty.

Thanks for pointing this out. I'll fix the problem (and a similar issue in
repos.c:write_to_filter()).

Regards,
Evgeny Kotkov

Received on 2016-08-23 17:58:56 CEST

This is an archived mail posted to the Subversion Dev mailing list.