You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Evgeny Kotkov <ev...@visualsvn.com> on 2016/08/19 13:44:21 UTC
Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
We (as the Apache Subversion developers) have discovered that mod_dav can
trigger an unbounded memory usage when used in conjunction with a module that
inserts an output filter — such as mod_headers or mod_deflate. Below is the
configuration that can be used to reproduce the issue:
MaxMemFree 128
Header always append Foo Bar
<Location />
DAV svn
SVNParentPath C:/Repositories
</Location>
With this configuration, responding to a GET request for a 500 MB file results
in the server consuming a large amount (~3.2 GB) of memory. The memory
footprint is caused by heap allocations. Serving larger files could quickly
exhaust all available memory for the machine.
I would be glad to provide a patch for this issue, but I think it's certainly
worth discussing the solution beforehand.
The problem is caused by how mod_dav passes the output filter list to its
providers. Please see the deliver() hook definition in mod_dav.h:1948 and
its usage in mod_dav.c:888:
/* Repository provider hooks */
struct dav_hooks_repository
{
...
dav_error * (*deliver)(const dav_resource *resource,
ap_filter_t *output);
The hook receives the current head of the output filter list for a particular
request. Certain filters, such as the one in mod_headers, are designed
to perform the work only once. When the work is done, a filter removes
itself from the list. If a filter is the first in the list, this updates the
head of the linked list in request_rec (r->output_filters).
So, after a particular DAV provider calls ap_pass_brigade(), the actual head
of the filter list may now be different from what was passed via the argument.
But the hook is unaware of that, since the previous head of the linked list
was passed via `output` argument. Subsequent calls to the ap_pass_brigade()
are going to invoke these (removed) filters again, and this is what happens
in the example. The mod_headers filter is called per every ap_pass_brigade()
call, it sets the headers multiple times, their values are allocated in the
request pool, and this causes unbounded memory usage. Apart from the memory
consumption, it could also cause various issues due to the filter being
unexpectedly called several times.
One way of solving the problem that I can think of is:
1) Introduce dav_hooks_repository2 with the deliver(), deliver_report() and
merge() hooks accepting a `request_rec` instead of an `ap_filter_t`
A DAV provider would be expected to use r->output_filters, and that's
going to handle the case when the head of the filter list is updated.
New structure is required to keep compatibility, as dav_hooks_repository
is a part of the mod_dav's public API. This would require a couple of
compatibility shims for the new API (and the old versions would become
deprecated).
2) Implement a workaround for existing DAV providers
Do that by inserting a special output filter to the beginning of the list
before calling the particular DAV provider hooks. The filter would be a
no-op, but it would never remove itself from the list, thus guaranteeing
that the head of the filter list doesn't change during the execution of
the hook.
The downside of this approach is that it doesn't guard against other mistakes
of the same kind. It's easy to miss that a head of the filter list can change
between invocations, and the consequences are fairly severe. For instance,
r1748047 [1] adds another public API, dav_send_one_response(), that receives
an `ap_filter_t`, that might change between calls to ap_pass_brigade(), so it
has the same kind of problem.
Maybe this can solved by introducing something like an `ap_filter_chain_t`
that keeps track of the actual head of the list, and starting to use it where
a function needs to push data to the filter stack?
I am ready to prepare a patch for this issue, but perhaps there's a completely
different and a better way to solve the problem? What do you think?
[1] https://svn.apache.org/r1748047
Regards,
Evgeny Kotkov
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Evgeny Kotkov <ev...@visualsvn.com> writes:
> However, the problem I described in this thread is different, and it just
> happens to have the same visible consequence.
>
> What is probably more important, the dav_send_one_response() function
> that has the flaw (see the proposed patch) *just* got promoted into a public
> API, and will become the part of the next 2.4.x release [2]. So I prepared
> the patch, assuming that it would be better to fix this before it becomes
> public.
I committed the fix for dav_send_one_response() function in r1764040 [1],
and proposed it for backport to 2.4.x. Apart from this, I committed the
second patch that adds the note about the API issue in r1764043 [2].
[1] https://svn.apache.org/r1764040
[2] https://svn.apache.org/r1764043
Regards,
Evgeny Kotkov
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Evgeny Kotkov <ev...@visualsvn.com> writes:
> However, the problem I described in this thread is different, and it just
> happens to have the same visible consequence.
>
> What is probably more important, the dav_send_one_response() function
> that has the flaw (see the proposed patch) *just* got promoted into a public
> API, and will become the part of the next 2.4.x release [2]. So I prepared
> the patch, assuming that it would be better to fix this before it becomes
> public.
I committed the fix for dav_send_one_response() function in r1764040 [1],
and proposed it for backport to 2.4.x. Apart from this, I committed the
second patch that adds the note about the API issue in r1764043 [2].
[1] https://svn.apache.org/r1764040
[2] https://svn.apache.org/r1764043
Regards,
Evgeny Kotkov
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Christophe JAILLET <ch...@wanadoo.fr> writes:
> Hi,
>
> while looking at some memory consumption issue in mod_dav, you can also
> have a look at the proposed patches in :
> https://bz.apache.org/bugzilla/show_bug.cgi?id=48130
>
> Definitively not the same approach, but the same goal.
Thanks for the link. We had a lengthy discussion about the mod_dav's
memory usage during PROPFIND requests in svn-dev@ [1].
However, the problem I described in this thread is different, and it just
happens to have the same visible consequence.
What is probably more important, the dav_send_one_response() function
that has the flaw (see the proposed patch) *just* got promoted into a public
API, and will become the part of the next 2.4.x release [2]. So I prepared
the patch, assuming that it would be better to fix this before it becomes
public.
[1] https://mail-archives.apache.org/mod_mbox/subversion-dev/201512.mbox/%3CCAP_GPNhA4hBFdOC7Z1D-K9h_NHm8d7WjyfSF4oUOteUepkjaaA@mail.gmail.com%3E
[2] https://svn.apache.org/r1756561
Regards,
Evgeny Kotkov
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Christophe JAILLET <ch...@wanadoo.fr> writes:
> Hi,
>
> while looking at some memory consumption issue in mod_dav, you can also
> have a look at the proposed patches in :
> https://bz.apache.org/bugzilla/show_bug.cgi?id=48130
>
> Definitively not the same approach, but the same goal.
Thanks for the link. We had a lengthy discussion about the mod_dav's
memory usage during PROPFIND requests in svn-dev@ [1].
However, the problem I described in this thread is different, and it just
happens to have the same visible consequence.
What is probably more important, the dav_send_one_response() function
that has the flaw (see the proposed patch) *just* got promoted into a public
API, and will become the part of the next 2.4.x release [2]. So I prepared
the patch, assuming that it would be better to fix this before it becomes
public.
[1] https://mail-archives.apache.org/mod_mbox/subversion-dev/201512.mbox/%3CCAP_GPNhA4hBFdOC7Z1D-K9h_NHm8d7WjyfSF4oUOteUepkjaaA@mail.gmail.com%3E
[2] https://svn.apache.org/r1756561
Regards,
Evgeny Kotkov
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Posted by Christophe JAILLET <ch...@wanadoo.fr>.
Le 29/08/2016 � 18:12, Evgeny Kotkov a �crit :
> Evgeny Kotkov <ev...@visualsvn.com> writes:
>
>> 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() \u2014 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).
> For the record, I completed a rework of mod_dav_svn that fixes a part
> of the issue. Now, all writes within mod_dav_svn target the actual filter
> list in r->output_filters, and this fixes one cause of the unbounded memory
> usage:
>
> https://svn.apache.org/r1758224
> https://svn.apache.org/r1758207
> https://svn.apache.org/r1758204
>
> However, the second cause of the unbounded memory usage is still present
> in mod_dav. This part of the issue is addressed by my patches from the
> previous e-mail.
>
> Does someone have the time and energy to review these two patches? Or
> maybe there's something in these patches that requires improving?
>
> Just in case, the patches are also available here:
>
> https://mail-archives.apache.org/mod_mbox/httpd-dev/201608.mbox/raw/%3CCAP_GPNhB92KN1KF7TO%3DGs%3DkDpO6weA5%2Bf3nxH3ni0CL0wvYj%2Bg%40mail.gmail.com%3E/2
> https://mail-archives.apache.org/mod_mbox/httpd-dev/201608.mbox/raw/%3CCAP_GPNhB92KN1KF7TO%3DGs%3DkDpO6weA5%2Bf3nxH3ni0CL0wvYj%2Bg%40mail.gmail.com%3E/3
>
> Thanks in advance!
>
>
> Regards,
> Evgeny Kotkov
>
Hi,
while looking at some memory consumption issue in mod_dav, you can also
have a look at the proposed patches in :
https://bz.apache.org/bugzilla/show_bug.cgi?id=48130
Definitively not the same approach, but the same goal.
CJ
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Posted by Christophe JAILLET <ch...@wanadoo.fr>.
Le 29/08/2016 � 18:12, Evgeny Kotkov a �crit :
> Evgeny Kotkov <ev...@visualsvn.com> writes:
>
>> 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() \u2014 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).
> For the record, I completed a rework of mod_dav_svn that fixes a part
> of the issue. Now, all writes within mod_dav_svn target the actual filter
> list in r->output_filters, and this fixes one cause of the unbounded memory
> usage:
>
> https://svn.apache.org/r1758224
> https://svn.apache.org/r1758207
> https://svn.apache.org/r1758204
>
> However, the second cause of the unbounded memory usage is still present
> in mod_dav. This part of the issue is addressed by my patches from the
> previous e-mail.
>
> Does someone have the time and energy to review these two patches? Or
> maybe there's something in these patches that requires improving?
>
> Just in case, the patches are also available here:
>
> https://mail-archives.apache.org/mod_mbox/httpd-dev/201608.mbox/raw/%3CCAP_GPNhB92KN1KF7TO%3DGs%3DkDpO6weA5%2Bf3nxH3ni0CL0wvYj%2Bg%40mail.gmail.com%3E/2
> https://mail-archives.apache.org/mod_mbox/httpd-dev/201608.mbox/raw/%3CCAP_GPNhB92KN1KF7TO%3DGs%3DkDpO6weA5%2Bf3nxH3ni0CL0wvYj%2Bg%40mail.gmail.com%3E/3
>
> Thanks in advance!
>
>
> Regards,
> Evgeny Kotkov
>
Hi,
while looking at some memory consumption issue in mod_dav, you can also
have a look at the proposed patches in :
https://bz.apache.org/bugzilla/show_bug.cgi?id=48130
Definitively not the same approach, but the same goal.
CJ
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Posted by Christophe JAILLET <ch...@wanadoo.fr>.
Le 29/08/2016 � 18:12, Evgeny Kotkov a �crit :
> Evgeny Kotkov <ev...@visualsvn.com> writes:
>
>> 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() \u2014 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).
> For the record, I completed a rework of mod_dav_svn that fixes a part
> of the issue. Now, all writes within mod_dav_svn target the actual filter
> list in r->output_filters, and this fixes one cause of the unbounded memory
> usage:
>
> https://svn.apache.org/r1758224
> https://svn.apache.org/r1758207
> https://svn.apache.org/r1758204
>
> However, the second cause of the unbounded memory usage is still present
> in mod_dav. This part of the issue is addressed by my patches from the
> previous e-mail.
>
> Does someone have the time and energy to review these two patches? Or
> maybe there's something in these patches that requires improving?
>
> Just in case, the patches are also available here:
>
> https://mail-archives.apache.org/mod_mbox/httpd-dev/201608.mbox/raw/%3CCAP_GPNhB92KN1KF7TO%3DGs%3DkDpO6weA5%2Bf3nxH3ni0CL0wvYj%2Bg%40mail.gmail.com%3E/2
> https://mail-archives.apache.org/mod_mbox/httpd-dev/201608.mbox/raw/%3CCAP_GPNhB92KN1KF7TO%3DGs%3DkDpO6weA5%2Bf3nxH3ni0CL0wvYj%2Bg%40mail.gmail.com%3E/3
>
> Thanks in advance!
>
>
> Regards,
> Evgeny Kotkov
>
Hi,
while looking at some memory consumption issue in mod_dav, you can also
have a look at the proposed patches in :
https://bz.apache.org/bugzilla/show_bug.cgi?id=48130
Definitively not the same approach, but the same goal.
CJ
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Evgeny Kotkov <ev...@visualsvn.com> writes:
> 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).
For the record, I completed a rework of mod_dav_svn that fixes a part
of the issue. Now, all writes within mod_dav_svn target the actual filter
list in r->output_filters, and this fixes one cause of the unbounded memory
usage:
https://svn.apache.org/r1758224
https://svn.apache.org/r1758207
https://svn.apache.org/r1758204
However, the second cause of the unbounded memory usage is still present
in mod_dav. This part of the issue is addressed by my patches from the
previous e-mail.
Does someone have the time and energy to review these two patches? Or
maybe there's something in these patches that requires improving?
Just in case, the patches are also available here:
https://mail-archives.apache.org/mod_mbox/httpd-dev/201608.mbox/raw/%3CCAP_GPNhB92KN1KF7TO%3DGs%3DkDpO6weA5%2Bf3nxH3ni0CL0wvYj%2Bg%40mail.gmail.com%3E/2
https://mail-archives.apache.org/mod_mbox/httpd-dev/201608.mbox/raw/%3CCAP_GPNhB92KN1KF7TO%3DGs%3DkDpO6weA5%2Bf3nxH3ni0CL0wvYj%2Bg%40mail.gmail.com%3E/3
Thanks in advance!
Regards,
Evgeny Kotkov
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Evgeny Kotkov <ev...@visualsvn.com> writes:
> 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).
For the record, I completed a rework of mod_dav_svn that fixes a part
of the issue. Now, all writes within mod_dav_svn target the actual filter
list in r->output_filters, and this fixes one cause of the unbounded memory
usage:
https://svn.apache.org/r1758224
https://svn.apache.org/r1758207
https://svn.apache.org/r1758204
However, the second cause of the unbounded memory usage is still present
in mod_dav. This part of the issue is addressed by my patches from the
previous e-mail.
Does someone have the time and energy to review these two patches? Or
maybe there's something in these patches that requires improving?
Just in case, the patches are also available here:
https://mail-archives.apache.org/mod_mbox/httpd-dev/201608.mbox/raw/%3CCAP_GPNhB92KN1KF7TO%3DGs%3DkDpO6weA5%2Bf3nxH3ni0CL0wvYj%2Bg%40mail.gmail.com%3E/2
https://mail-archives.apache.org/mod_mbox/httpd-dev/201608.mbox/raw/%3CCAP_GPNhB92KN1KF7TO%3DGs%3DkDpO6weA5%2Bf3nxH3ni0CL0wvYj%2Bg%40mail.gmail.com%3E/3
Thanks in advance!
Regards,
Evgeny Kotkov
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Joe Orton <jo...@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
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Joe Orton <jo...@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
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Aug 19, 2016 at 04:44:21PM +0300, Evgeny Kotkov wrote:
...
> The problem is caused by how mod_dav passes the output filter list to its
> providers. Please see the deliver() hook definition in mod_dav.h:1948 and
> its usage in mod_dav.c:888:
>
> /* Repository provider hooks */
> struct dav_hooks_repository
> {
> ...
> dav_error * (*deliver)(const dav_resource *resource,
> ap_filter_t *output);
>
> The hook receives the current head of the output filter list for a particular
> request. Certain filters, such as the one in mod_headers, are designed
> to perform the work only once. When the work is done, a filter removes
> itself from the list. If a filter is the first in the list, this updates the
> head of the linked list in request_rec (r->output_filters).
Ouch. Yes, this is a nasty API issue.
> One way of solving the problem that I can think of is:
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.
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.
Regards, Joe
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Aug 19, 2016 at 04:44:21PM +0300, Evgeny Kotkov wrote:
...
> The problem is caused by how mod_dav passes the output filter list to its
> providers. Please see the deliver() hook definition in mod_dav.h:1948 and
> its usage in mod_dav.c:888:
>
> /* Repository provider hooks */
> struct dav_hooks_repository
> {
> ...
> dav_error * (*deliver)(const dav_resource *resource,
> ap_filter_t *output);
>
> The hook receives the current head of the output filter list for a particular
> request. Certain filters, such as the one in mod_headers, are designed
> to perform the work only once. When the work is done, a filter removes
> itself from the list. If a filter is the first in the list, this updates the
> head of the linked list in request_rec (r->output_filters).
Ouch. Yes, this is a nasty API issue.
> One way of solving the problem that I can think of is:
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.
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.
Regards, Joe