Re: [Patch] Async write completion for the full connection filter stack
On 10 Sep 2014, at 18:19, Jim Jagielski j...@jagunet.com wrote: On Sep 10, 2014, at 12:07 PM, Graham Leggett minf...@sharp.fm wrote: Having thought long and hard about this, giving filters an opportunity to write has nothing to do with either data or metadata, we just want to give the filter an opportunity to write. “Dear filter, please wake up and if you’ve setaside data please write it, otherwise do nothing, thanks”. Empty brigade means “do nothing new”. Hey filter, here is an empty brigade, meaning I have no data for you; if you have setaside data, now is the time to push it thru. If someone is new to writing filters, I think that's a bit of a gotcha. An empty brigade meaning “do nothing new” is easy to code for and unsurprising. I'm thinking about how I'd help httpd cope with a filter that didn't know about this special behaviour. I would introduce a new bucket: NUDGE, and use that to wake up filters. If a filter doesn't understand NUDGE, this could cause a hang from the client's point of view. To avoid this, I'd have a configurable timeout after which a NUDGE-d filter would get a FLUSH bucket - say 30 seconds as a default, nice and high because FLUSH is an expensive operation. Server admins could drop this if they know that they have a problematic filter. If a filter gets a NUDGE and returns APR_SUCCESS I think it makes sense to NUDGE the next downstream filter. A new metadata bucket has all sorts of compatibility issues that need thinking about (and I don't know enough about). Changing the meaning of “empty brigade” also has compatibility issues but they will show up much later than build time. -- Tim Bannister – is...@jellybaby.net
RE: [Patch] Async write completion for the full connection filter stack
From: Graham Leggett [mailto:minf...@sharp.fm]
Sent: Donnerstag, 11. September 2014 06:40
To: dev@httpd.apache.org
Subject: Re: [Patch] Async write completion for the full connection filter stack
On 11 Sep 2014, at 1:51 AM, Yann Ylavic
ylavic@gmail.commailto:ylavic@gmail.com wrote:
+else if (*deferred_write_pool) {
+/*
+ * There are no more requests in the pipeline. We can just clear the
+ * pool.
+ */
Shouldn't *buffered_bb be set to NULL here when *deferred_write_pool
== (*buffered_bb)-p, or more generally
apr_pool_is_ancestor(*deferred_write_pool, (*buffered_bb)-p). We
can't leave a dangling pointer in this case.
+apr_pool_clear(*deferred_write_pool);
Hmmm... this came from the original code.
We can't set buffered_bb to NULL unless we are sure we created buffered_bb, and
this isn't necessarily the case. In the core filter, buffered_bb is created
when the connection is created.
How about doing a
apr_brigade_cleanup(buffered_bb)
before the apr_pool_clear?
Regards
Rüdiger
Re: mod_proxy: PHP SCRIPT_FILENAME (PHP-FPM using UDS) and Apache documentation
On 09/10/2014 07:17 PM, Jim Jagielski wrote: I know that PHP is current doing a LOT of fixes on hPHP-FPM... I've recently come to https://bugs.php.net/bug.php?id=65641 and was thinking if we can do anything about it. Jan Kaluza On Sep 10, 2014, at 12:00 PM, Martynas Bendorius marty...@martynas.it wrote: Hello, http://httpd.apache.org/docs/current/mod/mod_proxy.html#handler breaks PHP-FPM SCRIPT_FILENAME. It contains double // at the beginning of it like: SCRIPT_FILENAME: //home/admin/domains/testing.tld/public_html/test.php While it should be: SCRIPT_FILENAME: /home/admin/domains/testing.tld/public_html/test.php Replacing localhost/ to just localhost fixes the problem (removing / from the end). I mean: SetHandler proxy:unix:/path/to/app.sock|fcgi://localhost Instead of: SetHandler proxy:unix:/path/to/app.sock|fcgi://localhost/ Should it be considered a typo in Apache documentation or a bug in the way PHP-FPM SAPI translates the path? Thank you! -- Best regards, Martynas Bendorius
RE: [Patch] Async write completion for the full connection filter stack
-Original Message-
From: Yann Ylavic [mailto:ylavic@gmail.com]
Sent: Donnerstag, 11. September 2014 09:52
To: httpd
Subject: Re: [Patch] Async write completion for the full connection filter
stack
On Thu, Sep 11, 2014 at 6:39 AM, Graham Leggett minf...@sharp.fm wrote:
On 11 Sep 2014, at 1:51 AM, Yann Ylavic ylavic@gmail.com wrote:
+else if (*deferred_write_pool) {
+/*
+ * There are no more requests in the pipeline. We can just
clear the
+ * pool.
+ */
Shouldn't *buffered_bb be set to NULL here when *deferred_write_pool
== (*buffered_bb)-p, or more generally
apr_pool_is_ancestor(*deferred_write_pool, (*buffered_bb)-p). We
can't leave a dangling pointer in this case.
+apr_pool_clear(*deferred_write_pool);
Hmmm… this came from the original code.
We can’t set buffered_bb to NULL unless we are sure we created
buffered_bb, and this isn’t necessarily the case. In the core filter,
buffered_bb is created when the connection is created.
Another question is if we should clear a pool we haven't created.
If we don't then apr_pool_is_ancestor(*deferred_write_pool, (*buffered_bb)-p)
should be a save bet that we created the bucket brigade.
Another approach below.
If we or any filter has created the brigade on *deferred_write_pool
(or an ancestor, I'm only talking about this case), it is dead now
(unaccessible), and IMO should be marked as so.
In the case the caller let the function handle the brigade (as we do),
using NULL the first time only, (s)he expects to not crash whenever
(s)he come back with it after a clear (which (s)he isn't supposed to
know about).
Agree. Maybe if buffered_bb is NULL we should add a pool clean up to
*deferred_write_pool that sets *buffered_bb to NULL
Another solution would be to not require a pool at all, something like :
I fear that this creates high memory consumption / temporary memory leaks.
Regards
Rüdiger
Re: mod_proxy: PHP SCRIPT_FILENAME (PHP-FPM using UDS) and Apache documentation
Recall that that when used, the req is basically tacked on what is provided in the SetHandler, so whether a trailing / is required or not depends 100% on how the app is expecting it to be. On Sep 10, 2014, at 1:26 PM, Martynas Bendorius marty...@martynas.it wrote: Yes, I've tried their latest versions from GIT (with the #65641 fix (PHP-FPM incorrectly defines the SCRIPT_NAME variable when using Apache)). It still has the same problem with SCRIPT_FILENAME. Is there a special reason why / is required at the end? As it doesn't seem to break anything when the trailing slash is omitted from the end. Thank you! Best regards, Martynas Bendorius On 9/10/14 8:17 PM, Jim Jagielski wrote: I know that PHP is current doing a LOT of fixes on hPHP-FPM... On Sep 10, 2014, at 12:00 PM, Martynas Bendorius marty...@martynas.it wrote: Hello, http://httpd.apache.org/docs/current/mod/mod_proxy.html#handler breaks PHP-FPM SCRIPT_FILENAME. It contains double // at the beginning of it like: SCRIPT_FILENAME: //home/admin/domains/testing.tld/public_html/test.php While it should be: SCRIPT_FILENAME: /home/admin/domains/testing.tld/public_html/test.php Replacing localhost/ to just localhost fixes the problem (removing / from the end). I mean: SetHandler proxy:unix:/path/to/app.sock|fcgi://localhost Instead of: SetHandler proxy:unix:/path/to/app.sock|fcgi://localhost/ Should it be considered a typo in Apache documentation or a bug in the way PHP-FPM SAPI translates the path? Thank you! -- Best regards, Martynas Bendorius
Re: mod_proxy: PHP SCRIPT_FILENAME (PHP-FPM using UDS) and Apache documentation
When *we* generate SCRIPT_FILENAME, we simply use what r-filename is. On Sep 11, 2014, at 3:04 AM, Jan Kaluža jkal...@redhat.com wrote: On 09/10/2014 07:17 PM, Jim Jagielski wrote: I know that PHP is current doing a LOT of fixes on hPHP-FPM... I've recently come to https://bugs.php.net/bug.php?id=65641 and was thinking if we can do anything about it. Jan Kaluza On Sep 10, 2014, at 12:00 PM, Martynas Bendorius marty...@martynas.it wrote: Hello, http://httpd.apache.org/docs/current/mod/mod_proxy.html#handler breaks PHP-FPM SCRIPT_FILENAME. It contains double // at the beginning of it like: SCRIPT_FILENAME: //home/admin/domains/testing.tld/public_html/test.php While it should be: SCRIPT_FILENAME: /home/admin/domains/testing.tld/public_html/test.php Replacing localhost/ to just localhost fixes the problem (removing / from the end). I mean: SetHandler proxy:unix:/path/to/app.sock|fcgi://localhost Instead of: SetHandler proxy:unix:/path/to/app.sock|fcgi://localhost/ Should it be considered a typo in Apache documentation or a bug in the way PHP-FPM SAPI translates the path? Thank you! -- Best regards, Martynas Bendorius
Re: mod_proxy: PHP SCRIPT_FILENAME (PHP-FPM using UDS) and Apache documentation
Hmmm... ProxyPassMatch .*/sample/(test.php.*) fcgi://127.0.0.1:9000//tmp/sample/$1 So they *put* the '//' there. On Sep 11, 2014, at 3:04 AM, Jan Kaluža jkal...@redhat.com wrote: On 09/10/2014 07:17 PM, Jim Jagielski wrote: I know that PHP is current doing a LOT of fixes on hPHP-FPM... I've recently come to https://bugs.php.net/bug.php?id=65641 and was thinking if we can do anything about it. Jan Kaluza On Sep 10, 2014, at 12:00 PM, Martynas Bendorius marty...@martynas.it wrote: Hello, http://httpd.apache.org/docs/current/mod/mod_proxy.html#handler breaks PHP-FPM SCRIPT_FILENAME. It contains double // at the beginning of it like: SCRIPT_FILENAME: //home/admin/domains/testing.tld/public_html/test.php While it should be: SCRIPT_FILENAME: /home/admin/domains/testing.tld/public_html/test.php Replacing localhost/ to just localhost fixes the problem (removing / from the end). I mean: SetHandler proxy:unix:/path/to/app.sock|fcgi://localhost Instead of: SetHandler proxy:unix:/path/to/app.sock|fcgi://localhost/ Should it be considered a typo in Apache documentation or a bug in the way PHP-FPM SAPI translates the path? Thank you! -- Best regards, Martynas Bendorius
Re: [Patch] Async write completion for the full connection filter stack
On Thu, Sep 11, 2014 at 9:51 AM, Yann Ylavic ylavic@gmail.com wrote:
On Thu, Sep 11, 2014 at 6:39 AM, Graham Leggett minf...@sharp.fm wrote:
I don’t follow - that means something different as I’m reading it. We must
only signal that we have less data in the output filters if we actually
originally had data in the output filters, ie buffered_bb existed and wasn’t
empty.
You're right, I misread ap_core_output_filter() where I thought
ap_filter_reinstate_brigade() wasn't called when
APR_BRIGADE_EMPTY(bb).
Since ap_filter_reinstate_brigade() can be called with an empty bb,
ap_core_output_filter() can (still) be simplified though, ie :
should_write = ap_filter_reinstate_brigade(f, ctx-buffered_bb, bb,
flush_upto);
if (APR_BRIGADE_EMPTY(bb)) {
return APR_SUCCESS;
}
if (flush_upto != NULL) {
...
}
if (should_write) {
...
}
flush_upto will always be NULL with an empty bb.
Again I missed something...
This code is indeed what I'd like ap_core_output_filter() (or any
filter) to be able to do, but it won't work without a slight change in
ap_filter_reinstate_brigade(), which is :
AP_DECLARE(int) ap_filter_reinstate_brigade(ap_filter_t *f,
apr_bucket_brigade *buffered_bb,
apr_bucket_brigade *bb,
apr_bucket **flush_upto)
{
apr_bucket *bucket, *next;
apr_size_t bytes_in_brigade, non_file_bytes_in_brigade;
int eor_buckets_in_brigade, morphing_bucket_in_brigade;
int loglevel = ap_get_conn_module_loglevel(f-c, APLOG_MODULE_INDEX);
*flush_upto = NULL;
if ((buffered_bb != NULL) !APR_BRIGADE_EMPTY(buffered_bb)) {
int flush = APR_BRIGADE_EMPTY(bb);
APR_BRIGADE_PREPEND(bb, buffered_bb);
f-c-data_in_output_filters--;
if (flush) {
return 1;
}
}
...
}
The new thing is the flush check to return 1 immediatly when an
empty bb is given.
In this case the intent of the caller is to use what we have (push it
thru would say Jim), we don't need to walk the buffered_bb again.
That's, I think, what the original code did in ap_core_output_filter()
to send_brigade_nonblocking() immediatly in this case, and what the
current patch misses.
Thoughts?
Re: [Patch] Async write completion for the full connection filter stack
On Thu, Sep 11, 2014 at 2:12 PM, Yann Ylavic ylavic@gmail.com wrote:
AP_DECLARE(int) ap_filter_reinstate_brigade(ap_filter_t *f,
apr_bucket_brigade *buffered_bb,
apr_bucket_brigade *bb,
apr_bucket **flush_upto)
{
apr_bucket *bucket, *next;
apr_size_t bytes_in_brigade, non_file_bytes_in_brigade;
int eor_buckets_in_brigade, morphing_bucket_in_brigade;
int loglevel = ap_get_conn_module_loglevel(f-c, APLOG_MODULE_INDEX);
*flush_upto = NULL;
Oups, this is an unrelated change, but probably good to have too.
mod_status: Apache 2.4 incorrect IP (proxy, not useragent_ip) on server-status page
Hello, Would it be possible to change the documentation of mod_remoteip for 2.4 (http://httpd.apache.org/docs/2.4/mod/mod_remoteip.html), and get is reported by mod_status removed from the page? As it leds Apache customers to believe that it will report a real (useragent) IP instead of a proxy one in server-status page. useragent_ip is not even available in scoreboard, which is used by mod_status, so it's not available for mod_status. This has been already discussed here: https://issues.apache.org/bugzilla/show_bug.cgi?id=55886 Thank you! Best regards, Martynas Bendorius
Re: mod_status: Apache 2.4 incorrect IP (proxy, not useragent_ip) on server-status page
isn't the question rather What should ap_get_remote_host() return?? On Sep 11, 2014, at 8:17 AM, Martynas Bendorius marty...@martynas.it wrote: Hello, Would it be possible to change the documentation of mod_remoteip for 2.4 (http://httpd.apache.org/docs/2.4/mod/mod_remoteip.html), and get is reported by mod_status removed from the page? As it leds Apache customers to believe that it will report a real (useragent) IP instead of a proxy one in server-status page. useragent_ip is not even available in scoreboard, which is used by mod_status, so it's not available for mod_status. This has been already discussed here: https://issues.apache.org/bugzilla/show_bug.cgi?id=55886 Thank you! Best regards, Martynas Bendorius
Re: mod_status: Apache 2.4 incorrect IP (proxy, not useragent_ip) on server-status page
Yes, we may re-phrase it like that, if we'd like to fix it in apache source (and not documentation) :) Currently ap_get_remote_host in server/core.c doesn't return useragent_ip, and instead of it we get conn-client_ip. Best regards, Martynas Bendorius On 9/11/14 4:21 PM, Jim Jagielski wrote: isn't the question rather What should ap_get_remote_host() return?? On Sep 11, 2014, at 8:17 AM, Martynas Bendorius marty...@martynas.it wrote: Hello, Would it be possible to change the documentation of mod_remoteip for 2.4 (http://httpd.apache.org/docs/2.4/mod/mod_remoteip.html), and get is reported by mod_status removed from the page? As it leds Apache customers to believe that it will report a real (useragent) IP instead of a proxy one in server-status page. useragent_ip is not even available in scoreboard, which is used by mod_status, so it's not available for mod_status. This has been already discussed here: https://issues.apache.org/bugzilla/show_bug.cgi?id=55886 Thank you! Best regards, Martynas Bendorius
RE: Re: mod_status: Apache 2.4 incorrect IP (proxy, not useragent_ip) on server-status page
+1, this is the right question, Jim. From the docs for mod_remoteip; This module is used to treat the useragent which initiated the request as the originating useragent as identified by httpd for the purposes of authorization and logging The module overrides the client IP address for the connection Once replaced as instructed, this overridden useragent IP address is then used Any other behavior is invalid to users of mod_remoteip. It was correctly observed that there is an intermediate state, following the logging of a request and destruction of the request pool, where the identity of the keep-alive connection truly belongs to the direct-remote user agent, and is no longer an attribute of the proxied request. Therefore, falling back to the c-remote_addr is entirely appropriate, and that remote_addr must be used as the basis for mod_remoteip to handshake the next reported remote client ip. So here's a +1 to changing the behavior of ap_get_remote_host, as documented, the existing behavior is flawed. - Original Message - Subject: Re: mod_status: Apache 2.4 incorrect IP (proxy, not useragent_ip) on server-status page From: Martynas Bendorius marty...@martynas.it Date: 9/11/14 8:35 am To: dev@httpd.apache.org Yes, we may re-phrase it like that, if we'd like to fix it in apache source (and not documentation) :) Currently ap_get_remote_host in server/core.c doesn't return useragent_ip, and instead of it we get conn-client_ip. Best regards, Martynas Bendorius On 9/11/14 4:21 PM, Jim Jagielski wrote: isn't the question rather What should ap_get_remote_host() return?? On Sep 11, 2014, at 8:17 AM, Martynas Bendorius marty...@martynas.it wrote: Hello, Would it be possible to change the documentation of mod_remoteip for 2.4 (http://httpd.apache.org/docs/2.4/mod/mod_remoteip.html), and get is reported by mod_status removed from the page? As it leds Apache customers to believe that it will report a real (useragent) IP instead of a proxy one in server-status page. useragent_ip is not even available in scoreboard, which is used by mod_status, so it's not available for mod_status. This has been already discussed here: https://issues.apache.org/bugzilla/show_bug.cgi?id=55886 Thank you! Best regards, Martynas Bendorius
Re: [Patch] Async write completion for the full connection filter stack
On 11 Sep 2014, at 9:51 AM, Yann Ylavic ylavic@gmail.com wrote:
If we or any filter has created the brigade on *deferred_write_pool
(or an ancestor, I'm only talking about this case), it is dead now
(unaccessible), and IMO should be marked as so.
In the case the caller let the function handle the brigade (as we do),
using NULL the first time only, (s)he expects to not crash whenever
(s)he come back with it after a clear (which (s)he isn't supposed to
know about).
Agreed - as rpluem suggested a pool cleanup should be registered for this, I’ve
added it.
The core and mod_ssl deferred_write_pool won't be needed anymore.
The reason the deferred write pool exists is that connections might live for a
very long time. Any memory allocated during that time will accumulate and won’t
be cleaned up until the connection is closed. The deferred write pool allows us
to temporarily create data during the connection and then clean it out as soon
as we no longer need it.
You're right, I misread ap_core_output_filter() where I thought
ap_filter_reinstate_brigade() wasn't called when
APR_BRIGADE_EMPTY(bb).
Since ap_filter_reinstate_brigade() can be called with an empty bb,
ap_core_output_filter() can (still) be simplified though, ie :
should_write = ap_filter_reinstate_brigade(f, ctx-buffered_bb, bb,
flush_upto);
if (APR_BRIGADE_EMPTY(bb)) {
return APR_SUCCESS;
}
if (flush_upto != NULL) {
...
}
if (should_write) {
...
}
flush_upto will always be NULL with an empty bb.
This is indeed true - I’ve simplified it as above.
Regards,
Graham
—
Re: [Patch] Async write completion for the full connection filter stack
On 11 Sep 2014, at 2:12 PM, Yann Ylavic ylavic@gmail.com wrote:
Again I missed something...
This code is indeed what I'd like ap_core_output_filter() (or any
filter) to be able to do, but it won't work without a slight change in
ap_filter_reinstate_brigade(), which is :
AP_DECLARE(int) ap_filter_reinstate_brigade(ap_filter_t *f,
apr_bucket_brigade *buffered_bb,
apr_bucket_brigade *bb,
apr_bucket **flush_upto)
{
apr_bucket *bucket, *next;
apr_size_t bytes_in_brigade, non_file_bytes_in_brigade;
int eor_buckets_in_brigade, morphing_bucket_in_brigade;
int loglevel = ap_get_conn_module_loglevel(f-c, APLOG_MODULE_INDEX);
*flush_upto = NULL;
if ((buffered_bb != NULL) !APR_BRIGADE_EMPTY(buffered_bb)) {
int flush = APR_BRIGADE_EMPTY(bb);
APR_BRIGADE_PREPEND(bb, buffered_bb);
f-c-data_in_output_filters--;
if (flush) {
return 1;
}
}
...
}
The new thing is the flush check to return 1 immediatly when an
empty bb is given.
In this case the intent of the caller is to use what we have (push it
thru would say Jim), we don't need to walk the buffered_bb again.
That's, I think, what the original code did in ap_core_output_filter()
to send_brigade_nonblocking() immediatly in this case, and what the
current patch misses.
Thoughts?
We need this yes - if we’ve been called with an empty brigade and we still have
a buffered brigade that is “too small to write” we want to just write that
remaining brigade regardless of it’s size, otherwise we spin.
I would still do the flush check anyway - the ap_filter_reinstate_brigade() has
no idea what happened outside this function, we may have flushed buckets, we
may not have and setaside those flush buckets for whatever reason, and we want
the behaviour to remain predictable.
I’ve updated the patch to force the should_write if the original brigade was
empty.
Regards,
Graham
—
httpd-async-fullstack-ssl3.patch
Description: Binary data
RE: Re: mod_status: Apache 2.4 incorrect IP (proxy, not useragent_ip) on server-status page
However, the API is not going to make this trivial to fix. ap_get_remote_host is connection-based. And that is what mod_authz_host is currently relying upon. It seems that there needs to be a way for mod_remoteip to override the existing behavior, perhaps ap_set_remote_host(), that will cache the request-based on for the lifetime of the request pool. In the request pool cleanup, ap_set_remote_host(c, NULL) would clear that overridden request-based host, popping the value back to the cached c- fields. There is also the issue of the timing of setting the scoreboard record. All three issues are intertwined. - Original Message - Subject: Re: mod_status: Apache 2.4 incorrect IP (proxy, not useragent_ip) on server-status page From: Jim Jagielski j...@jagunet.com Date: 9/11/14 9:46 am To: dev@httpd.apache.org Yeah, the more I think about it, ap_get_remote_host() is currently broken wrt how it handles useragent_ip and client_ip. Will likely try to patch this on trunk sometime today... On Sep 11, 2014, at 9:35 AM, Martynas Bendorius marty...@martynas.it wrote: Yes, we may re-phrase it like that, if we'd like to fix it in apache source (and not documentation) :) Currently ap_get_remote_host in server/core.c doesn't return useragent_ip, and instead of it we get conn-client_ip. Best regards, Martynas Bendorius On 9/11/14 4:21 PM, Jim Jagielski wrote: isn't the question rather What should ap_get_remote_host() return?? On Sep 11, 2014, at 8:17 AM, Martynas Bendorius marty...@martynas.it wrote: Hello, Would it be possible to change the documentation of mod_remoteip for 2.4 (http://httpd.apache.org/docs/2.4/mod/mod_remoteip.html), and get is reported by mod_status removed from the page? As it leds Apache customers to believe that it will report a real (useragent) IP instead of a proxy one in server-status page. useragent_ip is not even available in scoreboard, which is used by mod_status, so it's not available for mod_status. This has been already discussed here: https://issues.apache.org/bugzilla/show_bug.cgi?id=55886 Thank you! Best regards, Martynas Bendorius
Re: mod_status: Apache 2.4 incorrect IP (proxy, not useragent_ip) on server-status page
Ugg. Yeah; we should actually have a complimentary version that takes request_req as the param, not conn_rec. ap_get_remote_host_r()?? Ugly. On Sep 11, 2014, at 11:28 AM, wr...@rowe-clan.net wrote: However, the API is not going to make this trivial to fix. ap_get_remote_host is connection-based. And that is what mod_authz_host is currently relying upon. It seems that there needs to be a way for mod_remoteip to override the existing behavior, perhaps ap_set_remote_host(), that will cache the request-based on for the lifetime of the request pool. In the request pool cleanup, ap_set_remote_host(c, NULL) would clear that overridden request-based host, popping the value back to the cached c- fields. There is also the issue of the timing of setting the scoreboard record. All three issues are intertwined. - Original Message - Subject: Re: mod_status: Apache 2.4 incorrect IP (proxy, not useragent_ip) on server-status page From: Jim Jagielski j...@jagunet.com Date: 9/11/14 9:46 am To: dev@httpd.apache.org Yeah, the more I think about it, ap_get_remote_host() is currently broken wrt how it handles useragent_ip and client_ip. Will likely try to patch this on trunk sometime today... On Sep 11, 2014, at 9:35 AM, Martynas Bendorius marty...@martynas.it wrote: Yes, we may re-phrase it like that, if we'd like to fix it in apache source (and not documentation) :) Currently ap_get_remote_host in server/core.c doesn't return useragent_ip, and instead of it we get conn-client_ip. Best regards, Martynas Bendorius On 9/11/14 4:21 PM, Jim Jagielski wrote: isn't the question rather What should ap_get_remote_host() return?? On Sep 11, 2014, at 8:17 AM, Martynas Bendorius marty...@martynas.it wrote: Hello, Would it be possible to change the documentation of mod_remoteip for 2.4 (http://httpd.apache.org/docs/2.4/mod/mod_remoteip.html), and get is reported by mod_status removed from the page? As it leds Apache customers to believe that it will report a real (useragent) IP instead of a proxy one in server-status page. useragent_ip is not even available in scoreboard, which is used by mod_status, so it's not available for mod_status. This has been already discussed here: https://issues.apache.org/bugzilla/show_bug.cgi?id=55886 Thank you! Best regards, Martynas Bendorius
RE: Re: mod_status: Apache 2.4 incorrect IP (proxy, not useragent_ip) on server-status page
- Original Message - Subject: Re: mod_status: Apache 2.4 incorrect IP (proxy, not useragent_ip) on server-status page From: Jim Jagielski j...@jagunet.com Date: 9/11/14 10:45 am To: dev@httpd.apache.org Ugg. Yeah; we should actually have a complimentary version that takes request_req as the param, not conn_rec. ap_get_remote_host_r()?? Considered that. But that still breaks mod_remoteip's advertised behavior against third party modules until they adapt. So I'd written this off as a non-starter :( Ugly. Indeed
Re: mod_status: Apache 2.4 incorrect IP (proxy, not useragent_ip) on server-status page
Am 11.09.2014 um 18:13 schrieb wr...@rowe-clan.net: Subject: Re: mod_status: Apache 2.4 incorrect IP (proxy, not useragent_ip) on server-status page From: Jim Jagielski j...@jagunet.com Date: 9/11/14 10:45 am To: dev@httpd.apache.org Ugg. Yeah; we should actually have a complimentary version that takes request_req as the param, not conn_rec. ap_get_remote_host_r()?? Considered that. But that still breaks mod_remoteip's advertised behavior against third party modules until they adapt. So I'd written this off as a non-starter :( yes - please don't break mod_security! it took me hours of discussions to bring developers to make it Apache 2.4 compliant and not enforce users to use the unstrusted forwarded for headers not caring about the connection IP if there needs now to be a change because 2.4.10 behaves different as a following release it becomes hard to handle signature.asc Description: OpenPGP digital signature
Re: mod_status: Apache 2.4 incorrect IP (proxy, not useragent_ip) on server-status page
Well, fixing this *specifically* for mod_status is easy, but, as you say, the problem is more systemic than that. On Sep 11, 2014, at 12:13 PM, wr...@rowe-clan.net wrote: - Original Message - Subject: Re: mod_status: Apache 2.4 incorrect IP (proxy, not useragent_ip) on server-status page From: Jim Jagielski j...@jagunet.com Date: 9/11/14 10:45 am To: dev@httpd.apache.org Ugg. Yeah; we should actually have a complimentary version that takes request_req as the param, not conn_rec. ap_get_remote_host_r()?? Considered that. But that still breaks mod_remoteip's advertised behavior against third party modules until they adapt. So I'd written this off as a non-starter :( Ugly. Indeed
Trunk errors for proxy
Anyone else seeing the test framework erroring out on mod_proxy* tests on trunk? t/modules/proxy.t ... 7/17 # Failed test 16 in t/modules/proxy.t at line 74 # Failed test 17 in t/modules/proxy.t at line 75 t/modules/proxy.t ... Failed 2/17 subtests t/modules/proxy_balancer.t .. 1/1 # Failed test 1 in t/modules/proxy_balancer.t at line 16 t/modules/proxy_balancer.t .. Failed 1/1 subtests # testing : reverse proxy of redirect via balancer # expected: 301 # received: '503' not ok 16 # testing : reverse proxy via balancer rewrote redirect # expected: 'http://localhost:8546/pr45434/5.html' # received: undef not ok 17 # Failed test 16 in t/modules/proxy.t at line 74 # Failed test 17 in t/modules/proxy.t at line 75
Re: Trunk errors for proxy
Nevermind... local build issue. On Sep 11, 2014, at 2:01 PM, Jim Jagielski j...@jagunet.com wrote: Anyone else seeing the test framework erroring out on mod_proxy* tests on trunk? t/modules/proxy.t ... 7/17 # Failed test 16 in t/modules/proxy.t at line 74 # Failed test 17 in t/modules/proxy.t at line 75 t/modules/proxy.t ... Failed 2/17 subtests t/modules/proxy_balancer.t .. 1/1 # Failed test 1 in t/modules/proxy_balancer.t at line 16 t/modules/proxy_balancer.t .. Failed 1/1 subtests # testing : reverse proxy of redirect via balancer # expected: 301 # received: '503' not ok 16 # testing : reverse proxy via balancer rewrote redirect # expected: 'http://localhost:8546/pr45434/5.html' # received: undef not ok 17 # Failed test 16 in t/modules/proxy.t at line 74 # Failed test 17 in t/modules/proxy.t at line 75
Re: [PATCH] SuexecUserGroup inside Directory context
+1
nsc
On 2014.09.11 22:16, Martynas Bendorius wrote:
I've created a patch for it, as I didn't have my question answered :)
From my point of view it's still secure, as it doesn't allow to set
SuexecUserGroup in .htaccess. I tested it and had no problems with it.
Please include it into the trunk if you think it's okay to add it.
=
--- httpd-2.4.10/modules/generators/mod_suexec.c.old2011-12-05
01:08:01.0 +0100
+++ httpd-2.4.10/modules/generators/mod_suexec.c2014-09-11
00:16:21.44409 +0200
@@ -59,7 +59,7 @@
const char *uid, const char *gid)
{
suexec_config_t *cfg = (suexec_config_t *) mconfig;
-const char *err = ap_check_cmd_context(cmd, NOT_IN_DIR_LOC_FILE);
+const char *err = ap_check_cmd_context(cmd,
NOT_IN_LOCATION|NOT_IN_FILES);
if (err != NULL) {
return err;
@@ -116,7 +116,7 @@
{
/* XXX - Another important reason not to allow this in .htaccess
is that
* the ap_[ug]name2id() is not thread-safe */
-AP_INIT_TAKE2(SuexecUserGroup, set_suexec_ugid, NULL, RSRC_CONF,
+AP_INIT_TAKE2(SuexecUserGroup, set_suexec_ugid, NULL,
RSRC_CONF|ACCESS_CONF,
User and group for spawned processes),
{ NULL }
};
=
Best regards,
Martynas Bendorius
Re: [Patch] Async write completion for the full connection filter stack
On 09/11/2014 05:20 PM, Graham Leggett wrote: On 11 Sep 2014, at 9:51 AM, Yann Ylavic ylavic@gmail.com wrote: If we or any filter has created the brigade on *deferred_write_pool (or an ancestor, I'm only talking about this case), it is dead now (unaccessible), and IMO should be marked as so. In the case the caller let the function handle the brigade (as we do), using NULL the first time only, (s)he expects to not crash whenever (s)he come back with it after a clear (which (s)he isn't supposed to know about). Agreed - as rpluem suggested a pool cleanup should be registered for this, I’ve added it. I know that I proposed this, but rethinking I see one risk with the cleanup. Take the following code with long_living_apr_bucket_brigade_ptr being NULL in the beginning: apr_bucket_brigade *stack_variable; stack_variable = long_living_apr_bucket_brigade_ptr; ap_save_brigade(f, stack_variable, bb, *deferred_write_pool); long_living_apr_bucket_brigade_ptr = stack_variable; I guess in this case this could cause unpredictable behaviour as the stack of the calling function is likely to be repurposed when the pool dies. Do we see that as something that we need to consider or do we see this as a bug on caller side? If we see it as a bug on caller side we should probably document our expectations. Regards Rüdiger
