[RFC/PATCH] Avoid redefining apr_filepath_merge() for UNC path checks
Hi all, Currently, `httpd.h` redefines the `apr_filepath_merge()` function on Win32 as follows: ``` #ifdef WIN32 #define apr_filepath_merge ap_filepath_merge #endif ``` The intent behind this redefinition seems to be to retroactively apply the recently introduced UNC path checks to any existing calls of apr_filepath_merge(), including those in third-party module code. However, this approach has several downsides, because the security-related behavior of the code is now tied to the inclusion of : - Utility layers that do not include httpd.h but use apr_filepath_merge() will need to include this header to ensure appropriate security behavior. - Refactorings that involve adding or removing #include can inadvertently change the security properties of the code. - It becomes challenging to understand the code behavior by inspection alone. Personally, I find these issues quite significant. An alternative would be to explicitly use ap_filepath_merge() everywhere and remove the platform-specific define. This alternative is demonstrated in the attached patch: [[[ * include/httpd.h (apr_filepath_merge): Remove this Win32-specific #define, and … * modules/*, server/*: …replace all its usages with ap_filepath_merge(). This ensures explicit and consistent security-related behavior that doesn't depend on the inclusion of the header. ]]] What do you think? Thanks, Evgeny Kotkov Index: include/httpd.h === --- include/httpd.h (revision 1918820) +++ include/httpd.h (working copy) @@ -2875,10 +2875,6 @@ apr_int32_t flags, apr_pool_t *p); -#ifdef WIN32 -#define apr_filepath_merge ap_filepath_merge -#endif - /* Win32/NetWare/OS2 need to check for both forward and back slashes * in ap_normalize_path() and ap_escape_url(). */ Index: modules/arch/win32/mod_isapi.c === --- modules/arch/win32/mod_isapi.c (revision 1918820) +++ modules/arch/win32/mod_isapi.c (working copy) @@ -990,7 +990,7 @@ #ifdef WIN32 /* We need to make this a real Windows path name */ -apr_filepath_merge(&file, "", file, APR_FILEPATH_NATIVE, r->pool); +ap_filepath_merge(&file, "", file, APR_FILEPATH_NATIVE, r->pool); #endif *buf_size = apr_cpystrn(buf_data, file, *buf_size) - buf_data; Index: modules/dav/fs/quota.c === --- modules/dav/fs/quota.c (revision 1918820) +++ modules/dav/fs/quota.c (working copy) @@ -98,10 +98,10 @@ break; case APR_DIR: -rv = apr_filepath_merge(&newpath, path, finfo.name, 0, r->pool); +rv = ap_filepath_merge(&newpath, path, finfo.name, 0, r->pool); if (rv != APR_SUCCESS) { ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, - "apr_filepath_merge \"%s\" \"%s\" failed", + "ap_filepath_merge \"%s\" \"%s\" failed", path, finfo.name); goto out; } Index: modules/filters/mod_include.c === --- modules/filters/mod_include.c (revision 1918820) +++ modules/filters/mod_include.c (working copy) @@ -1697,9 +1697,9 @@ char *newpath; /* be safe; only files in this directory or below allowed */ -rv = apr_filepath_merge(&newpath, NULL, tag_val, -APR_FILEPATH_SECUREROOTTEST | -APR_FILEPATH_NOTABSOLUTE, r->pool); +rv = ap_filepath_merge(&newpath, NULL, tag_val, + APR_FILEPATH_SECUREROOTTEST | + APR_FILEPATH_NOTABSOLUTE, r->pool); if (rv != APR_SUCCESS) { error_fmt = APLOGNO(02668) "unable to access file \"%s\" " @@ -1834,9 +1834,9 @@ char *newpath; /* be safe; only files in this directory or below allowed */ -rv = apr_filepath_merge(&newpath, NULL, parsed_string, -APR_FILEPATH_SECUREROOTTEST | -APR_FILEPATH_NOTABSOLUTE, ctx->dpool); +rv = ap_filepath_merge(&newpath, NULL, parsed_string, + APR_FILEPATH_SECUREROOTTEST | + APR_FILEPATH_NOTABSOLUTE, ctx->dpool); if (rv != APR_SUCCESS) { error_fmt = "unable to include file \"%s\" in parsed file %s"; Index: modules/lua/mod_lua.c ===
Re: [Regression in httpd 2.4.52] mod_dav: Potentially unbounded memory usage in PROPFIND with dav_get_props() and dav_propfind_walker()
Ruediger Pluem writes: > Can you please check if the below patch fixes your issue? I have to say that the reason and the original intention of using resource->pool's userdata here are still somewhat unclear to me. But it does look like the patch performs the allocation only once per resource->pool, and so it should fix the unbounded memory usage issue. Thanks! Regards, Evgeny Kotkov
[Regression in httpd 2.4.52] mod_dav: Potentially unbounded memory usage in PROPFIND with dav_get_props() and dav_propfind_walker()
Hi, I might have stumbled across a regression in httpd 2.4.52 where mod_dav was changed in a way where dav_get_props() now allocates data in resource->pool. I think that r1879889 [1] is the change that is causing the new behavior. This change has been backported to 2.4.x in r1895893 [2]. Consider the new part of the dav_get_props() function: DAV_DECLARE(dav_get_props_result) dav_get_props() { … element = apr_pcalloc(propdb->resource->pool, sizeof(dav_liveprop_elem)); element->doc = doc; … This code unconditionally allocates data in resource->pool (this is the only such place, other allocations performed by this function happen in propdb->p). The dav_get_props() is called by dav_propfind_walker(). The walker function is driven by a specific provider. Both of the two implementations known to me, mod_dav_fs and mod_dav_svn, happen to use a long-living pool as the resource->pool during the walk. I think that with this change, the PROPFIND walks are going to make O(N) allocations for O(N) walked items — which is an unbounded memory usage and a regression, compared to 2.4.51. [1] https://svn.apache.org/r1879889 [2] https://svn.apache.org/r1895893 Thanks, Evgeny Kotkov
Re: [Regression in httpd 2.4.49] mod_dav: REPORT requests no longer return errors
Ruediger Pluem writes: > Does the attached patch solve your issue? It does appear to solve the problem with missing errors, thanks! I haven't checked that in detail, but I think there might be a discrepancy in how `err` is handled in the patch and for example when calling the method_precondition() hook. With the patch, `err` is checked even if all hooks DECLINE the operation. Not too sure if that's intended, because the variable could potentially contain an arbitrary value or a leftover from some previous call. Thanks, Evgeny Kotkov
[Regression in httpd 2.4.49] mod_dav: REPORT requests no longer return errors
Hi, I think that I have stumbled across a significant regression in httpd 2.4.49 where mod_dav has been changed in a way that makes it ignore the errors returned by a versioning provider during REPORT requests. I haven't checked that in detail, but I think that r1892513 [1] is the change that is causing the new behavior. Consider the new core part of the dav_method_report() function: … result = dav_run_deliver_report(r, resource, doc, r->output_filters, &err); switch (result) { case OK: return DONE; case DECLINED: /* No one handled the report */ return HTTP_NOT_IMPLEMENTED; default: if ((err) != NULL) { … handle the error } Assuming there are no deliver_report hooks, this code is going to call dav_core_deliver_report(), whose relevant part is as follows: … if (vsn_hooks) { *err = (*vsn_hooks->deliver_report)(r, resource, doc, r->output_filters); return OK; } … Here the "return OK" part skips the error handling on the calling site, even if there is an associated error returned in *err. In turn, this causes the following regression: - For a failed request where the server hasn't started sending the response, it is now going to erroneously send a 200 OK with an empty body instead of an error response with the appropriate HTTP status. - For a failed request where the server has started sending the response body, it is now going to handle it as if it was successfully completed instead of aborting the connection. The likely outcome of this is that the server is going to send a truncated payload with a 2xx status indicating success. This regression can cause unexpected behavior of the Subversion clients, for example in cases where the (ignored) error occurred due to a non-successful authorization check. Other DAV clients may be susceptible to some kinds of unexpected behavior as well. [1] https://svn.apache.org/r1892513 Thanks, Evgeny Kotkov
Re: svn commit: r1837056 - in /httpd/httpd/trunk: ./ include/ modules/filters/ modules/http/ modules/http2/ modules/proxy/ modules/test/ server/
Evgeny Kotkov writes: >> +1 for the patch, I missed the separate 304 handling in >> mod_brotli/deflate, thanks for catching this! > > Thanks, I will commit it at the earliest opportunity. Committed and proposed for a backport to 2.4.x: https://svn.apache.org/r1843242 https://svn.apache.org/r1843245 Thanks, Evgeny Kotkov
Re: svn commit: r1837056 - in /httpd/httpd/trunk: ./ include/ modules/filters/ modules/http/ modules/http2/ modules/proxy/ modules/test/ server/
Yann Ylavic writes: >> I am thinking about fixing this with the attached patch and proposing it for >> backport to 2.4.x. >> >> Would there be any objections to that? > > +1 for the patch, I missed the separate 304 handling in > mod_brotli/deflate, thanks for catching this! Thanks, I will commit it at the earliest opportunity. > It seems that the 304 "shortcut" happens too late though, after > entity/content-* headers are added to the response, which does not > look right. Don't we need something like the attached patch too? I haven't checked it in details, but at first glance I think that this patch could break a few cases. One example would be a case with several filters working in a chain for a 304 response. The first of them sees Content-Encoding identity, performs some fix-ups, such as the one in deflate_check_etag() and removes itself from the chain without altering the r->content-encoding or the Content- Encoding header value. The next filter then sees the C-E identity again, decides to do another fix-up before bailing out, and thus results in an incorrect ETag value or something similar. (An interesting observation is that https://svn.apache.org/r743814 does an opposite of this patch by ensuring that C-E is actually set prior to bailing out and removing itself when dealing with 304's.) However, a more important question is whether there is an actual problem to solve. I see that ap_http_header_filter() features a whitelist of headers that are sent for 304 responses (http_filters.c:1428), and all headers such as Content-Encoding are filtered anyway. So maybe the current state doesn't require fixing at all — assuming that neither mod_deflate nor mod_brotli can actually begin streaming the 304 response with the (unexpected) set of headers — which I don't think could be happening. Thanks, Evgeny Kotkov
Re: svn commit: r1837056 - in /httpd/httpd/trunk: ./ include/ modules/filters/ modules/http/ modules/http2/ modules/proxy/ modules/test/ server/
Yann Ylavic writes: > Log: > http: Enforce consistently no response body with both 204 and 304 statuses. [...] > --- httpd/httpd/trunk/modules/filters/mod_deflate.c (original) > +++ httpd/httpd/trunk/modules/filters/mod_deflate.c Mon Jul 30 13:08:23 2018 > @@ -642,18 +642,19 @@ static apr_status_t deflate_out_filter(a > > /* > * Only work on main request, not subrequests, > - * that are not a 204 response with no content > + * that are not responses with no content (204/304), > * and are not tagged with the no-gzip env variable > * and not a partial response to a Range request. > */ > -if ((r->main != NULL) || (r->status == HTTP_NO_CONTENT) || > +if ((r->main != NULL) || > +AP_STATUS_IS_HEADER_ONLY(r->status) || I think that this change affects how mod_deflate and mod_brotli handle 304's. Previously, they handled HTTP_NO_CONTENT and HTTP_NOT_MODIFIED separately. This allowed them to fixup headers such as ETag for 304 responses, following RFC7232, 4.1 (saying that a 304 response must include appropriate ETag, Vary and etc.). However, with this change both of them would do nothing for 304, and potentially violate the spec for ETag and maybe some other header values. I am thinking about fixing this with the attached patch and proposing it for backport to 2.4.x. Would there be any objections to that? Thanks, Evgeny Kotkov Index: modules/filters/mod_brotli.c === --- modules/filters/mod_brotli.c(revision 1842726) +++ modules/filters/mod_brotli.c(working copy) @@ -346,12 +346,14 @@ static apr_status_t compress_filter(ap_filter_t *f const char *accepts; /* Only work on main request, not subrequests, that are not - * responses with no content (204/304), and are not tagged with the + * a 204 response with no content, and are not tagged with the * no-brotli env variable, and are not a partial response to * a Range request. + * + * Note that responding to 304 is handled separately to set + * the required headers (such as ETag) per RFC7232, 4.1. */ -if (r->main -|| AP_STATUS_IS_HEADER_ONLY(r->status) +if (r->main || r->status == HTTP_NO_CONTENT || apr_table_get(r->subprocess_env, "no-brotli") || apr_table_get(r->headers_out, "Content-Range")) { ap_remove_output_filter(f); Index: modules/filters/mod_deflate.c === --- modules/filters/mod_deflate.c (revision 1842726) +++ modules/filters/mod_deflate.c (working copy) @@ -642,12 +642,14 @@ static apr_status_t deflate_out_filter(ap_filter_t /* * Only work on main request, not subrequests, - * that are not responses with no content (204/304), + * that are not a 204 response with no content * and are not tagged with the no-gzip env variable * and not a partial response to a Range request. + * + * Note that responding to 304 is handled separately to + * set the required headers (such as ETag) per RFC7232, 4.1. */ -if ((r->main != NULL) || -AP_STATUS_IS_HEADER_ONLY(r->status) || +if ((r->main != NULL) || (r->status == HTTP_NO_CONTENT) || apr_table_get(r->subprocess_env, "no-gzip") || apr_table_get(r->headers_out, "Content-Range") ) { @@ -654,7 +656,7 @@ static apr_status_t deflate_out_filter(ap_filter_t if (APLOG_R_IS_LEVEL(r, APLOG_TRACE1)) { const char *reason = (r->main != NULL) ? "subrequest" : -AP_STATUS_IS_HEADER_ONLY(r->status) ? "no content" : +(r->status == HTTP_NO_CONTENT) ? "no content" : apr_table_get(r->subprocess_env, "no-gzip") ? "no-gzip" : "content-range"; ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, @@ -1533,12 +1535,14 @@ static apr_status_t inflate_out_filter(ap_filter_t /* * Only work on main request, not subrequests, - * that are not responses with no content (204/304), + * that are not a 204 response with no content * and not a partial response to a Range request, * and only when Content-Encoding ends in gzip. + * + * Note that responding to 304 is handled separately to + * set the required headers (such as ETag) per RFC7232, 4.1. */ -if (!ap_is_initial_req(r) || -AP_STATUS_IS_
Re: [PATCH] Remove unused enum values in mpm_winnt
Ivan Zhakov writes: > Please find attached patch that removes unused values of io_state_e > enum in mpm_winnt. Thanks, patch committed in https://svn.apache.org/r1801657 Regards, Evgeny Kotkov
Re: [RFC/PATCH] mpm_winnt: Fix several issues in the child process shutdown logic
Evgeny Kotkov writes: > (1) As a part of the process of shutting down worker threads, the code > around child.c:1170 currently posts an amount of I/O completion packets > equal to the amount of the threads blocked on the I/O completion port. > Then it sleeps until all these threads "acknowledge" the completion > packets by decrementing the global amount of blocked threads. > > This can be improved to avoid unnecessary Sleep()'s, and make the > shutdown faster. There is no need to block until the threads actually > receive the completion, as the shutdown process includes a separate step > that waits until the threads exit. Instead of synchronizing on the > amount of the threads blocked on the I/O completion port, we can send > the number of IOCP_SHUTDOWN completion packets equal to the total > amount of threads and immediately proceed to the next step. Committed in https://svn.apache.org/r1801635 > (2) If the shutdown routine hits a timeout while waiting for the worker > threads to exit, it uses TerminateThread() to terminate the remaining > threads. > > Using TerminateThread() can have dangerous consequences such as > deadlocks — say, if the the thread is terminated while holding a lock > or a heap lock in the middle of HeapAlloc(), as these locks would not > be released. Or it can corrupt the application state and cause a crash. > See https://msdn.microsoft.com/en-us/library/windows/desktop/ms686717 > > Presumably, a much better alternative here would be to leave the cleanup > to the operating system by calling TerminateProcess(). Committed in https://svn.apache.org/r1801636 > (3) Assuming (2) is in place, the code around child.c:1170 that waits for > multiple thread handles in batches can be simplified. With (2), there > is no difference between ending the wait with one or multiple remaining > threads. (Since we terminate the process if at least one thread is > still active when we hit a timeout.) > > Therefore, instead of making an effort to evenly distribute and batch > the handles with WaitForMultipleObjects(), we could just start from > one end, and wait for one thread handle at a time. Committed in https://svn.apache.org/r1801637 Regards, Evgeny Kotkov
[RFC/PATCH] mpm_winnt: Fix several issues in the child process shutdown logic
Hi all, Recently we (Ivan Zhakov and I) found several issues in the code responsible for shutting down a mpm_winnt's child process. The attached patch should fix these issues: (Please note that the changes are combined into a single patch to make the review easier, but I'll commit them independently.) (1) As a part of the process of shutting down worker threads, the code around child.c:1170 currently posts an amount of I/O completion packets equal to the amount of the threads blocked on the I/O completion port. Then it sleeps until all these threads "acknowledge" the completion packets by decrementing the global amount of blocked threads. This can be improved to avoid unnecessary Sleep()'s, and make the shutdown faster. There is no need to block until the threads actually receive the completion, as the shutdown process includes a separate step that waits until the threads exit. Instead of synchronizing on the amount of the threads blocked on the I/O completion port, we can send the number of IOCP_SHUTDOWN completion packets equal to the total amount of threads and immediately proceed to the next step. (2) If the shutdown routine hits a timeout while waiting for the worker threads to exit, it uses TerminateThread() to terminate the remaining threads. Using TerminateThread() can have dangerous consequences such as deadlocks — say, if the the thread is terminated while holding a lock or a heap lock in the middle of HeapAlloc(), as these locks would not be released. Or it can corrupt the application state and cause a crash. See https://msdn.microsoft.com/en-us/library/windows/desktop/ms686717 Presumably, a much better alternative here would be to leave the cleanup to the operating system by calling TerminateProcess(). (3) Assuming (2) is in place, the code around child.c:1170 that waits for multiple thread handles in batches can be simplified. With (2), there is no difference between ending the wait with one or multiple remaining threads. (Since we terminate the process if at least one thread is still active when we hit a timeout.) Therefore, instead of making an effort to evenly distribute and batch the handles with WaitForMultipleObjects(), we could just start from one end, and wait for one thread handle at a time. Note that apart from this, the code around child.c:1146 that shuts down the listeners, waits, and then proceeds to shutting down the worker threads is prone to a subtle race. Since the wait interval is hardcoded to 1 second, there could still be an accepted connection after it. And, as the worker threads are being shut down, it is feasible that this accepted connection won't ever find a suitable worker thread. (Eventually, it is going to be ungracefully closed). I think that this can be fixed by properly waiting for the listener threads to exit, and in the meanwhile, this would avoid having the Sleep(1000) call altogether. But this is something that I have now left for future work. I would greatly appreciate if someone could review or comment on the proposed changes. If anyone has an additional insight on the design or planned, but unaccomplished changes to the mpm_winnt module, I would appreciate hearing them as well. Thanks! Regards, Evgeny Kotkov Index: server/mpm/winnt/child.c === --- server/mpm/winnt/child.c(revision 1801135) +++ server/mpm/winnt/child.c(working copy) @@ -827,18 +827,6 @@ static DWORD __stdcall worker_main(void *thread_nu } -static void cleanup_thread(HANDLE *handles, int *thread_cnt, - int thread_to_clean) -{ -int i; - -CloseHandle(handles[thread_to_clean]); -for (i = thread_to_clean; i < ((*thread_cnt) - 1); i++) -handles[i] = handles[i + 1]; -(*thread_cnt)--; -} - - /* * child_main() * Entry point for the main control thread for the child process. @@ -890,7 +878,6 @@ void child_main(apr_pool_t *pconf, DWORD parent_pi HANDLE *child_handles; int listener_started = 0; int threads_created = 0; -int watch_thread; int time_remains; int cld; DWORD tid; @@ -1162,16 +1149,16 @@ void child_main(apr_pool_t *pconf, DWORD parent_pi /* Shutdown the worker threads * Post worker threads blocked on the ThreadDispatch IOCompletion port */ -while (g_blocked_threads > 0) { +if (g_blocked_threads > 0) { ap_log_error(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS, ap_server_conf, APLOGNO(00361) "Child: %d threads blocked on the completion port", g_blocked_threads); -for (i=g_blocked_threads; i > 0; i--) { -PostQueuedCompletionStatus(ThreadDispatchIOCP, 0, -
Re: mod_brotli in 2.4.x is missing a few Makefile changes
Jim Jagielski writes: > BTW, there are also deltas for config.m4 in modules/filters mainly > around mod_brotli. At present what is in 2.4 seems to work fine, but > should we consider backporting config.m4 as well? The difference is caused by these four mentioned changes (r1761824, r1771789, r1771827, r1779111). With them applied, this part of the modules/filters/config.m4 file should be identical between trunk and 2.4.x. I proposed the whole group for the backport in r1792805 and r1792806. Regards, Evgeny Kotkov
mod_brotli in 2.4.x is missing a few Makefile changes
Hi all, I noticed that the version of mod_brotli that has been backported to 2.4.x lacks a few Makefile changes from trunk. This results in a failing Unix build when mod_brotli is not being built. Another issue is that by default the CMakeLists.txt file refers to invalid library filenames. ./configure ; make ... Building shared: mod_buffer.la mod_ratelimit.la mod_reqtimeout.la mod_ext_filter.la mod_request.la mod_include.la mod_filter.la mod_substitute.la mod_sed.la mod_deflate.la /usr/bin/ld: cannot find -lbrotlienc collect2: error: ld returned 1 exit status ... The missing changesets are: https://svn.apache.org/r1761824 https://svn.apache.org/r1771789 https://svn.apache.org/r1771827 https://svn.apache.org/r1779111 Here is the shortlog for them: r1761824: Unbreak building other filter modules without libbrotlienc. r1771789: Rewrite the autoconf script in a, hopefully, less convoluted way. This lays the groundwork to simplify the switch to the official Brotli library. r1771827: Update makefiles to use the library layout of the official Brotli repository. r1779111: Update makefile to cope with the pkg-config layout change in https://github.com/google/brotli/commit/fe9f9a9 With the backport being in place in the 2.4.x branch, these changes no longer merge cleanly from trunk. I can prepare a backport nomination for them that resolves the conflicts and add it to the 2.4.x STATUS. What do you think? Regards, Evgeny Kotkov
Re: The drive for 2.4.26
Jim Jagielski writes: > Let's shoot for a 2.4.26 within the next handful of > weeks. There are some entries in STATUS that could > use some love and attention, and I'm hoping/pushing > for a Brotli[1] release so we can fold *that* capability > in as well. > > 1. https://github.com/google/brotli >https://github.com/google/brotli/issues/483 I noticed that the current mod_brotli backport proposal lacks a couple of changes that allow building against the official repository. I think that the core change (excluding the docs) should be: https://svn.apache.org/r1761714 https://svn.apache.org/r1761824 https://svn.apache.org/r1762512 https://svn.apache.org/r1762515 https://svn.apache.org/r1771789 https://svn.apache.org/r1771791 https://svn.apache.org/r1771827 https://svn.apache.org/r1779077 https://svn.apache.org/r1779111 https://svn.apache.org/r1779700 https://svn.apache.org/r1790852 https://svn.apache.org/r1790853 https://svn.apache.org/r1790860 For the convenience, here is a shortlog for these changes: r1761714: Add initial implementation. r1761824: Unbreak building other filter modules without libbrotlienc. r1762512: Allow compression ratio logging with new BrotliFilterNote directive. r1762515: Handle new 'no-brotli' internal environment variable that disables Brotli compression for a particular request. r1771789: Rewrite the autoconf script in a, hopefully, less convoluted way. This lays the groundwork to simplify the switch to the official Brotli library. r1771791: Explicitly cast 'const uint8_t *' to 'const char *' when using the data received from Brotli to create a bucket. r1771827: Update makefiles to use the library layout of the official Brotli repository. r1779077: Unused variable error could mistakenly note that brotli isn't available. r1779111: Update makefile to cope with the pkg-config layout change in https://github.com/google/brotli/commit/fe9f9a9 r1779700: Save a few bytes and a few cycles. r1790852: Update makefile to allow using Brotli library >= 0.6.0. r1790853: Fix a minor typo in the description of BrotliAlterETag that has been referring to httpd 2.2.x. r1790860: Comment on the default choice (0) for BROTLI_PARAM_LGBLOCK. Regards, Evgeny Kotkov
Re: FYI brotli
William A Rowe Jr writes: > My open questions; has this been entirely reviewed in conjunction with h2? > Will A-E: br,gzip,deflate axe all others from that list when deciding to > enable brotli? (I presume not-yet.) Will gzip filter work where A-E: gzip was > given without br, or are we ceasing to encode half of the web if the user > elects to serve brotli compression? Hi William, Answering some of the questions: 1) I did test mod_brotli in conjunction with mod_http2 around the time it was committed. As far as I remember, I didn't spot anything unusual or any issues. 2) The brotli and deflate output filters can coexist. Moreover, mod_brotli was written with a particular use case in mind where this module is added to an existing mod_deflate installation, and results in sending brotli-encoded data only to the clients that advertise they know how to deal with it via "Accept-Encoding: br". "Accept-Encoding: br, gzip, deflate" is not going to double-encode the data, as both mod_brotli and mod_deflate are smart enough to only kick in for identity Content-Encoding. "Accept-Encoding: gzip" is going to use only the mod_deflate's filter, and mod_brotli will remove itself from the chain, after not finding "br" in the Accept-Encoding. There are a couple of tests from https://svn.apache.org/r1761716 that verify this behavior. (However, I think that right now there is no explicit test for the case of sending "Accept-Encoding: gzip, deflate" to a location with both mod_brotli and mod_deflate.) Regards, Evgeny Kotkov
Re: FYI brotli
Jim Jagielski writes: > Functional patch avail... working on doccos. > > http://home.apache.org/~jim/patches/brotli-2.4.patch Hi Jim, Thank you for the backport patch. There is, however, a potential problem with backporting mod_brotli, since it relies on the Brotli library 1.0.0, which has not yet been released. In other words, if the upstream changes the API or the library layout or their pkg-config files after mod_brotli is shipped with httpd 2.4.x, it's either going to stop building or working. The open ticket about the new release is here: https://github.com/google/brotli/issues/483 My impression on this is that mod_brotli is only safe to backport after the Brotli authors publish the 1.0.0 version of their library. But perhaps I am missing something? (Apart from this, I think that Brotli did change the layout of their pkg-config files in [https://github.com/google/brotli/commit/fe9f9a91], and it requires an update in the filters/config.m4 file; I'll do that.) Regards, Evgeny Kotkov
Re: Mod_brotli, C89 & NetWare - finale
NormW writes: > The only C89 issue (AFAICT) is the following patch, which should allow any > other C89 (with a more up to date OS ;-( ) to compile it, which NetWare's CW > can do for the likes of Apache2, Lua, Zlib, NGHTTP2, OSSL and a bunch of > others. Thanks, I committed the patch in https://svn.apache.org/r1771791 Regards, Evgeny Kotkov
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Evgeny Kotkov 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: C89 (alias NetWare) tries new mod_ on the block.
NormW writes: > G/E. > Aimed 'our' C89 compiler at mod_brotli as much for interest as anything > technical) and get the following using release 0.5.2 of Brotli: > >> Calling NWGNUmod_brotli >> CC mod_brotli.c >> ### mwccnlm Compiler: >> #File: mod_brotli.c >> # - >> # 22: #include >> # Error:^ >> # the file 'brotli/encode.h' cannot be opened >> # Too many errors printed, aborting program Hi, Thanks for giving it a look. mod_brotli is targeted against the upcoming 1.0.x series of brotli (with a different include layout than 0.5.2, and with new BrotliEncoderTakeOutput() API that allows zero-copy processing). If you have the time and energy, could you please try this with the latest development snapshot from https://github.com/google/brotli ? >> Calling NWGNUmod_brotli >> GEN obj_release/mod_brotli_cc.opt >> CC mod_brotli.c >> ### mwccnlm Compiler: >> #File: mod_brotli.c >> # - >> # 240: output = BrotliEncoderTakeOutput(ctx->state, >> &output_len); >> # Error: >> ^ >> # illegal implicit conversion from 'int' to >> # 'const unsigned char *' > > Which I assume is either a C89-ism or CW-ism.. This error happens because 0.5.2 doesn't have the BrotliEncoderTakeOutput() API. As it's undefined, the compiler assumes it returning an int, and that results in the error. > The >>> >>> # >>> # 30: static const float kInfinity = INFINITY; >>> # Error:^ >>> # illegal constant expression > > seems more like a C89-ism to me. Unfortunately, this comes from the brotli itself, and would probably require a fix in the upstream: https://github.com/google/brotli/blob/8148001/enc/backward_references.c#L30 Not too sure on what is the issue here. (How does the #define INFINITY look like in your environment? Perhaps, it's specifically designed to prohibit assignment?). One workaround to get past this would be to use the #else part of this code. Regards, Evgeny Kotkov
Re: svn commit: r1761714 - in /httpd/httpd/trunk: CHANGES CMakeLists.txt docs/log-message-tags/next-number modules/filters/config.m4 modules/filters/mod_brotli.c os/win32/BaseAddr.ref
Ruediger Pluem writes: >> +ac_brotli_libs="${ac_brotli_libs:--lbrotlienc `$apr_config --libs`} " >> +APR_ADDTO(MOD_LDFLAGS, [$ac_brotli_libs]) > > This breaks compilation of trunk if libbrotlienc is not present as it is > added unconditionally. But even if would be added conditionally I sense > that all filters would be linked against libbrotlienc. So better only add > to MOD_BROTLI_LDADD instead of MOD_LDFLAGS Indeed, I totally messed up this part of the configuration script. Sorry for that. I committed a rework of this part in r1761824 [1], and it fixes this build issue for me. Could you please check it in your environment as well? [1] https://svn.apache.org/r1761824 Thanks, Evgeny Kotkov
Re: [PATCH] Introducing mod_brotli
Evgeny Kotkov writes: >>> Wow! This is great stuff. Brotli support has been in my TODO >>> queue for awhile. >>> >>> Thanks! >> >> +1, cool stuff and thanks! > > Glad to hear that, thanks everyone. > > I would be happy to continue the work on this module, for instance, by > adding the necessary documentation and the ability to log compression ratio. So, my current plan is to commit the V1 patch, prepare the necessary bits of documentation (marking the module as experimental), and then continue shaping it up by adding the optional, but important things, such as the compression ratio logging. I'll start doing this tomorrow. Regards, Evgeny Kotkov
Re: Welcome Evgeny Kotkov as a new committer
Jacob Champion writes: >> Please welcome Evgeny Kotkov as a new committer! > > Excellent, welcome to the team! Thanks everyone! Regards, Evgeny Kotkov
Re: [PATCH] Introducing mod_brotli
Reindl Harald writes: > agreed - however, below some configs where my brain rumours how have that > identically behavior by just use "brotli" compression in case the cient > supports it - maybe someone with deeper insights as my pure adiminstrator > view has a idea by looking at it > > the "no-gzip dont-vary" stuff is for long running scripts with > output-flushing to give "realtime" feedback instead have it all buffered > > one brainstorming: "AddOutputCompressionByType" provided by whatever module, > proceed the Accept-Encoding of the client and deciding the compression algo Adding the new output filter to the AddOutputFilterByType directive will result in what you're looking for. For instance, changing AddOutputFilterByType DEFLATE text/html to AddOutputFilterByType BROTLI_COMPRESS;DEFLATE text/html means that the server will send Brotli-compressed data to clients that have "br" in their Accept-Encoding request header, and Deflate data to other clients that indicate gzip/deflate support. > SetEnvIfNoCase Request_URI (download.php)$ no-gzip dont-vary > SetEnvIfNoCase Request_URI (download_imgzip.php)$ no-gzip dont-vary > SetEnvIfNoCase Request_URI (presse_download_zip.php)$ no-gzip dont-vary > SetEnvIfNoCase Request_URI (content_sub_move.php)$ no-gzip dont-vary > SetEnvIfNoCase Request_URI (synch.php)$ no-gzip dont-vary > SetEnvIfNoCase Request_URI (import.php)$ no-gzip dont-vary > SetEnvIfNoCase Request_URI (admin_imagecopyrights.php)$ no-gzip dont-vary > SetEnvIfNoCase Request_URI (newsletter.php)$ no-gzip dont-vary > SetEnvIfNoCase Request_URI (importer.php)$ no-gzip dont-vary > SetEnvIfNoCase Request_URI (create.php)$ no-gzip dont-vary The V1 patch doesn't add the equivalent of "no-gzip" (like, "no-brotli") env variable. This is left out for future work, and until then these statements cannot be migrated seamlessly. However, I think that the wanted behavior here could be achieved with mod_filter's FilterProvider directive using an expression that doesn't install any compression filters for the scripts. Regards, Evgeny Kotkov
Re: [PATCH] Introducing mod_brotli
Eric Covener writes: >> Wow! This is great stuff. Brotli support has been in my TODO >> queue for awhile. >> >> Thanks! > > +1, cool stuff and thanks! Glad to hear that, thanks everyone. I would be happy to continue the work on this module, for instance, by adding the necessary documentation and the ability to log compression ratio. Regards, Evgeny Kotkov
Re: [PATCH] Introducing mod_brotli
Reindl Harald writes: > how is the ordering? > defined by SetOutputFilter or client? Currently, the order is defined by SetOutputFilter, because AFAIK there is no centralized way to handle Accept-Encoding priorities (like ;q=0.7). > does it also support (%{ratio_info}n%%) in the log configuration? > > LogFormat "%a %l %u %t \"%r\" %>s %b \"%{Referer}i\" \"%{User-Agent}i\" > (%{ratio_info}n%%)" combined > > leads to: > > 213.47.77.186 - - [16/Sep/2016:15:13:28 +0200] "GET / HTTP/1.1" 200 4133 "" > "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/537.36 (KHTML, > like Gecko) Chrome/49.0.2623.112 Safari/537.36" (38%) > > would especially when you compare clients with and without support nice to > see the difference here It would certainly be nice, but this version of the patch doesn't have it yet. I could do this separately or provide the V2 patch that supports ratio_info. Regards, Evgeny Kotkov
[PATCH] Introducing mod_brotli
Hi all, This patch adds a module for dynamic Brotli (RFC 7932) compression in httpd. The new compression format is supported by Mozilla Firefox since 44.0 and by Google Chrome since 50.0 [1, 2], and both nginx and IIS have modules that offer Brotli compression. With the new module, existing mod_deflate installations can benefit from better compression ratio by sending Brotli-compressed data to the clients that support it: LoadModule brotli_module modules/mod_brotli.so LoadModule deflate_module modules/mod_deflate.so SetOutputFilter BROTLI_COMPRESS;DEFLATE This module features zero-copy processing, which is only possible with the new API from the upcoming 1.0.x series of brotli [3]. The Linux makefile works against libbrotli [4], as currently the core brotli repository doesn't offer a way to build a library [5]. Enabling mod_brotli can be done with: configure --enable-brotli=shared [--with-brotli=] CMake build is supported as well. Please note that the patch doesn't include the documentation updates and other types of makefiles, but I will do it separately. The second patch adds a couple of tests to the test framework. [1] https://www.mozilla.org/en-US/firefox/44.0/releasenotes/ [2] https://www.chromestatus.com/feature/5420797577396224 [3] https://github.com/google/brotli [4] https://github.com/bagder/libbrotli [5] https://github.com/google/brotli/pull/332 Regards, Evgeny Kotkov Index: CMakeLists.txt === --- CMakeLists.txt (revision 1760986) +++ CMakeLists.txt (working copy) @@ -58,6 +58,12 @@ ELSE() SET(default_nghttp2_libraries "${CMAKE_INSTALL_PREFIX}/lib/nghttp2.lib") ENDIF() +IF(EXISTS "${CMAKE_INSTALL_PREFIX}/lib/brotli_enc.lib") + SET(default_brotli_libraries "${CMAKE_INSTALL_PREFIX}/lib/brotli_enc.lib" "${CMAKE_INSTALL_PREFIX}/lib/brotli_common.lib") +ELSE() + SET(default_brotli_libraries) +ENDIF() + SET(APR_INCLUDE_DIR "${CMAKE_INSTALL_PREFIX}/include" CACHE STRING "Directory with APR[-Util] include files") SET(APR_LIBRARIES ${default_apr_libraries} CACHE STRING "APR libraries to link with") SET(NGHTTP2_INCLUDE_DIR "${CMAKE_INSTALL_PREFIX}/include" CACHE STRING "Directory with NGHTTP2 include files within nghttp2 subdirectory") @@ -66,6 +72,8 @@ SET(PCRE_INCLUDE_DIR "${CMAKE_INSTALL_PREFIX} SET(PCRE_LIBRARIES${default_pcre_libraries} CACHE STRING "PCRE libraries to link with") SET(LIBXML2_ICONV_INCLUDE_DIR "" CACHE STRING "Directory with iconv include files for libxml2") SET(LIBXML2_ICONV_LIBRARIES "" CACHE STRING "iconv libraries to link with for libxml2") +SET(BROTLI_INCLUDE_DIR"${CMAKE_INSTALL_PREFIX}/include" CACHE STRING "Directory with include files for Brotli") +SET(BROTLI_LIBRARIES ${default_brotli_libraries}CACHE STRING "Brotli libraries to link with") # end support library configuration # Misc. options @@ -191,6 +199,18 @@ ELSE() SET(NGHTTP2_FOUND FALSE) ENDIF() +# See if we have Brotli +SET(BROTLI_FOUND TRUE) +IF(EXISTS "${BROTLI_INCLUDE_DIR}/brotli/encode.h") + FOREACH(onelib ${BROTLI_LIBRARIES}) +IF(NOT EXISTS ${onelib}) + SET(BROTLI_FOUND FALSE) +ENDIF() + ENDFOREACH() +ELSE() + SET(BROTLI_FOUND FALSE) +ENDIF() + MESSAGE(STATUS "") MESSAGE(STATUS "Summary of feature detection:") MESSAGE(STATUS "") @@ -199,6 +219,7 @@ MESSAGE(STATUS "LUA51_FOUND .. : ${LUA MESSAGE(STATUS "NGHTTP2_FOUND : ${NGHTTP2_FOUND}") MESSAGE(STATUS "OPENSSL_FOUND : ${OPENSSL_FOUND}") MESSAGE(STATUS "ZLIB_FOUND ... : ${ZLIB_FOUND}") +MESSAGE(STATUS "BROTLI_FOUND . : ${BROTLI_FOUND}") MESSAGE(STATUS "APR_HAS_LDAP . : ${APR_HAS_LDAP}") MESSAGE(STATUS "APR_HAS_XLATE : ${APR_HAS_XLATE}") MESSAGE(STATUS "APU_HAVE_CRYPTO .. : ${APU_HAVE_CRYPTO}") @@ -273,6 +294,7 @@ SET(MODULE_LIST "modules/examples/mod_case_filter_in+O+Example uppercase conversion input filter" "modules/examples/mod_example_hooks+O+Example hook callback handler module" "modules/examples/mod_example_ipc+O+Example of shared memory and mutex usage" + "modules/filters/mod_brotli+i+Brotli compression support" "modules/filters/mod_buffer+I+Filter Buffering" "modules/filters/mod_charset_lite+i+character set translation" "modules/filters/mod_data+O+RFC2397 data encoder" @@ -393,6 +415,11 @@ IF(ZLIB_FOUND) SET(mod_deflate_extra_includes ${ZLIB_INCLUDE_DIR}) SET(mod_deflate_extra_libs ${ZLIB_LIBRAR
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Christophe JAILLET 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_nhm8d7wjyfsf4ouoteuepkj...@mail.gmail.com%3E [2] https://svn.apache.org/r1756561 Regards, Evgeny Kotkov
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Evgeny Kotkov 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/...
Joe Orton 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 '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 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 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, ""); + ap_fputs(r->output_filters, bb, ""); } else { - ap_fputs(output, bb, "output_filters, bb, "propresult.xmlns; t; t = t->next) { -ap_fputs(output, bb, t->text); +ap_fputs(r->output_filters, bb, t->text);
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 DAV svn SVNParentPath C:/Repositories 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: [PATCH] mod_proxy_http2: fix proxying of 32K+ sized responses
Jim Jagielski writes: > Thanks for the patch... It came thru, at least for me, mangled. > > Could you resend? I think that's because gmail did base64 encoding for the attachment. The unmangled patch is available here: https://mail-archives.apache.org/mod_mbox/httpd-dev/201606.mbox/raw/%3CCAP_GPNgHADdp2YbNB2CtraR_9x%2Bdg4Wss6Kd1ZG0BsU_v%2Bw2rw%40mail.gmail.com%3E/2 Regards, Evgeny Kotkov
[PATCH] mod_proxy_http2: fix proxying of 32K+ sized responses
I noticed that currently mod_proxy_http2 cannot serve responses larger than 32 KiB for incoming requests over HTTP/1.1. Below is a simple config that can be used to reproduce the problem: LoadModule http2_module modules/mod_http2.so LoadModule proxy_module modules/mod_proxy.so LoadModule proxy_http_module modules/mod_proxy_http.so LoadModule proxy_http2_module modules/mod_proxy_http2.so ProxyPass "/" "h2c://nghttp2.org/" Issuing an HTTP/1.1 request to a file that exceeds 32 KiB is going to stall and eventually fail: curl --http1.1 http://localhost/stylesheets/screen.css ... curl: (18) transfer closed with 6016 bytes remaining to read The reason is that in this particular case mod_proxy_http2 doesn't update the HTTP/2 flow control windows. Once the amount of the received data hits a hardcoded threshold of 32 KiB, no data can be received without a WINDOW_UPDATE, and since it doesn't happen, the request eventually times out. Please see the push_request_somewhere() function in mod_proxy_http2.c:441: if (!ctx->engine) { /* No engine was available or has been initialized, handle this * request just by ourself. */ ctx->engine_id = apr_psprintf(ctx->pool, "eng-proxy-%ld", c->id); ctx->engine_type = engine_type; ctx->engine_pool = ctx->pool; ctx->req_buffer_size = (32*1024); ctx->standalone = 1; ... Currently, mod_proxy_http2 can either handle the request to a backend by itself or push the handling of the request to the mod_http2's request engine. In the latter case, the HTTP/2 flow control windows are updated per the h2_req_engine_out_consumed() function calls. But if mod_proxy_http2 processes the request by itself, they are not. This is exactly what happens in the described case with the HTTP/1.1 request. I attached the patch with a fix for this issue. The idea behind the patch is that we keep track of how a particular request is handled and call the nghttp2_session_consume() function if that's required. Regards, Evgeny Kotkov Index: modules/http2/h2_proxy_session.c === --- modules/http2/h2_proxy_session.c(revision 1747688) +++ modules/http2/h2_proxy_session.c(working copy) @@ -36,6 +36,7 @@ typedef struct h2_proxy_stream { const char *url; request_rec *r; h2_request *req; +int standalone; h2_stream_state_t state; unsigned int suspended : 1; @@ -370,6 +371,12 @@ static int on_data_chunk_recv(nghttp2_session *ngh stream_id, NGHTTP2_STREAM_CLOSED); return NGHTTP2_ERR_STREAM_CLOSING; } +if (stream->standalone) { +nghttp2_session_consume(ngh2, stream_id, len); +ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, stream->r, + "h2_proxy_session(%s): stream %d, win_update %d bytes", + session->id, stream_id, (int)len); +} return 0; } @@ -576,7 +583,8 @@ static apr_status_t session_start(h2_proxy_session } static apr_status_t open_stream(h2_proxy_session *session, const char *url, -request_rec *r, h2_proxy_stream **pstream) +request_rec *r, int standalone, +h2_proxy_stream **pstream) { h2_proxy_stream *stream; apr_uri_t puri; @@ -588,6 +596,7 @@ static apr_status_t open_stream(h2_proxy_session * stream->pool = r->pool; stream->url = url; stream->r = r; +stream->standalone = standalone; stream->session = session; stream->state = H2_STREAM_ST_IDLE; @@ -761,12 +770,13 @@ static apr_status_t h2_proxy_session_read(h2_proxy } apr_status_t h2_proxy_session_submit(h2_proxy_session *session, - const char *url, request_rec *r) + const char *url, request_rec *r, + int standalone) { h2_proxy_stream *stream; apr_status_t status; -status = open_stream(session, url, r, &stream); +status = open_stream(session, url, r, standalone, &stream); if (status == APR_SUCCESS) { ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03381) "process stream(%d): %s %s%s, original: %s", Index: modules/http2/h2_proxy_session.h === --- modules/http2/h2_proxy_session.h(revision 1747688) +++ modules/http2/h2_proxy_session.h(working copy) @@ -89,7 +89,7 @@ h2_proxy_session *h2_proxy_session_setup(const cha h2_proxy_request_done *done); apr_status_t h2_proxy_session_submit(h2_proxy_session *s, const char *url, - request_rec *r); +
[PATCH] mod_http2: fix undefined behavior with LogLevel trace
This patch fixes an instance of undefined behavior in mod_http2 with LogLevel >= trace2. Please see the h2_h2_process_conn() function in h2_h2.c:631. The call to ap_log_cerror() passes a pointer to a non-null terminated buffer while specifying %s in the format string. This causes an out-of-bounds access, and the behavior is undefined: h2_h2.c(631): [client 127.0.0.1:22398] h2_h2, not detected in 24 bytes: GET /Azimuthal_equidista\xcd\xcd\xcd\xcd\xcd\xcd\xcd\xcd\xcd\xcd \xcd\xcd\xcd\xcd\xcd\xcd\xcd\xcd\xcd\xcd\xcd\xcd\xcd\xcd\xcd\xcd... I attached the patch with a fix for this issue. Regards, Evgeny Kotkov Index: modules/http2/h2_h2.c === --- modules/http2/h2_h2.c (revision 1747688) +++ modules/http2/h2_h2.c (working copy) @@ -629,8 +629,8 @@ int h2_h2_process_conn(conn_rec* c) } else { ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c, - "h2_h2, not detected in %d bytes: %s", - (int)slen, s); + "h2_h2, not detected in %d bytes: %.*s", + (int)slen, (int)slen, s); } apr_brigade_destroy(temp);
Re: [PATCH] mod_rewrite: support proxying via mod_proxy_http2
Stefan Eissing writes: > Hmm, can someone with more brains than me on mod_rewrite look at the > first patch if we want to add handling for h2: and h2c: uri schemes > here or if there is a better way? Thanks. In case this will help the review, here are some of the existing changes that target similar problems (mod_rewrite not picking up a new scheme for proxy URLs). I examined them prior to preparing the patch: https://svn.apache.org/r1528556 https://svn.apache.org/r405625 https://svn.apache.org/r124584 Regards, Evgeny Kotkov
[PATCH] mod_rewrite: support proxying via mod_proxy_http2
I noticed that it's currently impossible to use mod_proxy_http2 in conjunction with mod_rewrite's [P] request proxying: RewriteEngine On RewriteRule ^/foo h2c://hostname/bar [P] mod_proxy_http2 registers on h2:// and h2c:// proxy URLs [1], however, mod_rewrite needs an update to handle these as absolute URIs. Without that, the configuration above is going to produce incorrectly rewritten rewritten URLs, e.g.: http://www.example.org/h2c://hostname/bar I attached the patch with a fix for this issue. The second patch adds the corresponding tests to the mod_http2 testing framework. [1] https://mail-archives.apache.org/mod_mbox/httpd-dev/201602.mbox/%30ea0e-00ce-430d-a01c-022e7a2ff...@greenbytes.de%3E Regards, Evgeny Kotkov mod_rewrite: Add handling of 'h2' and 'h2c' schemes for mod_proxy_http2. Index: modules/mappers/mod_rewrite.c === --- modules/mappers/mod_rewrite.c (revision 1743495) +++ modules/mappers/mod_rewrite.c (working copy) @@ -568,6 +568,14 @@ static unsigned is_absolute_uri(char *uri, int *su *sqs = 1; return 8; } +else if (!ap_casecmpstrn(uri, "2://", 4)) {/* h2:// */ +*sqs = 1; +return 5; +} +else if (!ap_casecmpstrn(uri, "2c://", 5)) { /* h2c://*/ +*sqs = 1; +return 6; +} break; case 'l': Cover the mod_rewrite's [P] proxying in mod_proxy_http2 tests. Index: Makefile.am === --- Makefile.am (revision 1743495) +++ Makefile.am (working copy) @@ -223,6 +223,8 @@ proxytx: \ h2proxytx: \ $(SERVER_DIR)/.test-setup $(H2LOAD) -c 8 -t 8 -n 1000 -m 1 --npn-list=h2 https://$(HTTPS_AUTH_2)/h2proxy/005.txt + $(H2LOAD) -c 8 -t 8 -n 1000 -m 1 --npn-list=h2 https://$(HTTPS_AUTH_2)/h2proxy-rewrite/005.txt + $(H2LOAD) -c 8 -t 8 -n 1000 -m 1 http://$(HTTP_AUTH)/h2cproxy-rewrite/005.txt $(H2LOAD) -c 8 -t 8 -n 1000 -m 1 http://$(HTTP_AUTH)/h2cproxy/005.txt Index: conf/sites/test.example.org.conf === --- conf/sites/test.example.org.conf(revision 1743495) +++ conf/sites/test.example.org.conf(working copy) @@ -135,8 +135,10 @@ ProxyPass "/h2proxy" "balancer://h2-local" ProxyPassReverse "/h2proxy" "balancer://h2-local" +RewriteRule /h2proxy-rewrite(.*) h2://test.example.org:SUBST_PORT_HTTPS_SUBST$1 [P] ProxyPass "/h2cproxy" "balancer://h2c-local" ProxyPassReverse "/h2cproxy" "balancer://h2c-local" +RewriteRule /h2cproxy-rewrite(.*) h2c://test.example.org:SUBST_PORT_HTTP_SUBST$1 [P] = 2.4.19> @@ -209,8 +211,10 @@ ProxyPass "/h2proxy" "balancer://h2-local" ProxyPassReverse "/h2proxy" "balancer://h2-local" +RewriteRule /h2proxy-rewrite(.*) h2://test.example.org:SUBST_PORT_HTTPS_SUBST$1 [P] ProxyPass "/h2cproxy" "balancer://h2c-local" ProxyPassReverse "/h2cproxy" "balancer://h2c-local" +RewriteRule /h2cproxy-rewrite(.*) h2c://test.example.org:SUBST_PORT_HTTP_SUBST$1 [P] Index: conf/sites/test2.example.org.conf === --- conf/sites/test2.example.org.conf (revision 1743495) +++ conf/sites/test2.example.org.conf (working copy) @@ -10,6 +10,8 @@ DocumentRoot "SUBST_SERVER_ROOT_SUBST/htdocs/test.example.org" Protocols http/1.1 h2 +RewriteEngine on + SSLEngine on SSLCertificateFile conf/ssl/test.example.org.pem SSLCertificateKeyFile conf/ssl/test.example.org.key @@ -29,8 +31,10 @@ ProxyPass "/h2proxy" "balancer://h2-local" ProxyPassReverse "/h2proxy" "balancer://h2-local" +RewriteRule /h2proxy-rewrite(.*) h2://test.example.org:SUBST_PORT_HTTPS_SUBST$1 [P] ProxyPass "/h2cproxy" "balancer://h2c-local" ProxyPassReverse "/h2cproxy" "balancer://h2c-local" +RewriteRule /h2cproxy-rewrite(.*) h2c://test.example.org:SUBST_PORT_HTTP_SUBST$1 [P] @@ -59,8 +63,10 @@ ProxyPass "/h2proxy" "balancer://h2-local" ProxyPassReverse "/h2proxy" "balancer://h2-local" +RewriteRule /h2proxy-rewrite(.*) h2://test.example.org:SUBST_PORT_HTTPS_SUBST$1 [P] ProxyPass "/h2cproxy" "balancer://h2c-local" Pro
[PATCH] mod_proxy_http2: fix theoretically possible segfault when parsing URL
This patch fixes a theoretically possible segfault in mod_proxy_http2. Please see the open_stream() function in h2_proxy_session.c:598. If the call to apr_uri_parse() fails, some of the apr_uri_t's fields may be set to NULL, and this would cause a segfault in the following lines: authority = puri.hostname; if (!ap_strchr_c(authority, ':') && puri.port ... Currently, the URIs are preprocessed by ap_proxy_canon_netloc() much earlier than opening the proxy stream, but this may not hold in the future. As well as that, I think that there might be subtle differences between how these two functions handle invalid URIs, and that could also result in the crash. I attached the patch with a fix for this issue. While here, I also spotted an opportunity for a minor code cleanup. There are a couple of places where a function returns an apr_status_t, but the calling site tests the result against the OK (hook return code). I updated such places in the second patch. Regards, Evgeny Kotkov mod_proxy_http2: Guard against theoretically possible segfault with invalid URIs. Index: modules/http2/h2_proxy_session.c === --- modules/http2/h2_proxy_session.c(revision 1743495) +++ modules/http2/h2_proxy_session.c(working copy) @@ -581,6 +581,7 @@ static apr_status_t open_stream(h2_proxy_session * h2_proxy_stream *stream; apr_uri_t puri; const char *authority, *scheme, *path; +apr_status_t status; stream = apr_pcalloc(r->pool, sizeof(*stream)); @@ -595,7 +596,10 @@ static apr_status_t open_stream(h2_proxy_session * stream->req = h2_req_create(1, stream->pool, 0); -apr_uri_parse(stream->pool, url, &puri); +status = apr_uri_parse(stream->pool, url, &puri); +if (status != APR_SUCCESS) +return status; + scheme = (strcmp(puri.scheme, "h2")? "http" : "https"); authority = puri.hostname; if (!ap_strchr_c(authority, ':') && puri.port mod_proxy_http2: Fix minor inconsistency when checking apr_status_t values. Index: modules/http2/h2_proxy_session.c === --- modules/http2/h2_proxy_session.c(revision 1743495) +++ modules/http2/h2_proxy_session.c(working copy) @@ -763,7 +763,7 @@ apr_status_t h2_proxy_session_submit(h2_proxy_sess apr_status_t status; status = open_stream(session, url, r, &stream); -if (status == OK) { +if (status == APR_SUCCESS) { ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03381) "process stream(%d): %s %s%s, original: %s", stream->id, stream->req->method, Index: modules/http2/mod_proxy_http2.c === --- modules/http2/mod_proxy_http2.c (revision 1743495) +++ modules/http2/mod_proxy_http2.c (working copy) @@ -259,7 +259,7 @@ static apr_status_t add_request(h2_proxy_session * apr_table_setn(r->notes, "proxy-source-port", apr_psprintf(r->pool, "%hu", ctx->p_conn->connection->local_addr->port)); status = h2_proxy_session_submit(session, url, r); -if (status != OK) { +if (status != APR_SUCCESS) { ap_log_cerror(APLOG_MARK, APLOG_ERR, status, r->connection, APLOGNO(03351) "pass request body failed to %pI (%s) from %s (%s)", ctx->p_conn->addr, ctx->p_conn->hostname ?
[PATCH] mod_http2, mod_proxy_http2: fix CMake support
I noticed a few issues in CMakeLists.txt that currently prevent building mod_http2 and mod_proxy_http2 — that is, missing includes and library references. The attached patch should fix them. Regards, Evgeny Kotkov Fix CMake support for mod_http2 and mod_proxy_http2. Index: CMakeLists.txt === --- CMakeLists.txt (revision 1743495) +++ CMakeLists.txt (working copy) @@ -396,6 +396,7 @@ SET(mod_heartbeat_extra_libs mod_watchdog) SET(mod_http2_requires NGHTTP2_FOUND) SET(mod_http2_extra_defines ssize_t=long) +SET(mod_http2_extra_includes ${NGHTTP2_INCLUDE_DIR}) SET(mod_http2_extra_libs ${NGHTTP2_LIBRARIES}) SET(mod_http2_extra_sources modules/http2/h2_alt_svc.c modules/http2/h2_bucket_eoc.c @@ -451,11 +452,11 @@ SET(mod_proxy_wstunnel_extra_libsmod_proxy) SET(mod_proxy_http2_requires NGHTTP2_FOUND) SET(mod_proxy_http2_extra_defines ssize_t=long) -SET(mod_proxy_http2_extra_libs ${NGHTTP2_LIBRARIES}) +SET(mod_proxy_http2_extra_includes ${NGHTTP2_INCLUDE_DIR}) +SET(mod_proxy_http2_extra_libs ${NGHTTP2_LIBRARIES} mod_proxy) SET(mod_proxy_http2_extra_sources modules/http2/h2_proxy_session.c modules/http2/h2_util.c ) -SET(mod_proxy_http2_extra_libs mod_proxy) SET(mod_ratelimit_extra_defines AP_RL_DECLARE_EXPORT) SET(mod_sed_extra_sources modules/filters/regexp.c modules/filters/sed0.c
[PATCH] mod_rewrite: double escaping of query strings in server context
This patch fixes a problem in mod_rewrite. The following config with the RewriteRule in per-directory context works fine for query strings: RewriteEngine On RewriteRule ^foo http://hostname/bar [R=301,L] But the same RewriteRule in per-server context doesn't work: RewriteEngine On RewriteRule ^/foo http://hostname/bar [R=301,L] The second example leads to double URL-escaping of the query string: GET /foo?q=%25 301 Moved Permanently Location: http://hostname/bar?q=%25%25 This is PR 50447 [1], and the issue was fixed for per-directory contexts in r1044673 [2]. However, RewriteRules in server context are handled differently in mod_rewrite, and their handling wasn't updated. The attached patch applies the same fix for such rules. The second patch adds a regression test for the issue in t/modules/rewrite.t. Both of the patches are against trunk. [1] https://bz.apache.org/bugzilla/show_bug.cgi?id=50447 [2] https://svn.apache.org/r1044673 Regards, Evgeny Kotkov mod_rewrite: Fix double escaping of the query string for RewriteRules in server context. See PR 50447 and r1044673 that fixed this for directory contexts. Index: modules/mappers/mod_rewrite.c === --- modules/mappers/mod_rewrite.c (revision 1735001) +++ modules/mappers/mod_rewrite.c (working copy) @@ -4548,6 +4548,7 @@ static int hook_uri2file(request_rec *r) unsigned int port; int rulestatus; void *skipdata; +const char *oargs; /* * retrieve the config structures @@ -4598,6 +4599,12 @@ static int hook_uri2file(request_rec *r) } /* + * remember the original query string for later check, since we don't + * want to apply URL-escaping when no substitution has changed it. + */ +oargs = r->args; + +/* * add the SCRIPT_URL variable to the env. this is a bit complicated * due to the fact that apache uses subrequests and internal redirects */ @@ -4731,11 +4738,21 @@ static int hook_uri2file(request_rec *r) /* append the QUERY_STRING part */ if (r->args) { +char *escaped_args = NULL; +int noescape = (rulestatus == ACTION_NOESCAPE || +(oargs && !strcmp(r->args, oargs))); + r->filename = apr_pstrcat(r->pool, r->filename, "?", - (rulestatus == ACTION_NOESCAPE) + noescape ? r->args -: ap_escape_uri(r->pool, r->args), +: (escaped_args = + ap_escape_uri(r->pool, r->args)), NULL); + +rewritelog((r, 1, NULL, "%s %s to query string for redirect %s", +noescape ? "copying" : "escaping", +r->args , +noescape ? "" : escaped_args)); } /* determine HTTP redirect response code */ mod_rewrite: Add regression tests for PR 50447 (double URL-escaping of the query string). Index: t/conf/extra.conf.in === --- t/conf/extra.conf.in(revision 1735001) +++ t/conf/extra.conf.in(working copy) @@ -234,6 +234,9 @@ ## Proxy and QSA RewriteRule ^proxy-qsa.html$ http://@SERVERNAME@:@PORT@/modules/cgi/env.pl?foo=bar [QSA,L,P] +## Redirect, directory context +RewriteRule ^redirect-dir.html$ http://@SERVERNAME@:@PORT@/foobar.html [L,R=301] + ### Proxy pass-through to env.pl @@ -243,6 +246,9 @@ RewriteCond %{QUERY_STRING} horse=trigger RewriteRule ^/modules/rewrite/proxy3/(.*)$ http://@SERVERNAME@:@PORT@/modules/cgi/$1 [L,P] +### Redirect, server context +RewriteRule ^/modules/rewrite/redirect.html$ http://@SERVERNAME@:@PORT@/foobar.html [L,R=301] + DocumentRoot @SERVERROOT@/htdocs/modules/proxy RewriteEngine On Index: t/modules/rewrite.t === --- t/modules/rewrite.t (revision 1735001) +++ t/modules/rewrite.t (working copy) @@ -12,11 +12,20 @@ use Apache::TestUtil; my @map = qw(txt rnd prg); #dbm XXX: howto determine dbm support is available? my @num = qw(1 2 3 4 5 6); my @url = qw(forbidden gone perm temp); -my @todo = (); +my @todo; my $r; -plan tests => @map * @num + 11, todo => \@todo, need_module 'rewrite'; +if (!have_min_apache_version('2.5')) { +# PR 50447, server context +push @todo, 26 +} +if (!have_min_apache_version('2.4'