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,