Joe Orton <jor...@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
mod_dav: Fix a potential cause of unbounded memory usage or incorrect
behavior in a routine that sends <DAV:response>'s to the output filters.
The dav_send_one_response() function accepts the current head of the output
filter list as an argument, but the actual head can change between calls to
ap_pass_brigade(). This can happen with self-removing filters, e.g., with
the filter from mod_headers or mod_deflate. Consequently, executing an
already removed filter can either cause unwanted memory usage or incorrect
behavior.
* modules/dav/main/mod_dav.c
(dav_send_one_response): Accept a request_rec instead of an ap_filter_t.
Write the response to r->output_filters.
(dav_send_multistatus, dav_stream_response): Update these calling sites
of dav_send_one_response().
* modules/dav/main/mod_dav.h
(dav_send_one_response): Adjust function definition.
Index: modules/dav/main/mod_dav.c
===================================================================
--- modules/dav/main/mod_dav.c (revision 1757382)
+++ modules/dav/main/mod_dav.c (working copy)
@@ -438,7 +438,8 @@ static const char *dav_xml_escape_uri(apr_pool_t *
/* Write a complete RESPONSE object out as a <DAV:repsonse> xml
element. Data is sent into brigade BB, which is auto-flushed into
- OUTPUT filter stack. Use POOL for any temporary allocations.
+ the output filter stack for request R. Use POOL for any temporary
+ allocations.
[Presumably the <multistatus> tag has already been written; this
routine is shared by dav_send_multistatus and dav_stream_response.]
@@ -445,23 +446,23 @@ static const char *dav_xml_escape_uri(apr_pool_t *
*/
DAV_DECLARE(void) dav_send_one_response(dav_response *response,
apr_bucket_brigade *bb,
- ap_filter_t *output,
+ request_rec *r,
apr_pool_t *pool)
{
apr_text *t = NULL;
if (response->propresult.xmlns == NULL) {
- ap_fputs(output, bb, "<D:response>");
+ ap_fputs(r->output_filters, bb, "<D:response>");
}
else {
- ap_fputs(output, bb, "<D:response");
+ ap_fputs(r->output_filters, bb, "<D:response");
for (t = response->propresult.xmlns; t; t = t->next) {
- ap_fputs(output, bb, t->text);
+ ap_fputs(r->output_filters, bb, t->text);
}
- ap_fputc(output, bb, '>');
+ ap_fputc(r->output_filters, bb, '>');
}
- ap_fputstrs(output, bb,
+ ap_fputstrs(r->output_filters, bb,
DEBUG_CR "<D:href>",
dav_xml_escape_uri(pool, response->href),
"</D:href>" DEBUG_CR,
@@ -472,7 +473,7 @@ DAV_DECLARE(void) dav_send_one_response(dav_respon
* default to 500 Internal Server Error if first->status
* is not a known (or valid) status code.
*/
- ap_fputstrs(output, bb,
+ ap_fputstrs(r->output_filters, bb,
"<D:status>HTTP/1.1 ",
ap_get_status_line(response->status),
"</D:status>" DEBUG_CR,
@@ -481,7 +482,7 @@ DAV_DECLARE(void) dav_send_one_response(dav_respon
else {
/* assume this includes <propstat> and is quoted properly */
for (t = response->propresult.propstats; t; t = t->next) {
- ap_fputs(output, bb, t->text);
+ ap_fputs(r->output_filters, bb, t->text);
}
}
@@ -490,7 +491,7 @@ DAV_DECLARE(void) dav_send_one_response(dav_respon
* We supply the description, so we know it doesn't have to
* have any escaping/encoding applied to it.
*/
- ap_fputstrs(output, bb,
+ ap_fputstrs(r->output_filters, bb,
"<D:responsedescription>",
response->desc,
"</D:responsedescription>" DEBUG_CR,
@@ -497,7 +498,7 @@ DAV_DECLARE(void) dav_send_one_response(dav_respon
NULL);
}
- ap_fputs(output, bb, "</D:response>" DEBUG_CR);
+ ap_fputs(r->output_filters, bb, "</D:response>" DEBUG_CR);
}
@@ -559,7 +560,7 @@ DAV_DECLARE(void) dav_send_multistatus(request_rec
for (; first != NULL; first = first->next) {
apr_pool_clear(subpool);
- dav_send_one_response(first, bb, r->output_filters, subpool);
+ dav_send_one_response(first, bb, r, subpool);
}
apr_pool_destroy(subpool);
@@ -1189,7 +1190,7 @@ static void dav_stream_response(dav_walk_resource
resp.propresult = *propstats;
}
- dav_send_one_response(&resp, ctx->bb, ctx->r->output_filters, pool);
+ dav_send_one_response(&resp, ctx->bb, ctx->r, pool);
}
Index: modules/dav/main/mod_dav.h
===================================================================
--- modules/dav/main/mod_dav.h (revision 1757382)
+++ modules/dav/main/mod_dav.h (working copy)
@@ -546,7 +546,8 @@ typedef enum {
/* Write a complete RESPONSE object out as a <DAV:response> xml
* element. Data is sent into brigade BB, which is auto-flushed into
- * OUTPUT filter stack. Use POOL for any temporary allocations.
+ * the output filter stack for request R. Use POOL for any temporary
+ * allocations.
*
* [Presumably the <multistatus> tag has already been written; this
* routine is shared by dav_send_multistatus and dav_stream_response.]
@@ -553,7 +554,7 @@ typedef enum {
*/
DAV_DECLARE(void) dav_send_one_response(dav_response *response,
apr_bucket_brigade *bb,
- ap_filter_t *output,
+ request_rec *r,
apr_pool_t *pool);
/* Factorized helper function: prep request_rec R for a multistatus
mod_dav: Note an API issue with the dav_hooks_repository.deliver(),
dav_hooks_vsn.deliver_report() and dav_hooks_vsn.merge() hook
definitions.
* modules/dav/main/mod_dav.h
(dav_hooks_repository.deliver, dav_hooks_vsn.deliver_report,
dav_hooks_vsn.merge): Add notes about the API issue.
Index: modules/dav/main/mod_dav.h
===================================================================
--- modules/dav/main/mod_dav.h (revision 1757382)
+++ modules/dav/main/mod_dav.h (working copy)
@@ -1933,8 +1933,8 @@ struct dav_hooks_repository
const dav_resource *resource);
/*
- ** The provider should deliver the resource into the specified filter.
- ** Basically, this is the response to the GET method.
+ ** The provider should deliver the resource into the request's output
+ ** filter stack. Basically, this is the response to the GET method.
**
** Note that this is called for all resources, including collections.
** The provider should determine what has content to deliver or not.
@@ -1944,6 +1944,15 @@ struct dav_hooks_repository
**
** This may be NULL if handle_get is FALSE.
** ### maybe toss handle_get and just use this function as the marker
+ **
+ ** API ISSUE: don't use the passed-in 'output' filter.
+ **
+ ** Instead, generate the response into the output filter stack for the
+ ** request (r->output_filters). An implementation can use the request_rec
+ ** that was passed to get_resource() for this purpose. Using 'output'
+ ** filter for the response can cause unbounded memory usage.
+ **
+ ** See
https://mail-archives.apache.org/mod_mbox/httpd-dev/201608.mbox/%3C20160822151917.GA22369%40redhat.com%3E
*/
dav_error * (*deliver)(const dav_resource *resource,
ap_filter_t *output);
@@ -2295,6 +2304,15 @@ struct dav_hooks_vsn
**
** ### maybe we need a way to signal an error anyways, and then
** ### apache can abort the connection?
+ **
+ ** API ISSUE: don't use the passed-in 'output' filter.
+ **
+ ** Instead, generate the response into the output filter stack for the
+ ** request (r->output_filters). An implementation can use the request_rec
+ ** that was passed to get_resource() for this purpose. Using 'output'
+ ** filter for the response can cause unbounded memory usage.
+ **
+ ** See
https://mail-archives.apache.org/mod_mbox/httpd-dev/201608.mbox/%3C20160822151917.GA22369%40redhat.com%3E
*/
dav_error * (*deliver_report)(request_rec *r,
const dav_resource *resource,
@@ -2416,6 +2434,15 @@ struct dav_hooks_vsn
**
** This hook is optional; if the provider does not support merging,
** then this should be set to NULL.
+ **
+ ** API ISSUE: don't use the passed-in 'output' filter.
+ **
+ ** Instead, generate the response into the output filter stack for the
+ ** request (r->output_filters). An implementation can use the request_rec
+ ** that was passed to get_resource() for this purpose. Using 'output'
+ ** filter for the response can cause unbounded memory usage.
+ **
+ ** See
https://mail-archives.apache.org/mod_mbox/httpd-dev/201608.mbox/%3C20160822151917.GA22369%40redhat.com%3E
*/
dav_error * (*merge)(dav_resource *target, dav_resource *source,
int no_auto_merge, int no_checkout,
