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, );
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_HEADE
Re: [PATCH] Remove unused enum values in mpm_winnt
Ivan Zhakov <i...@visualsvn.com> 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 <evgeny.kot...@visualsvn.com> 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,
- IOCP_SHUTDOWN, NULL)
Re: mod_brotli in 2.4.x is missing a few Makefile changes
Jim Jagielski <j...@jagunet.com> 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 <j...@jagunet.com> 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 <wr...@rowe-clan.net> 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 <j...@jagunet.com> 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 <no...@gknw.net> 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 <evgeny.kot...@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: C89 (alias NetWare) tries new mod_ on the block.
NormW <no...@gknw.net> 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, >> _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 <rpl...@apache.org> 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 <evgeny.kot...@visualsvn.com> 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 <champio...@gmail.com> 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 <h.rei...@thelounge.net> 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 <cove...@gmail.com> 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 <h.rei...@thelounge.net> 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_LIBRARIES})
END
Re: Unbounded memory usage in mod_dav + mod_headers/mod_deflate/...
Christophe JAILLET <christophe.jail...@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_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 <evgeny.kot...@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/...
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, );
> 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, "<D:response");
+ ap_fputs(r->output_filters, bb, "<D:response");
for (t = response->propresult.xmlns; t; t = t->next) {
-ap_fputs(output, bb, t->t
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 <j...@jagunet.com> 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_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 <stefan.eiss...@greenbytes.de> 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"
ProxyPassReverse "/h2cproxy" &qu
[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, );
+status = apr_uri_parse(stream->pool, url, );
+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, );
-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')) {
+# PR 50447, directory context (r104
