Re: balancer-manager and XML

[Jim Riggs]

On Nov 30, 2011, at 9:16 AM, Jim Jagielski wrote:

 The XML interface for the balancer manager has, admittedly, lagged
 behind...
 
 Anyone have cycles and/or talent to bring it up to snuff? If not,
 I'll try to muddle thru it ;)

As I mentioned to Jim at ApacheCon, I have a 2.2 patch for this that I have 
been using in production for a couple of years so that we can monitor our 
balancers. I am in the process of updating this patch for 2.4 right now. I plan 
to submit the updated patch for discussion/input today. I am just doing some 
final testing and debugging.

Re: balancer-manager and XML

[Jim Riggs]

On Nov 30, 2011, at 10:26 AM, Jim Riggs wrote:

 On Nov 30, 2011, at 9:16 AM, Jim Jagielski wrote:
 
 The XML interface for the balancer manager has, admittedly, lagged
 behind...
 
 Anyone have cycles and/or talent to bring it up to snuff? If not,
 I'll try to muddle thru it ;)
 
 As I mentioned to Jim at ApacheCon, I have a 2.2 patch for this that I have 
 been using in production for a couple of years so that we can monitor our 
 balancers. I am in the process of updating this patch for 2.4 right now. I 
 plan to submit the updated patch for discussion/input today. I am just doing 
 some final testing and debugging.

https://issues.apache.org/bugzilla/show_bug.cgi?id=52264

mod_proxy_fcgi and EAGAIN (hacking)

[Jim Riggs]

Here is what I am testing: I am using (currently one) mod_proxy_fcgi member in 
a balancer to php-fpm. I have already run into some issues with fcgi:// as a 
balancer member as described in 
http://mail-archives.apache.org/mod_mbox/httpd-dev/201109.mbox/%3CB0DADBC2-5154-4C37-93B5-D38B834BE571%40riggs.me%3E.
 So, I have applied a small patch to httpd and php to get around these issues.

Everything has been working perfectly, but we noticed that some uploads fail 
with a 503. (We could upload a 181KB file but not 182KB.) I spent considerable 
time debugging and tracing the issue. I finally tracked this down to 
send_data() in mod_proxy_fcgi. While looping over the calls to 
apr_socket_sendv(), it would make 23 successful calls of 8200 bytes followed by 
a partial send and then receive EAGAIN. Because this is not APR_SUCCESS, it 
breaks the loop in the next line and returns a 503.

Since it received EAGAIN, I just brute-forced it to not break the loop on 
EAGAIN, but what is the correct fix? Is there something wrong in my setup? Is 
it a bug? Should send_data() be handling EAGAIN and continue the loop up to a 
timeout?

Your thoughts are appreciated.

- Jim


My brute-force hack:

--- mod_proxy_fcgi.c.orig   2012-02-03 13:23:09.132232659 -0600
+++ mod_proxy_fcgi.c2012-02-03 13:25:19.794906516 -0600
@@ -188,7 +188,7 @@
 while (to_write) {
 apr_size_t n = 0;
 rv = apr_socket_sendv(s, vec + offset, nvec - offset, n);
-if (rv != APR_SUCCESS) {
+if ((rv != APR_SUCCESS)  !APR_STATUS_IS_EAGAIN(rv)) {
 break;
 }
 if (n  0) {

Re: svn commit: r1242351 - in /httpd/httpd/trunk: docs/manual/mod/mod_proxy.xml modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h modules/proxy/mod_proxy_fcgi.c server/util_script.c

[Jim Riggs]

On Feb 9, 2012, at 10:22 AM, Ruediger Pluem wrote:

 j...@apache.org wrote:
 Author: jim
 Date: Thu Feb  9 15:07:22 2012
 New Revision: 1242351
 
 URL: http://svn.apache.org/viewvc?rev=1242351view=rev
 Log:
 Handle cases, esp when using mod_proxy_fcgi, when we do not
 want SCRIPT_FILENAME to include the query string.
 
 Modified:
httpd/httpd/trunk/docs/manual/mod/mod_proxy.xml
httpd/httpd/trunk/modules/proxy/mod_proxy.c
httpd/httpd/trunk/modules/proxy/mod_proxy.h
httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
httpd/httpd/trunk/server/util_script.c
 
 
 Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
 URL: 
 http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c?rev=1242351r1=1242350r2=1242351view=diff
 ==
 --- httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c (original)
 +++ httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c Thu Feb  9 15:07:22 2012
 @@ -188,7 +188,7 @@ static apr_status_t send_data(proxy_conn
 while (to_write) {
 apr_size_t n = 0;
 rv = apr_socket_sendv(s, vec + offset, nvec - offset, n);
 -if (rv != APR_SUCCESS) {
 +if ((rv != APR_SUCCESS)  !APR_STATUS_IS_EAGAIN(rv)) {
 break;
 }
 if (n  0) {
 
 
 How is this related to the log message and the query string issue?

It appears this commit has to do with the message I sent the other day 
(http://mail-archives.apache.org/mod_mbox/httpd-dev/201202.mbox/%3c8ea4e5e4-f97e-410b-aad4-257ecb4f9...@riggs.me%3E).

The question is, was that EAGAIN fix supposed to be included in this commit 
or not? Is that even the right fix? That was my brute-force attempt to 
address the issue I was having, but I don't know if it is correct. Do we need 
to check in the loop for a timeout or something in case we keep receiving 
EAGAIN? Or would the timeout get picked up and bail out elsewhere?

Re: [PATCH] mod_log_forensic security considerations

[Jim Riggs]

On Jun 7, 2012, at 3:11 PM, Stefan Fritsch wrote:

 I share Williams concern that this makes mod_forensic potentially less 
 useful.
 
 Maybe making the forensic log mode 600 by default would be a better 
 idea?


I have to agree with Jeff. I would rather have a more difficult or even 
impossible time debugging a crash than have a security hole that relies solely 
on file permissions.

Maybe it should be a toggle in mod_forensic for debugging purposes (defaulting 
to hiding Authorization). The problem with just changing the file permissions 
is that sensitive data is still stored in the files. Even if the files are 
owned by root, anyone with root access would have access to others' usernames 
and passwords. I don't want to have that access to others' credentials, nor do 
I want them to have access to mine.

I applied Jeff's patch as soon as it came across, wiped out all of our archived 
forensic logs, and had all of our affected users reset their passwords. Thanks, 
Jeff!

- Jim

Re: [PATCH] mod_log_forensic security considerations

[Jim Riggs]

On Jun 8, 2012, at 11:51 AM, Graham Leggett wrote:

 On 08 Jun 2012, at 5:45 PM, Joe Schaefer wrote:
 
 Well not quite, we'd still have had a problem with storing and archiving
 those logs even if we hadn't made them available to committers, because
 they violate our password retention policies.
 
 Can you clarify if possible what purpose you were trying to solve by enabling 
 the forensic logs?
 
 Forensic logging is to answer the question what is going wrong, and 
 shouldn't be enabled under normal operational circumstances unless there is 
 something genuinely going wrong, at which point you record what you need and 
 then switch it off again.
 
 A forensic log that has had a whole lot of filters applied to it is 
 counterproductive, because the forensic log no longer tells you exactly what 
 is going on, and when you're troubleshooting you need to know precisely that.

In my situation, we have them enabled so that when an issue arises, we have one 
more tool at our disposal to identify a root cause. When I get an alert that 
there is something wrong with one of our sites, it is usually too late to 
enable forensic logging at that point. Something has already happened. We need 
to mitigate and get everything back up to normal. The question is usually not 
what IS going wrong?, but rather what WENT wrong?, because it is often a 
short-lived event.

Having the forensic logs available has proven extremely helpful in this 
scenario. Might the full, unfiltered forensic data be valuable? Yes, but I 
don't believe the security risk is worth it in my situation. The rare case 
where an Authorization header might be truly useful for debugging or RCA is 
vastly overshadowed by the usefulness of the rest of the request information 
stored in the forensic log.

The key to the forensic log, obviously, is that we have some information about 
an incoming request before it is completed. We can't get this information from 
any of the standard or custom logs, and we don't have any control over the 
format. Perhaps, just like we have LogFormat and now ErrorLogFormat, we should 
have ForensicLogFormat? If we did, then everyone could have what they 
want/need, whether full or partial forensic data.

- Jim

New RewriteMap Help/Suggestions

[Jim Riggs]

I am in the process of preparing a patch to add a new RewriteMap type and could 
use some input from all of you on the best implementation. What I am creating 
is basically a clone of the txt map type, except that each line is a regexp 
followed by a replacement (with potential back-references).

You can just do those as RewriteRules...no need for a map, you say. True, 
except that I am looking at an automated, application-created list, and I don't 
really want the application to be writing out configuration files or .htaccess 
files. I would be much more comfortable with the app writing out a map file 
that has limited functionality and scope. Plus, it would be easy for the app to 
just create 'regexp replacement' lines at build time.

So, I have created a crude, working proof-of-concept of this. It basically 
copies all of the functionality of the txt maps, including the cache, but in 
the lookup_map_regexpfile() function, it compiles the regexp for each line, 
attempts a match, and returns the backref-substituted replacement. (This pair 
gets cached.) This works beautifully as is, but it is horribly inefficient to 
have to compile the REs every time we come in with a new key/URL. So, I was 
thinking of precompiling all of them and see three options:

1. Precompile and store all of the REs at config load time.
2. Compile and store all of the REs the first time we hit 
lookup_map_regexpfile() or when the map file is updated.
3. Compile and store each RE as we read through the map file in 
lookup_map_regexpfile() until a match is found and bail (full list will be 
built over time).

#1 is nice, because all of the work is done up front and will be fast from then 
on. The problem, though, is that I would like this map to reload/refresh if the 
map file gets changed like the other types do. #2 and #3 solve this. With #2 I 
worry about performance of compiling everything if the map file gets updated 
and we get a thundering herd. With #3 there is some coordination to manage with 
respect to which lines have been compiled and which ones haven't.

Does anyone have thoughts as to:

1. When/how should the map REs be compiled/precompiled? One of the options 
above or something else?
2. Where should the compiled REs be stored: in an existing pool or a new one?

Thanks for any input.

- Jim

Re: FreeBSD make

[Jim Riggs]

On 20 Dec 2013, at 07:04, Jim Jagielski j...@jagunet.com wrote:

 OK... this is weird, FreeBSD 9.2 make doesn't like
 
   http://svn.apache.org/viewvc?view=revisionrevision=1327907
 
 gmake works fine. :/
 
 (can't grab what the exact error is right now, but something
 like needs an operator or something like that... it doesn't
 like the 'ifdef' statement... or the 'else')

BSD's make needs dot-prefixed .ifdef, .else, and .endif. I don't believe there 
is a good cross-make compatible way to do this. See 
http://stackoverflow.com/questions/9096018/make-gmake-compatible-if-else-statment.

mod_proxy_ajp: ignoring flush before headers (again)

[Jim Riggs]

For some (old 2007) context, see:

http://markmail.org/message/btwcnbl2i7ftwj4n

https://community.jivesoftware.com/message/201787


I am proxying an app via AJP to Tomcat 6/7.  In certain circumstances, it 
appears that the app (or possibly Tomcat) is erroneously sending a flush before 
the headers have been sent.  In r57, Jim added an exception to handle this 
situation with the intention of ignoring the flush.  I'm not sure it's working 
quite right, though, as the brigade is still getting passed through the filter 
chain.  So, ap_headers_output_filter() is getting called too soon, I think.  (I 
am no expert in the httpd code, so I'm not sure this is really the problem.)

Can any of you who ARE experts in the code tell me what you think of the issue 
and how we can fix it?  I'm thinking that if we are ignoring a flush at 
mod_proxy_ajp.c:448 (in 2.2.x), we should not be calling ap_pass_brigade() at 
line 472, but I don't know if there are any ramifications of that.

The symptom is that when this issue happens, the user gets prompted to save a 
file (Content-Type returned by httpd is 'text/plain' even though Tomcat is 
returning 'text/html;charset=utf-8').  Below is some debug output showing 
correct and incorrect behavior:

Correct:

[Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(266): proxy: 
APR_BUCKET_IS_EOS
[Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(271): proxy: data to read 
(max 8186 at 4)
[Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(286): proxy: got 0 bytes of 
data
[Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(687): ajp_read_header: 
ajp_ilink_received 04
[Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(697): ajp_parse_type: got 04
[Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(516): ajp_unmarshal_response: 
status = 200
[Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(537): ajp_unmarshal_response: 
Number of headers is = 5
[Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: 
Header[0] [Pragma] = [No-cache]
[Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: 
Header[1] [Cache-Control] = [no-cache]
[Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: 
Header[2] [Expires] = [Wed, 31 Dec 1969 18:00:00 CST]
[Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: 
Header[4] [Content-Type] = [text/html;charset=utf-8]
[Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(609): ajp_unmarshal_response: 
ap_set_content_type done
[Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(687): ajp_read_header: 
ajp_ilink_received 03
[Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(697): ajp_parse_type: got 03
[Tue Aug 02 09:34:50 2011] [debug] mod_headers.c(756): headers: 
ap_headers_output_filter()


Incorrect (notice how ap_headers_output_filter() is called before the headers 
are received):

[Tue Aug 02 09:32:18 2011] [debug] mod_proxy_ajp.c(266): proxy: 
APR_BUCKET_IS_EOS
[Tue Aug 02 09:32:18 2011] [debug] mod_proxy_ajp.c(271): proxy: data to read 
(max 8186 at 4)
[Tue Aug 02 09:32:18 2011] [debug] mod_proxy_ajp.c(286): proxy: got 0 bytes of 
data
[Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(687): ajp_read_header: 
ajp_ilink_received 03
[Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(697): ajp_parse_type: got 03
[Tue Aug 02 09:32:18 2011] [debug] mod_proxy_ajp.c(452): Ignoring flush message 
received before headers
[Tue Aug 02 09:32:18 2011] [debug] mod_headers.c(756): headers: 
ap_headers_output_filter()
[Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(687): ajp_read_header: 
ajp_ilink_received 03
[Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(697): ajp_parse_type: got 03
[Tue Aug 02 09:32:18 2011] [debug] mod_proxy_ajp.c(452): Ignoring flush message 
received before headers
[Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(687): ajp_read_header: 
ajp_ilink_received 04
[Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(697): ajp_parse_type: got 04
[Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(516): ajp_unmarshal_response: 
status = 200
[Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(537): ajp_unmarshal_response: 
Number of headers is = 5
[Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: 
Header[0] [Pragma] = [No-cache]
[Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: 
Header[1] [Cache-Control] = [no-cache]
[Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: 
Header[2] [Expires] = [Wed, 31 Dec 1969 18:00:00 CST]
[Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: 
Header[3] [Set-Cookie] = [JSESSIONID=39968855F543CA08A440E6136EA6FC28.app1; 
Path=/tomcat-manager; Secure; HttpOnly]
[Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: 
Header[4] [Content-Type] = [text/html;charset=utf-8]
[Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(609): ajp_unmarshal_response: 
ap_set_content_type done

Re: mod_proxy_ajp: ignoring flush before headers (again)

[Jim Riggs]

This does appear to work.  Our automated tests are running right now.  Shall I 
submit a but with this patch attached?


On Aug 3, 2011, at 7:43 AM, Plüm, Rüdiger, VF-Group wrote:

 Can you please try if the following patch fixes your issue?
 
 Index: mod_proxy_ajp.c
 ===
 --- mod_proxy_ajp.c (revision 1150558)
 +++ mod_proxy_ajp.c (working copy)
 @@ -506,16 +506,18 @@
 if (bb_len != -1)
 conn-worker-s-read += bb_len;
 }
 -if (ap_pass_brigade(r-output_filters,
 -output_brigade) != APR_SUCCESS) {
 -ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
 -  proxy: error processing body.%s,
 -  r-connection-aborted ?
 -   Client aborted connection. : 
 );
 -output_failed = 1;
 +if (headers_sent) {
 +if (ap_pass_brigade(r-output_filters,
 +output_brigade) != 
 APR_SUCCESS) {
 +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
 +  proxy: error processing 
 body.%s,
 +  r-connection-aborted ?
 +   Client aborted connection. 
 : );
 +output_failed = 1;
 +}
 +data_sent = 1;
 +apr_brigade_cleanup(output_brigade);
 }
 -data_sent = 1;
 -apr_brigade_cleanup(output_brigade);
 }
 }
 else {
 
 Currently the code sends an empty brigade in your case which also triggers 
 the sending of headers by httpd.
 
 Regards
 
 Rüdiger
 
 -Original Message-
 From: Jim Riggs 
 Sent: Dienstag, 2. August 2011 18:03
 To: dev@httpd.apache.org
 Subject: mod_proxy_ajp: ignoring flush before headers (again)
 
 For some (old 2007) context, see:
 
 http://markmail.org/message/btwcnbl2i7ftwj4n
 
 https://community.jivesoftware.com/message/201787
 
 
 I am proxying an app via AJP to Tomcat 6/7.  In certain 
 circumstances, it appears that the app (or possibly Tomcat) 
 is erroneously sending a flush before the headers have been 
 sent.  In r57, Jim added an exception to handle this 
 situation with the intention of ignoring the flush.  I'm not 
 sure it's working quite right, though, as the brigade is 
 still getting passed through the filter chain.  So, 
 ap_headers_output_filter() is getting called too soon, I 
 think.  (I am no expert in the httpd code, so I'm not sure 
 this is really the problem.)
 
 Can any of you who ARE experts in the code tell me what you 
 think of the issue and how we can fix it?  I'm thinking that 
 if we are ignoring a flush at mod_proxy_ajp.c:448 (in 2.2.x), 
 we should not be calling ap_pass_brigade() at line 472, but I 
 don't know if there are any ramifications of that.
 
 The symptom is that when this issue happens, the user gets 
 prompted to save a file (Content-Type returned by httpd is 
 'text/plain' even though Tomcat is returning 
 'text/html;charset=utf-8').  Below is some debug output 
 showing correct and incorrect behavior:
 
 Correct:
 
 [Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(266): 
 proxy: APR_BUCKET_IS_EOS
 [Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(271): 
 proxy: data to read (max 8186 at 4)
 [Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(286): 
 proxy: got 0 bytes of data
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(687): 
 ajp_read_header: ajp_ilink_received 04
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(697): 
 ajp_parse_type: got 04
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(516): 
 ajp_unmarshal_response: status = 200
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(537): 
 ajp_unmarshal_response: Number of headers is = 5
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): 
 ajp_unmarshal_response: Header[0] [Pragma] = [No-cache]
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): 
 ajp_unmarshal_response: Header[1] [Cache-Control] = [no-cache]
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): 
 ajp_unmarshal_response: Header[2] [Expires] = [Wed, 31 Dec 
 1969 18:00:00 CST]
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): 
 ajp_unmarshal_response: Header[4] [Content-Type] = 
 [text/html;charset=utf-8]
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(609): 
 ajp_unmarshal_response: ap_set_content_type done
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(687): 
 ajp_read_header: ajp_ilink_received 03
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(697

Re: mod_proxy_ajp: ignoring flush before headers (again)

[Jim Riggs]

https://issues.apache.org/bugzilla/show_bug.cgi?id=51608

Thank you for your help and quick fix, Rüdiger!!


On Aug 3, 2011, at 10:09 AM, Plüm, Rüdiger, VF-Group wrote:

 Thanks for testing. Committed to trunk as r1153531.
 A bug report mentioning the trunk revision could be handy as a backport 
 tracker for 2.2.x.
 
 Regards
 
 Rüdiger
 
 -Original Message-
 From: Jim Riggs 
 Sent: Mittwoch, 3. August 2011 16:48
 To: dev@httpd.apache.org
 Subject: Re: mod_proxy_ajp: ignoring flush before headers (again)
 
 This does appear to work.  Our automated tests are running 
 right now.  Shall I submit a but with this patch attached?
 
 
 On Aug 3, 2011, at 7:43 AM, Plüm, Rüdiger, VF-Group wrote:
 
 Can you please try if the following patch fixes your issue?
 
 Index: mod_proxy_ajp.c
 ===
 --- mod_proxy_ajp.c (revision 1150558)
 +++ mod_proxy_ajp.c (working copy)
 @@ -506,16 +506,18 @@
if (bb_len != -1)
conn-worker-s-read += bb_len;
}
 -if (ap_pass_brigade(r-output_filters,
 -
 output_brigade) != APR_SUCCESS) {
 -ap_log_rerror(APLOG_MARK, 
 APLOG_DEBUG, 0, r,
 -  proxy: error 
 processing body.%s,
 -  r-connection-aborted ?
 -   Client aborted 
 connection. : );
 -output_failed = 1;
 +if (headers_sent) {
 +if (ap_pass_brigade(r-output_filters,
 +
 output_brigade) != APR_SUCCESS) {
 +ap_log_rerror(APLOG_MARK, 
 APLOG_DEBUG, 0, r,
 +  proxy: 
 error processing body.%s,
 +  
 r-connection-aborted ?
 +   Client 
 aborted connection. : );
 +output_failed = 1;
 +}
 +data_sent = 1;
 +apr_brigade_cleanup(output_brigade);
}
 -data_sent = 1;
 -apr_brigade_cleanup(output_brigade);
}
}
else {
 
 Currently the code sends an empty brigade in your case 
 which also triggers the sending of headers by httpd.
 
 Regards
 
 Rüdiger
 
 -Original Message-
 From: Jim Riggs 
 Sent: Dienstag, 2. August 2011 18:03
 To: dev@httpd.apache.org
 Subject: mod_proxy_ajp: ignoring flush before headers (again)
 
 For some (old 2007) context, see:
 
 http://markmail.org/message/btwcnbl2i7ftwj4n
 
 https://community.jivesoftware.com/message/201787
 
 
 I am proxying an app via AJP to Tomcat 6/7.  In certain 
 circumstances, it appears that the app (or possibly Tomcat) 
 is erroneously sending a flush before the headers have been 
 sent.  In r57, Jim added an exception to handle this 
 situation with the intention of ignoring the flush.  I'm not 
 sure it's working quite right, though, as the brigade is 
 still getting passed through the filter chain.  So, 
 ap_headers_output_filter() is getting called too soon, I 
 think.  (I am no expert in the httpd code, so I'm not sure 
 this is really the problem.)
 
 Can any of you who ARE experts in the code tell me what you 
 think of the issue and how we can fix it?  I'm thinking that 
 if we are ignoring a flush at mod_proxy_ajp.c:448 (in 2.2.x), 
 we should not be calling ap_pass_brigade() at line 472, but I 
 don't know if there are any ramifications of that.
 
 The symptom is that when this issue happens, the user gets 
 prompted to save a file (Content-Type returned by httpd is 
 'text/plain' even though Tomcat is returning 
 'text/html;charset=utf-8').  Below is some debug output 
 showing correct and incorrect behavior:
 
 Correct:
 
 [Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(266): 
 proxy: APR_BUCKET_IS_EOS
 [Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(271): 
 proxy: data to read (max 8186 at 4)
 [Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(286): 
 proxy: got 0 bytes of data
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(687): 
 ajp_read_header: ajp_ilink_received 04
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(697): 
 ajp_parse_type: got 04
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(516): 
 ajp_unmarshal_response: status = 200
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(537): 
 ajp_unmarshal_response: Number of headers is = 5
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): 
 ajp_unmarshal_response: Header[0] [Pragma] = [No-cache]
 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): 
 ajp_unmarshal_response: Header[1] [Cache-Control] = [no-cache

mod_proxy_fcgi + mod_proxy_balancer vs. php-fpm and query strings

[Jim Riggs]

I am having a couple of problems WRT using mod_proxy_fcgi inside a balancer 
proxied to php-fpm. There are lots of variables in this scenario, but I think I 
have narrowed the issues down.  The setup looks like this:

httpd - balancer - fcgi balancer members - php-fpm

Issue 1: PHP-FPM does not handle the proxy:balancer prefix in 
SCRIPT_FILENAME. It does handle proxy:fcgi as a special case (see 
https://bugs.php.net/bug.php?id=54152 fix by jim). So, it seems we need to also 
add a proxy:balancer exception there unless a balanced mod_proxy_fcgi member 
should actually be using proxy:fcgi instead. What are people's thoughts on 
the prefix that should be sent by httpd in this case? To address this for now, 
I have modified PHP (fpm_main.c alongside jim's existing changes).

Issue 2: Once I got Issue 1 addressed, everything started working except in the 
case of a query string. I spent considerable time tracing and trying to figure 
out where the issue is occurring, but I am hoping one of you who is much more 
familiar with the code than I will be able to say, Oh, look right here. The 
problem is that the query string is getting appended to SCRIPT_FILENAME if 
proxied through a balancer. FPM does not like this. It does not seem to happen 
in the case of proxying directly to fcgi://..., but once I change this to 
balancer://..., the query string gets added to SCRIPT_FILENAME. I believe 
this happened with both ProxyPass* and mod_rewrite [P]. In mod_rewrite, this 
should get handled in splitout_queryargs(), but somehow it is getting added 
back (probably in proxy_balancer_canon() which adds the query string back to 
r-filename?). For right now, I have done a brute-force fix for this by 
adding the code below to the beginning of send_environment() in 
mod_proxy_fcgi.c, before the calls to ap_add_common_vars() and 
ap_add_cgi_vars(). I am guessing that this isn't the ultimate fix for this 
issue, so I am interested in others' thoughts.

+/* Remove query string from r-filename (r-args is already set and passed 
via QUERY_STRING) */
+q = ap_strchr_c(r-filename, '?');
+if (q != NULL) {
+*q = '\0';
+}

Re: RFE: Control of HTTP cache control headers within mod_rewrite rules

[Jim Riggs]

On Oct 27, 2011, at 3:35 PM, Stefan Fritsch wrote:

 On Tuesday 25 October 2011, Noah Robin wrote:
 I ran some tests on this and the following modified version will
 work:
 
 Header always set Cache-Control max-age=%{CACHE_LIFETIME}e
 env=CACHE_LIFETIME
 RewriteRule /example http://www.example.com/
 [E=CACHE_LIFETIME:604800]
 
 ..however, this still leaves an open question in my mind: How to
 solve for the more general case where I want Apache to set the
 cache control header on any 301 it sends, even if the 301 was
 generated within the application rather than in Apache's
 configuration. I don't see a way to set an environment variable
 based on a response attribute (e.g. r-status). Am I missing
 something or would something need to be written to handle this
 case?
 
 2.3/2.4 supports this:
 
 Header set Cache-Control max-age=604800 %{REQUEST_STATUS} == 301
 
 or even %{REQUEST_STATUS} -in { 301 , 302 }. This is a good 
 candidate for an example in the docs.
 
 I can't think of a way to do this in 2.2.x, though.

In 2.2 you can do this with mod_setenvifplus 
http://modsetenvifplus.sourceforge.net/. I do things like this all of the 
time.

mod_cache thundering herd bug

[Jim Riggs]

https://issues.apache.org/bugzilla/show_bug.cgi?id=50317

While we are at ApacheCon, I would love to address this nasty bug with someone 
familiar with 2.2's mod_cache. Our sites were brought down a few times last 
year before we finally tracked it down to being this particular bug. I am using 
a crude backport of the 2.3 patch (r1023398) in 2.2. It works, but I don't know 
if it is correct.

Can someone look at this one with me? We really need to get this fixed in 2.2, 
because there is NO thundering herd protection at all as things stand right now.

- Jim

Re: mod_cache thundering herd bug

[Jim Riggs]

On 9 Apr 2014, at 14:46, Eric Covener cove...@gmail.com wrote:

 r1023398 for 2.2:
 
  http://people.apache.org/~covener/patches/httpd-2.2.x-thunder.diff
 
 The remove_url() prevents other threads from serving a stale cached
 file during refresh of a slow response, but it's unnecessary to have a
 separate path because the refresh has to deal with 200s already.  When
 the remove_url was added, there as no thundering herd lock / no
 ability to serve stale content while one guy was reloading.


covener, mrumph, and I looked at this today at ApacheCon. I updated the bug 
with some comments and attached this patch.

https://issues.apache.org/bugzilla/show_bug.cgi?id=50317

Re: Configuration error handling after httpd restart

[Jim Riggs]

On 27 Mar 2014, at 14:16, Mike Rumph mike.ru...@oracle.com wrote:

 Hello all,
 
 I have been doing some testing on the results of httpd restart with 
 configuration errors.
 This gave me some interesting results.
 
 For these tests I build httpd trunk with APR trunk on Linux using the 
 following configure:
 $ ./configure --prefix=/home/mrumph/apache25 --with-included-apr 
 --with-mpm=worker --enable-mpms-shared='worker'
 
 Modify the default httpd.conf to include the following line:
 Listen localhost:8080
 
 Start the httpd server and verify the process information.
 $ bin/apachectl -k start
 $ ps -ef | grep -i httpd
 
 Now restart httpd from this starting point with the following configuration 
 error cases:
 
 Case 1:  An unknown directive.
 Add the following line to the httpd.conf file.
 Xyzzy
 
 bin/apachectl -k restart
   - Returns with exit code 1 and following error message in stderr:
 AH00526: Syntax error on line 198 of /home/mrumph/apache25/conf/httpd.conf:
 Invalid command 'Xyzzy', perhaps misspelled or defined by a module not 
 included in the server configuration
 httpd: abnormal exit 1
 
 $ ps -ef | grep -i httpd
   - The httpd server was not stopped and all of the previous processes remain.
   - And the logs/httpd.pid file remains intact.
 
 $ tail logs/error_log
   - No error message logged.
 
 Case 2:  A duplicate Listen directive.
 Duplicate the Listen directive in the httpd.conf file.
 Listen localhost:8080
 Listen localhost:8080
 
 $ bin/apachectl -k restart
   - Returns with exit code 0 and no error message in stderr.
   - So the httpd server appears to be working at this point.
   - (But appearances are deceiving.)
 
 $ ps -ef | grep -i httpd
   - All of the httpd processes have stopped.
   - But the logs/httpd.pid file remains intact.
 
 $ tail logs/error_log
 [Thu Mar 27 11:26:22.836887 2014] [mpm_worker:notice] [pid 2677:tid 
 47577479346656] AH00298: SIGHUP received.  Attempting to restart
 (98)Address already in use: AH00072: make_sock: could not bind to address 
 127.0.0.1:8080
 [Thu Mar 27 11:26:22.844003 2014] [mpm_worker:alert] [pid 2677:tid 
 47577479346656] no listening sockets available, shutting down
 [Thu Mar 27 11:26:22.844031 2014] [core:emerg] [pid 2677:tid 47577479346656] 
 AH00019: Unable to open logs, exiting
 
 
 This was httpd trunk, but similar results are seen with httpd 2.2.22, 
 2.2-HEAD and 2.4.6.
 
 
 Before working on this as a bug, I am trying to understand what should be the 
 correct behavior.
 I think case 1 is working correctly.
 But case 2 doesn't seem quite right.
 First of all, it doesn't seem correct to have an httpd.pid file when all of 
 the httpd processes have vanished.
 Secondly, it would be nice to see an error code from the apachectl -k restart.
 (But this is probably due to a different processing phase in the validation 
 for both of the cases.)
 It is also a little strange to see a message Unable to open logs in the log.
 
 Does anyone have some opinions what the correct behavior should be for these 
 cases.
 
 If there are some actual bugs here, I have some time available to work on 
 them.



Check out the attached patch to ignore duplicate Listen directives.



duplicate_listen.patch
Description: Binary data

Re: Configuration error handling after httpd restart

[Jim Riggs]

On 14 Apr 2014, at 10:38, Eric Covener cove...@gmail.com wrote:

 On Mon, Apr 14, 2014 at 11:15 AM, Mike Rumph mike.ru...@oracle.com wrote:
 If there is an unknown directive in the config file, simply ignore it with a
 warning.
 
 You can't do that.  What if it was Reqiure?

I agree with Eric. I would not want unknown directives to be ignored. It might 
be a typo of a really important directive like Eric describes. Or, what if a 
module I really, really need is accidentally disabled and we just ignore all of 
its directives? Not good.

In this particular case, duplicating a Listen directive doesn't seem like it 
should bomb out the server.

Listen 80
...
Listen 80

It's superfluous, but not really a critical error. So, my patch just ignores 
subsequent duplicate Listens.

Re: mod_cache thundering herd bug

[Jim Riggs]

On 21 Apr 2014, at 06:38, Graham Leggett minf...@sharp.fm wrote:

 On 19 Apr 2014, at 10:26 PM, Eric Covener cove...@gmail.com wrote:
 
 Graham -- related subject brought up either in Denver or in the bug.
 It seems that when we serve a stale file while the cache is locked,
 the age headers are small instead of large. I got totally lost trying
 to track down the issue, maybe it makes sense to you?  It's almost as
 if they time of the revalidation is somehow updated early and the
 delta in the stale cache hits is based off of that.
 
 All thundering herd does is after letting the first conditional request 
 through, it serves stale data (RFC willing) until that conditional request 
 comes back or a specific maximum time is reached, whichever comes first.
 
 The most valuable piece of information in this process is the reason 
 variable, which describes the reason why something wasn't eligible for 
 caching. In httpd v2.4 the X-Cache-Detail header will give this to you, in 
 httpd v2.2 you'll need to log at DEBUG level to get this:
 
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
cache: %s not cached. Reason: %s, r-unparsed_uri,
reason);
 
 The questions to answer are:
 
 - Is there stale content to serve? No stale content, no thundering herd 
 protection.
 - If stale content is being deleted, identify why that is. This is likely to 
 be unrelated to thundering herd, but rather in other parts of mod_cache.



Covener - Are you talking about my comments in #16 on the ticket? 
(https://issues.apache.org/bugzilla/show_bug.cgi?id=50317#c16)

If so, do either you or Graham have thoughts on the Age header getting returned 
with stale content? In my testing, when stale content is getting returned, no 
Age header is set which appears to be a violation of HTTP 1.1.

[patch] regexp rewrite map

[Jim Riggs]

[Posting separately to both dev and users to see if anyone on either side sees 
value in getting this committed.]

About a year ago, I had an idea for a new type of RewriteMap that would fill an 
important need for a few particular use cases that we have [1]. While we were 
at ApacheCon in Denver, I spent some time talking with JimJag, Rich, and 
Covener as well as updating the code from a crude proof of concept to something 
real. I would appreciate feedback from anyone, especially on whether or not 
this is something worth pursuing getting committed.

It is a simple concept: the map is just a list of regexp patterns and 
replacements. These could be done as individual RewriteRules, obviously, but 
this rewrite map would reduce clutter in the config file, be more readable, and 
could even be externally generated/maintained without any httpd admin 
involvement. For example, an application or batch job could generate a map file 
with dozens or hundreds of entries that httpd would pick up without a 
restart/graceful, and the config might only contain a single RewriteRule:


[map file]
/foo(bar)? /baz$1
/(apple|banana|orange) /fruit/$1
/post/(\d+)(/.*)?  /article/$1$2
...


[config file]
RewriteMap regexptest regexp:path/to/re.map

RewriteCond ${regexptest:$1} ^(.+)$
RewriteRule ^(.*)$ %1 [R]
...


Possible use cases that I can think of:

1. Redirect list (e.g. legacy site to new site) without pages of 
RewriteRules/Redirects
2. Simplify 100s or 1000s of rewrite rules into 1 + the map as above
3. White list of URL patterns to proxy through to backend servers (can be 
application generated; my particular use case)
4. Maps could be application generated, maintained in a spreadsheet or DB, or 
created with scripts/greps/etc.

Just like text and hash maps, results are cached. I did some tests with up to 
100K entries in the map, and it was still extremely responsive and worked 
flawlessly. The only thing this doesn't have is flags (e.g. NC), but that can 
be handled in the pattern itself via (?i).

If interested, I would love it if some folks would try the attached patch and 
let me know what you think.

- Jim

[1] http://httpd.markmail.org/thread/3dheejtgwmdpxxt5



regexp_map.patch
Description: Binary data

Re: Time for httpd 2.2.28??

[Jim Riggs]

 If so, I can RM.

PLEASE! We need those thundering herd fixes in mod_cache! :-)

Re: Time for httpd 2.2.28??

[Jim Riggs]

On 16 Jul 2014, at 08:19, Yann Ylavic ylavic@gmail.com wrote:

 On Wed, Jul 16, 2014 at 3:15 PM, Jim Riggs apache-li...@riggs.me wrote:
 If so, I can RM.
 
 PLEASE! We need those thundering herd fixes in mod_cache! :-)
 
 
 This has already been backported by http://svn.apache.org/r1608302.

I know. I mean we need the fixes to be officially released. I've been patching 
mod_cache for over 2 years now, I believe.

Re: ApacheCon Austin, httpd track

[Jim Riggs]

On 11/30/2014 11:08 AM, Jeff Trawick wrote:
 * deploying Python web apps under uWSGI behind mod_proxy_fcgi/scgi
 (some material
 here: http://emptyhammock.com/projects/info/pyweb/index.html)


On 1 Dec 2014, at 19:15, Daniel Ruggeri drugg...@primary.net wrote:
 Similarly, I'm always up for giving my proxy talk if it's welcome (after
 the first day since I can't make it until Tues). If we think proxy is a
 big topic, we ought to arrange for a general overview like my proxy talk
 followed by more specific deep dives such as what Jeff mentions here and
 a session on new sexiness like WebSockets.


Picking up on what Jeff and Daniel are saying, I think some focus on the 
powerful things mod_proxy_* can do would be really useful.

One particular thought that has been in the back of my head for some time is a 
Begone libphp5.so! talk. For better or worse, PHP is still around and will be 
for some time, but it is time to get it out of the web server and treat it like 
the application/backend it is for both security and resource-usage reasons. 
mod_proxy_fcgi + php-fpm is a really elegant, simple solution to make this 
happen, but I have found a lot of devs and admins who just aren't even aware of 
this configuration possibility. (I have explained it to several people at 
ApacheCon NAs over the past couple of years.) I've actually been using a 
backported mod_proxy_fcgi in 2.2 for just this purpose for a few years in 
production.

That's certainly a talk I would be willing to give if there is interest.

P.S. mod_proxy_balancer - mod_proxy_fcgi - php-fpm is really fun and 
interesting too! ;-)

Re: mod_proxy_fcgi issues

[Jim Riggs]

 On Thu, Dec 4, 2014 at 9:58 AM, Eric Covener cove...@gmail.com wrote:
 
 On Tue, Dec 2, 2014 at 4:14 PM, Jim Riggs apache-li...@riggs.me wrote:
 P.S. mod_proxy_balancer - mod_proxy_fcgi - php-fpm is really fun and
 interesting too! ;-)
 
 mod_proxy_fcgi seems to need a bit of work from what I have been
 seeing in bugzilla and IRC.  I hope to spend a little time on the code
 and doc, but not being an actual user of it I don't know how far I
 will really get before being distracted.

This all may certainly be true, but I just for clarity's sake (since it was my 
quote that started this new mod_proxy_fcgi thread), my mod_proxy_balancer - 
mod_proxy_fcgi - php-fpm issue is NOT an httpd issue...at least that is not 
how I have treated it. It is actually a code fix I have had to make in PHP to 
get it to work.

See:

https://bugs.php.net/bug.php?id=54152
https://bugs.php.net/bug.php?id=62172

It would be great if JimJag could maybe address this if he still has PHP 
access. Jim?

Re: Proposal/RFC: informed load balancing

[Jim Riggs]

On Wednesday, April 29, 2015, Jim Riggs apache-li...@riggs.me 
mailto:apache-li...@riggs.me wrote:
Warn out from writing all of this and hopeful that someone other than me 
actually cares, I wish you all well today/tonight!

 *Worn* out, even! Boy, I was tired!

Re: *Match, RewriteRule POLA violation?

[Jim Riggs]

 On 28 Apr 2015, at 17:55, Yann Ylavic ylavic@gmail.com wrote:
 
 It seems that while Location is compared to ap_no2slash(r-uri),
 LocationMatch is matched against r-uri directly.
 That's probably the issue.
 
 A possible fix (untested) could be:
 
 Index: server/request.c
 ===
 --- server/request.c(revision 1674695)
 +++ server/request.c(working copy)
 @@ -1446,7 +1446,7 @@
 pmatch = apr_palloc(rxpool, nmatch*sizeof(ap_regmatch_t));
 }
 
 -if (ap_regexec(entry_core-r, r-uri, nmatch, pmatch, 0)) {
 +if (ap_regexec(entry_core-r, entry_uri, nmatch, pmatch, 0)) 
 {
 continue;
 }
 
 @@ -1456,7 +1456,7 @@
 apr_table_setn(r-subprocess_env,
((const char
 **)entry_core-refs-elts)[i],
apr_pstrndup(r-pool,
 -   r-uri + pmatch[i].rm_so,
 +   entry_uri + pmatch[i].rm_so,
pmatch[i].rm_eo - pmatch[i].rm_so));
 }
 }

Thanks, Yann. I remember looking at this code before. The question remains, 
though: Is it currently wrong? Does it need to be fixed, or was this 
distinction made intentionally? Is there a specific use case that requires the 
regex-matching directives to not get slash-normalized URIs?


 On Mon, Apr 27, 2015 at 10:52 PM, Jim Riggs apache-li...@riggs.me wrote:
 This came up at ApacheCon a couple of weeks ago. I just took this knowledge 
 for granted, as I have always accounted for it, but both Rich and Trawick 
 were surprised. As I thought about it some more, it seems this may be a POLA 
 violation. Thoughts? If we agree it should be fixed, I can make the bugz and 
 make a patch.
 
 Consider:
 
 Location /slash/foo
 ...
 /Location
 
 vs.
 
 LocationMatch ^/slash/foo
 ...
 /LocationMatch
 
 
 These do not behave the same if multiple slashes are used. The leading 
 slashes are always coalesced, so ^/... is fine; however, any intermediate 
 slashes are not. So, in order for the LocationMatch directive above to 
 behave the same as the Location, it has to be specified as ^/slash/+foo. 
 Like I said, I have always accounted for this in my regexps, but it doesn't 
 seem right. Should the URL be normalized before being passed to 
 regex-matching directives, or is there a specific reason that is not done?
 
 +---+--+--+--+
 | Path  |   Non-Regex  |*Match,   |*Match,   |
 |   |  Directive:  | RewriteRule: | RewriteRule: |
 |   |  /slash/foo  |  ^/slash/foo | ^/slash/+foo |
 +---+--+--+--+
 | /slash/foo| Match| Match| Match|
 +---+--+--+--+
 | slash/foo | Match| Match| Match|
 +---+--+--+--+
 | /slash///foo  | Match|  XXX | Match|
 +---+--+--+--+
 | slash///foo// | Match|  XXX | Match|
 +---+--+--+--+
 

*Match, RewriteRule POLA violation?

[Jim Riggs]

This came up at ApacheCon a couple of weeks ago. I just took this knowledge for 
granted, as I have always accounted for it, but both Rich and Trawick were 
surprised. As I thought about it some more, it seems this may be a POLA 
violation. Thoughts? If we agree it should be fixed, I can make the bugz and 
make a patch.

Consider:

Location /slash/foo
 ...
/Location

vs.

LocationMatch ^/slash/foo
 ...
/LocationMatch


These do not behave the same if multiple slashes are used. The leading slashes 
are always coalesced, so ^/... is fine; however, any intermediate slashes are 
not. So, in order for the LocationMatch directive above to behave the same as 
the Location, it has to be specified as ^/slash/+foo. Like I said, I have 
always accounted for this in my regexps, but it doesn't seem right. Should 
the URL be normalized before being passed to regex-matching directives, or is 
there a specific reason that is not done?

+---+--+--+--+
| Path  |   Non-Regex  |*Match,   |*Match,   |
|   |  Directive:  | RewriteRule: | RewriteRule: |
|   |  /slash/foo  |  ^/slash/foo | ^/slash/+foo |
+---+--+--+--+
| /slash/foo| Match| Match| Match|
+---+--+--+--+
| slash/foo | Match| Match| Match|
+---+--+--+--+
| /slash///foo  | Match|  XXX | Match|
+---+--+--+--+
| slash///foo// | Match|  XXX | Match|
+---+--+--+--+

Re: *Match, RewriteRule POLA violation?

[Jim Riggs]

 On 1 May 2015, at 10:52, André Malo n...@perlig.de wrote:
 
 * Niklas Edmundsson wrote:
 
 On Thu, 30 Apr 2015, Yann Ylavic wrote:
 On Thu, Apr 30, 2015 at 2:57 PM, Jim Riggs apache-li...@riggs.me 
 wrote:
 Thanks, Yann. I remember looking at this code before. The question
 remains, though: Is it currently wrong? Does it need to be fixed,
 or was this distinction made intentionally? Is there a specific use
 case that requires the regex-matching directives to not get
 slash-normalized URIs?
 
 I would like it to be fixed, non leading /+ is equivalent to /,
 this would break very few (if any) cases IMHO, and may even unbreak
 more ones .
 
 +1
 
 I would expect Location and LocationMatch using the same uri for
 comparison.
 
 Hmm, that assumption is wrong by definition. Location always matches a 
 prefix (a part of a parsed/unparsed url), while LocationMatch always 
 matches the complete URL.

We need to be careful on that phrasing. LocationMatch *can* match the complete 
URL if it is anchored at both ends. By default, though, it will match anywhere 
in the URL if it is unanchored, and it can (with the '/+' we are discussing 
here) behave just like Location and match a prefix if it is only anchored at 
the beginning.


 I would actually go so far as the current state might 
 warrant a CVE for being a hidden security risk that might cause
 inadvertent information exposure.
 
 It *is* documented right here, btw: 
 http://httpd.apache.org/docs/2.4/mod/core.html#location
 
 (found it, eventually...)

Yay! I knew I had read it somewhere. Good find!

However, that documentation is actually not correct. This is not true: For 
example, LocationMatch \^/abc\ would match the request URL /abc but not the 
request URL //abc. Based on all of my tests, the leading slash *is* collapsed 
in the *Match and RewriteRule directives. Subsequent slashes after the first 
are not.

So, it is documented, but is there a compelling use case for this? When would 
someone actually need to match against the multiple slashes (unless it's some 
really strange hack someone has implemented)? The only thing I can think of is 
that maybe you want to force a redirect to remove multiple slashes, but 
couldn't a directive in mod_alias(?) maybe handle that if deemed necessary?

Also, yes it is documented, but when I brought it up at a hackathon table full 
of long-time committers at ApacheCon, no one knew about it, and I had to make 
an example. This says to me says that maybe it's unexpected behavior and a 
potential security risk as both Niklas and Ruggeri have suggested. That's why I 
brought it up in the first place.

And so the questions still remain: Right or wrong? Change it or leave it? 
Expected or unexpected? Security risk or user/configuration error?

The world may never know... ;-)

I may go ahead and write up a patch this weekend to change them all (*Match and 
RewriteRule) and then we can all debate it over on bugz too!

Proposal/RFC: informed load balancing

[Jim Riggs]

[ Long message and proposal follows. Bear with me. There are a lot of words, 
but that is because we need a lot of help/input! ;-) ]

So, this has come up in the past several times, and we discussed it again this 
year at ApacheCon: How do we get the load balancer to make smarter, more 
informed decisions about where to send traffic?

The different LB methods provide some different attempts at balancing traffic, 
but ultimately none of them is smart about its decision. Other than a member 
being in error state, the balancer makes its decision solely based on 
configuration (LB set, factor, etc.) and its own knowledge of the member (e.g. 
requests, bytes). What we have often discussed is a way to get some type of 
health/load/capacity information from the backend to make informed balancing 
decisions.

One method is to use health checks (a la haproxy, AWS ELBs, etc.) that request 
one or more URLs and the response code/time indicates whether or not the 
service is up and available, allowing more proactive decisions. While this is 
better than our current state of reactively marking members in error state 
based on failed requests, it still provides a limited view of the health/state 
of the backend.

We have also discussed implementing a way for backends to communicate a magical 
load number to the front end to take into account as it balances traffic. 
This would give a much better view into the backend's state, but requires some 
way to come up with this calculation that each backend 
system/server/service/app must provide. This then has to be implemented in all 
the various backends (e.g. httpd, tomcat, php-fpm, unicorn, mongrel, etc., 
etc.), probably a hard sell to all of those projects. And, the front end would 
have limited control over what that number means or how to use it.

During JimJag's balancer talk at ApacheCon this year, he brought up this issue 
of better, more informed decision making again. I put some thought into it 
that night and came up with some ideas. Jim, Covener, Trawick, Ruggeri, and I 
then spent some time over the next couple of days talking it through and 
fleshing out some of the details.

Based on all of that, below is what I am proposing. I have some initial code 
that I am working on to implement the different pieces of this, and I will put 
them up in bugz or somewhere when they're a little less rudimentary.

--

Our hope is to create a general standard that can be used by various projects, 
products, proxies, servers, etc., to have a more consistent way for a load 
balancer to request and receive useful internal state information from its 
backend nodes. This information can then be used by the *frontend* 
software/admin (this is the main change from what we have discussed before) to 
calculate a load factor appropriate for each backend node.

This communication uses a new, standard HTTP header, X-Backend-Info, that 
takes this form in RFC2616 BNF:

backend-info  = version = numeric-entry
[
  *LWS , *LWS
  #( numeric-entry | string-entry )
]

numeric-entry = numeric-field = ( float |  float  )
; that is, numbers may optionally be enclosed in
; quotation marks

float = 1*DIGIT [ . 1*DIGIT ]

numeric-field = workers-max
; maximum number of workers the backend supports
  | workers-used
; current number of used/busy workers
  | workers-allocated
; current number of allocated/ready workers
  | workers-free
; current number of workers available for use
; (generally the difference between workers-max and
; workers-used, though some implementations may have
; a different notion)
  | uptime
; number of seconds the backend has been running
  | requests
; number of requests the backend has processed
  | memory-max
; total amount of memory available in bytes
  | memory-used
; amount of used memory in bytes
  | memory-allocated
; amount of allocated/committed memory in bytes
  | memory-free
; amount of memory available for use (generally
; the difference between memory-max and memory-used,
; though some implementations may have a different
; notion)
  | load-current
; the (subjective) current load for the backend
  | load-5
; the (subjective) 5-minute load for the backend
  | load-15
; the (subjective) 15-minute load for the backend

string-entry  = string-field = ( token | 

Re: Listen on UDS

[Jim Riggs]

 On 28 Apr 2015, at 10:20, Graham Leggett minf...@sharp.fm wrote:
 
 On 28 Apr 2015, at 5:17 PM, Jim Jagielski j...@jagunet.com wrote:
 
 Anyone looked into having httpd be able to Listen on a UDS, as
 well as scenarios where we may want that even?
 
 I have always wanted it - one thing it allows us to do is reverse proxy to 
 versions of httpd (or other daemon software) running as another user, with a 
 proper hope of securing it.

+1. Reverse proxy is the prime example I can think of for it.

ApacheCon Arrival

[Jim Riggs]

I just updated https://wiki.apache.org/httpd/Face2Face for Austin. If you are 
getting into town on Sunday and are interested in grabbing some dinner, let me 
know or update the page. Some of us got together last year on ApacheCon Eve and 
had a nice time. Also update the page if there are other httpd-related 
meetings/hangouts/hacking sessions you would like to get going.

See you all -- OK, several of you at least -- next week!

Re: 2.2 and 2.4 and 2.6/3.0

[Jim Riggs]

 On 28 May 2015, at 14:30, Reindl Harald h.rei...@thelounge.net wrote:
 
 Am 28.05.2015 um 21:22 schrieb Rich Bowen:
 On 05/27/2015 05:38 PM, olli hauer wrote:
 - for long time there was no working mod_php module for 2.4, and
 changing to
   php-fpm was not for everyone a solution.
 
 In my experience, the only reason that php-fpm wasn't a solution for
 everyone is that it was poorly documented. We could still stand to do
 better there, but php-fpm is, in fact, a solution for everyone
 
 no, because it does not support php_admin_flag and php_admin_value inside 
 of Directory and so would require re-design environments with many hundrets 
 of vhosts running for many years that way with automatic deployment of such 
 rules depending on the target application

Having to expend effort (e.g. re-design/update config and deployment) to 
switch/update/upgrade to a new paradigm does not, IMO, mean that it's not a 
solution for everyone. Anyone can take the time to implement and automate the 
switch. Once that effort has been spent, you should have nearly the same (maybe 
better) setup, with hopefully better security and resource utilization in many 
cases. All of those php_admin_* directives end up in a php-fpm conf file that 
can easily be automatically updated and deployed.

I'm certainly not saying that this work is trivial with many years of history, 
and it may not be for everyone, but it is certainly possible for anyone.

With fpm and mod_proxy_fcgi, I don't see myself ever using mod_php again, but 
that's just me.

Re: A little nit

[Jim Riggs]

> On 2 Aug 2017, at 12:33, Jim Jagielski  wrote:
> 
> I'll be adding some code to allow for lbfactors to be
> single decimal numbers (like 1.1, 2.5, etc...)... People
> have asked "How do I change it so that machine B is like 10%
> preferred" and I mention that "Well, you could make one a
> 10 and the other an 11" but that confuses people. :/

Is this just some GUI/config magic with the existing int (e.g. shifting decimal 
places), or are you thinking of changing lbfactor to a FP type in the struct? 
It would be really nice if lbfactor (and lbstatus?) were floating point. I have 
had a couple of instances where that would have helped me do something, but I 
realize that would not an insignificant change to the code or its compatibility.

Re: 2.4.27

[Jim Riggs]

> On 11 Jul 2017, at 11:46, James Cloos  wrote:
> 
>> "JJ" == Jim Jagielski  writes:
> 
> JJ>   *) mod_http2: disable and give warning when mpm_prefork is encountered. 
> The server will
> JJ>  continue to work, but HTTP/2 will no longer be negotiated. [Stefan 
> Eissing]
> 
> Why break h2 w/ prefork.
> 
> AIUI, one still needs to use prefork with the perl and php modules.
> 
> And it works fine here with 2.4.25.
> 
> That change makes 2.4.27 worthless.

See previous discussion in the "2.4.27" thread, specifically:

https://lists.apache.org/thread.html/bae472cadaeeb761b88bb4569cc0b7d87bc2dcb2fbcbf472d895f32e@%3Cdev.httpd.apache.org%3E

Re: A little nit

[Jim Riggs]

> On 3 Aug 2017, at 08:30, Jim Jagielski  wrote:
> 
> It's just GUI magic... Basically, it will internally take '1.1' and
> convert it to 11, 1.0 to 10, etc...

If that's the case, I would recommend going 2 decimal places (1.1 = 100, 1.25 = 
125, etc.) to allow using percentages. I could see people wanting to be able to 
favor a backend by 25%, for example, by using 1.25. Any more than 2 decimal 
places could be rounded to 2.

Re: Ideas from ApacheCon

[Jim Riggs]

> On 18 May 2017, at 12:46, Jim Jagielski  wrote:
> 
> Based on feedback from various sessions:
> 
>  o A new-kind of "hot standby" in mod_proxy which kicks
>in whenever a worker moves out of the pool (ie, doesn't
>wait until all workers are out)... ala a redundant
>hard drive.
> 
>  o Look into AAA and mod_cache; eg: "bolt in at the end"
> 
>  o HTTP/2 no longer experimental!
> 
>  o balancer-manager more scriptable (move to REST for realz?)
> 
>  o When restarting w/ persistent balancer data, warn if
>config files mtime is newer.
> 
>  o Warn if the trailing '/'s don't match in ProxyPass/Reverse
>directives (eg: ProxyPass /foo http://www.example.com/foo/ )
> 
> All I can recall at present...


  o Investigate mod_cache performance when bypassing cache (e.g.
`Cache-Control: no-cache'). In benchmark testing for my
talk,

  (1) no mod_cache
  vs.
  (2) cached responses
  vs.
  (3) cache enabled but requests specify `Cache-Control: no-cache'

showed that (3) was significantly slower than (1). I would expect
that no-cache would short-circuit/bypass mod_cache and have
results similar to (1), but there is *something* going on in
there. I have not investigated the code...just noted the results.

Re: Ideas from ApacheCon

[Jim Riggs]

> On 22 May 2017, at 06:45, Jim Jagielski <j...@jagunet.com> wrote:
> 
> I'll let Jim Riggs answer that...it came up during his mod_cache
> talk.
>> On May 18, 2017, at 2:25 PM, Eric Covener <cove...@gmail.com> wrote:
>> 
>> On Thu, May 18, 2017 at 2:22 PM, Rainer Jung <rainer.j...@kippdata.de> wrote:
>>>> o Look into AAA and mod_cache; eg: "bolt in at the end"
>> 
>> Does that differ from "CacheQuickHandler OFF"?


This one is a bit quirky and requires some investigation to see the edge cases. 
There appear to be some situations where Require directives with 
`CacheQuickHandler off' do not always take effect. Thus, items that have been 
cached will be served from the cache even if access restrictions would 
otherwise disallow them.

We will just need to do some more testing to see when -- and if -- this is 
actually occurring. I stumbled across it while doing testing/prep for my talk, 
but did not investigate it thoroughly. (It is entirely possible that I just 
messed something up in my config and didn't catch it...or forgot a 
graceful...or something...)

In my talk, I just told people to "be careful" with access control and CQF off.

Re: Ideas from ApacheCon

[Jim Riggs]

> On 18 May 2017, at 13:22, Rainer Jung  wrote:
> 
> Am 18.05.2017 um 19:46 schrieb Jim Jagielski:
>> Based on feedback from various sessions:
>> 
>> o A new-kind of "hot standby" in mod_proxy which kicks
>>   in whenever a worker moves out of the pool (ie, doesn't
>>   wait until all workers are out)... ala a redundant
>>   hard drive.
> 
> Maybe "spare worker" (and we could have more than one such spares).

Exactly. I am already working on some code for this, though it to seems to 
necessarily be a bit convoluted in the current codebase.

The way we treat a "hot standby" in mod_proxy_balancer is as a last-ditch 
effort to return something. I.e. *all* workers are unavailable, so then we use 
the hot standby. (This problem can actually be solved a different way by 
setting a high value for lbset.)

In my mind, though, what is proposed here is actually how I actually expect a 
"hot standby" to work. Think of it more like a "hot spare" in disk RAID terms. 
That is, if *any* worker is unavailable, the hot spare will be used (or at 
least added to the list of potential workers...still to be determined by the 
lbmethod implementation).

Example:


 BalancerMember "http://192.168.1.50:80;
 BalancerMember "http://192.168.1.51:80;
 BalancerMember "http://192.168.1.52:80;
 BalancerMember "http://192.168.1.53:80; status=+H


In this case, .53 will only get used if .50, .51, and .52 are *all* unavailable.


 BalancerMember "http://192.168.1.50:80;
 BalancerMember "http://192.168.1.51:80;
 BalancerMember "http://192.168.1.52:80;
 BalancerMember "http://192.168.1.53:80; status=+R # new "hot spare" status
 BalancerMember "http://192.168.1.54:80; status=+R # new "hot spare" status


In this case, if .50 becomes unavailable, .53 (or .54 depending on 
implementation) will be treated as an available worker for the lbmethod to 
potentially choose. If 2 or more of .50, .51, and .52 become unavailable, both 
.53 and .54 would be available to be chosen.

So, instead of having a single fallback option when *all* workers are dead, we 
will have a way of trying to ensure that a specific number of workers (3 in the 
example above) are always available...just like a hot spare drive plugs into 
the RAID array when one of the members dies. In our case, though, once the main 
worker recovers, the hot spare will go back to being a hot spare (except for 
matching routes).

Comments welcome.

Broken OCSP Stapling

[Jim Riggs]

This was mentioned in today's Bulletproof TLS newsletter 
(https://www.feistyduck.com/bulletproof-tls-newsletter/issue_28_lets_encrypt_downtime.html):

https://blog.hboeck.de/archives/886-The-Problem-with-OCSP-Stapling-and-Must-Staple-and-why-Certificate-Revocation-is-still-broken.html

It discusses httpd's (and nginx's) broken OCSP stapling implementations. This 
is outside of my wheelhouse, but wanted to raise awareness for someone familiar 
with that code who may be interested in taking a look. The post references 
bz57121 from 2014(!).

Implement hot spares in mod_proxy_balancer [was Re: Ideas from ApacheCon]

[Jim Riggs]

> On 1 Jun 2017, at 17:15, Yann Ylavic <ylavic@gmail.com> wrote:
> 
> On Thu, Jun 1, 2017 at 10:48 PM, Jim Riggs <apache-li...@riggs.me> wrote:
>>> On 1 Jun 2017, at 15:25, Yann Ylavic <ylavic@gmail.com> wrote:
>>> 
>>> On Thu, Jun 1, 2017 at 10:22 PM, Yann Ylavic <ylavic@gmail.com>
>>> wrote:
>>>> On Thu, Jun 1, 2017 at 7:29 PM, Jim Riggs <apache-li...@riggs.me>
>>>> wrote:
>>>>>> On 1 Jun 2017, at 07:55, Jim Jagielski <j...@jagunet.com>
>>>>>> wrote: 2. I understand the logic behind creating the arrays,
>>>>>> but doesn't this increase the overhead. We go thru the full
>>>>>> list of workers one time, and then go thru the list of avail
>>>>>> works and spares right after that. Assume that all workers
>>>>>> are available; doesn't it mean we go thru that last 2x?
>>>> []
>>>>> 
>>>>> The only way I can think of to avoid this without going back
>>>>> to duplicating code across lbmethods, which I would argue is
>>>>> worse, is to maybe have the lbmethod provide a callback
>>>>> function and context pointer to
>>>>> ap_proxy_balancer_usable_workers() that gets called during the
>>>>> loop iteration to pick the best member in flight.
>>>> 
>>>> Couldn't a simple 'best' for ap_proxy_balancer_usable_workers()
>>>> make it return a single entry?
>>> 
>>> ... a simple 'best' *flag* (as argument) ...
>> 
>> I'm not sure I follow what this flag would be. The lbmethod would
>> somehow have to tell ap_proxy_balancer_usable_workers() how to pick
>> the best worker (e.g. by comparing the number of bytes sent or the
>> number of requests processed). I'm not sure how that information
>> could be passed as a flag unless we baked the behavior of byrequests,
>> bybusyness, and bytraffic into ap_proxy_balancer_usable_workers().
>> But then how would we allow for plugging in additional lbmethods?
> 
> Oh right, nevermind, I thought per lbmethod logic was already there.
> After a better look into it, a callback (and its baton) with that
> logic looks straightforward, though.

Indeed. I put a new patch that uses callbacks on BZ61140 (and visually 
available at https://github.com/jhriggs/httpd/pull/1/files). So, the lbmethods 
call ap_proxy_balancer_get_best_worker() with a callback and baton so that the 
best worker is chosen in flight during iteration of the workers, addressing 
Jim's overhead concern. Changes:

- use a callback and baton to the lbmethod in
  ap_proxy_balancer_get_best_worker() (previously
  ap_proxy_balancer_usable_workers()) so that we can find the best worker in
  flight while iterating rather than returning an array of usable workers that
  has to then in turn be iterated

- consider a draining worker unusable and suitable for replacement by a spare


> Regarding:
> worker = _ARRAY_IDX(balancer->workers, i, proxy_worker *);
> I don't see why worker needs to be a 'proxy_worker**' ('*worker' is
> never updated IIUC).
> 
> Looks like:
> proxy_worker *worker = APR_ARRAY_IDX(balancer->workers, i, proxy_worker *);
> would be fine and allow using 'worker->xxx' instead of
> '(*worker)->xxx' all over the place...

Much of what I did was based on what already existed. The existing code was 
iterating through proxy_worker**, so mine did too. You are correct, though, 
that there is no real reason for it. The latest patch switches to just using * 
rather than **.

Thanks for putting eyes on this! I appreciate it.

Re: Ideas from ApacheCon

[Jim Riggs]

> On 2 Jun 2017, at 07:20, Jim Jagielski <j...@jagunet.com> wrote:
> 
>> On Jun 1, 2017, at 1:29 PM, Jim Riggs <apache-li...@riggs.me> wrote:
>> 
>> Regardless, even worst case, we are looking at what, iterating 6 pointers 
>> instead of 3 or 10 instead of 5? We probably have some lower hanging fruit 
>> across the request lifecycle code to increase performance than saving some 
>> arithmetic on a handful of structs, no? ;-)
>> 
> 
> Oh I agree... it just seems that we add overhead during the
> normal and nominal case (everybody is up and available) and
> that the overhead is worse the more workers we have. I am
> personally OK with abnormal situations being slower
> 
> And yeah, I also have never like the duplication that's inherent
> in all the lbmethods, and so I really like it being simplified.
> But I could never figure out a way to remove that without making
> it more complex or slower. Last idea I had was to create some
> sort of looping cpp MACRO to encapsulate it at some level :/

Yeah. Take a look at the callback version I submitted last night. It has 
reduced the code in the lbmethods to almost nothing. The find_best_*() 
functions just call ap_proxy_balancer_get_best_worker() with a small, quick 
callback function and then do a little housekeeping. Nothing else.

BUT, this implementation makes it extremely flexible if someone want to create 
a complex lbmethod. It can do whatever special/custom things it wants to in its 
find_best_() function and/or in the is_best_() callback...or maybe it wants to 
implement its own iteration for finding usable workers and doesn't want to use 
ap_proxy_balancer_get_best_worker()'s logic at all. Just because the 3 existing 
lbmethods use that flow doesn't mean everything has to, but we removed a lot of 
duplicate code with a shared function for them (and future lbmethods?) to use. 
I think it's a big win for extensibility and simplification of the lbmethod 
interface.

Re: Ideas from ApacheCon

[Jim Riggs]

> On 23 May 2017, at 09:16, Jim Riggs <apache-li...@riggs.me> wrote:
> 
>> On 18 May 2017, at 13:22, Rainer Jung <rainer.j...@kippdata.de> wrote:
>> 
>> Am 18.05.2017 um 19:46 schrieb Jim Jagielski:
>>> Based on feedback from various sessions:
>>> 
>>> o A new-kind of "hot standby" in mod_proxy which kicks
>>>  in whenever a worker moves out of the pool (ie, doesn't
>>>  wait until all workers are out)... ala a redundant
>>>  hard drive.
>> 
>> Maybe "spare worker" (and we could have more than one such spares).
> 
> Exactly. I am already working on some code for this, though it to seems to 
> necessarily be a bit convoluted in the current codebase.
> 
> The way we treat a "hot standby" in mod_proxy_balancer is as a last-ditch 
> effort to return something. I.e. *all* workers are unavailable, so then we 
> use the hot standby. (This problem can actually be solved a different way by 
> setting a high value for lbset.)
> 
> In my mind, though, what is proposed here is actually how I actually expect a 
> "hot standby" to work. Think of it more like a "hot spare" in disk RAID 
> terms. That is, if *any* worker is unavailable, the hot spare will be used 
> (or at least added to the list of potential workers...still to be determined 
> by the lbmethod implementation).
> 
> Example:
> 
> 
> BalancerMember "http://192.168.1.50:80;
> BalancerMember "http://192.168.1.51:80;
> BalancerMember "http://192.168.1.52:80;
> BalancerMember "http://192.168.1.53:80; status=+H
> 
> 
> In this case, .53 will only get used if .50, .51, and .52 are *all* 
> unavailable.
> 
> 
> BalancerMember "http://192.168.1.50:80;
> BalancerMember "http://192.168.1.51:80;
> BalancerMember "http://192.168.1.52:80;
> BalancerMember "http://192.168.1.53:80; status=+R # new "hot spare" status
> BalancerMember "http://192.168.1.54:80; status=+R # new "hot spare" status
> 
> 
> In this case, if .50 becomes unavailable, .53 (or .54 depending on 
> implementation) will be treated as an available worker for the lbmethod to 
> potentially choose. If 2 or more of .50, .51, and .52 become unavailable, 
> both .53 and .54 would be available to be chosen.
> 
> So, instead of having a single fallback option when *all* workers are dead, 
> we will have a way of trying to ensure that a specific number of workers (3 
> in the example above) are always available...just like a hot spare drive 
> plugs into the RAID array when one of the members dies. In our case, though, 
> once the main worker recovers, the hot spare will go back to being a hot 
> spare (except for matching routes).
> 
> Comments welcome.


As promised, balancer hot spare support: 
https://bz.apache.org/bugzilla/show_bug.cgi?id=61140

Re: Ideas from ApacheCon

[Jim Riggs]

> On 1 Jun 2017, at 07:55, Jim Jagielski <j...@jagunet.com> wrote:
> 
> I really like where this is going... I just have a few questions:
> 
>  1. The style, esp with array usage is different; eg APR_ARRAY_PUSH
> and APR_ARRAY_IDX... any particular reason why?

Well, we don't seem to be entirely consistent with using elts and apr_array*() 
directly vs. the helper #defines across the codebase. I was doing everything 
with direct pointer access and arithmetic until I started looking at the 
heartbeat lbmethod that was using some of the helpers. IMNSHO, I think they are 
a little cleaner and easier use/read. I got lost in dereferencing hell with all 
of the `&((*...) **)->(*)'! :-) The helpers are there, and I think they are 
convenient and worth using, but I'm not digging through all of this code as 
regularly as some of the others on here. If the standard/consensus is the other 
way, it's easy to change.


>  2. I understand the logic behind creating the arrays, but doesn't
> this increase the overhead. We go thru the full list of workers
> one time, and then go thru the list of avail works and spares
> right after that. Assume that all workers are available; doesn't
> it mean we go thru that last 2x?

If everything is available, the old implementation only goes through the list 
once and picks a worker. This new implementation goes through the list once and 
returns a *subset* that the lbmethod then has to loop through to pick a worker. 
Worst case (e.g. X workers all available in lbset 0), yes, we would now do 2X.

In a more complex scenario (e.g. X workers all available in each lbsets 0, 1, 
and 2), we do 3X (1st loop) + X (2nd loop) = 4X, not 2 * 3X = 6X. So, yes, 
there is slightly more overhead...but, the second loop (in the lbmethod) has at 
most one lbset's worth of workers and really isn't doing anything except an 
arithmetic comparison to pick the best from them.

If a number of workers are unavailable, however, it's different. The old 
implementation loops the whole list twice per lbset if it doesn't find a 
worker, once for workers and once for standbys, whereas this new implementation 
still loops the whole list only once per lbset. In the example above, 2 * 3X = 
6X for old, 3X + X = 4X for new.

The only way I can think of to avoid this without going back to duplicating 
code across lbmethods, which I would argue is worse, is to maybe have the 
lbmethod provide a callback function and context pointer to 
ap_proxy_balancer_usable_workers() that gets called during the loop iteration 
to pick the best member in flight. I am open to that...it just seemed to add 
unnecessary complexity.

Regardless, even worst case, we are looking at what, iterating 6 pointers 
instead of 3 or 10 instead of 5? We probably have some lower hanging fruit 
across the request lifecycle code to increase performance than saving some 
arithmetic on a handful of structs, no? ;-)


>> On May 31, 2017, at 1:44 PM, Jim Riggs <apache-li...@riggs.me> wrote:
>> 
>>> On 23 May 2017, at 09:16, Jim Riggs <apache-li...@riggs.me> wrote:
>>> 
>>>> On 18 May 2017, at 13:22, Rainer Jung <rainer.j...@kippdata.de> wrote:
>>>> 
>>>> Am 18.05.2017 um 19:46 schrieb Jim Jagielski:
>>>>> Based on feedback from various sessions:
>>>>> 
>>>>> o A new-kind of "hot standby" in mod_proxy which kicks
>>>>> in whenever a worker moves out of the pool (ie, doesn't
>>>>> wait until all workers are out)... ala a redundant
>>>>> hard drive.
>>>> 
>>>> Maybe "spare worker" (and we could have more than one such spares).
>>> 
>>> Exactly. I am already working on some code for this, though it to seems to 
>>> necessarily be a bit convoluted in the current codebase.
>>> 
>>> The way we treat a "hot standby" in mod_proxy_balancer is as a last-ditch 
>>> effort to return something. I.e. *all* workers are unavailable, so then we 
>>> use the hot standby. (This problem can actually be solved a different way 
>>> by setting a high value for lbset.)
>>> 
>>> In my mind, though, what is proposed here is actually how I actually expect 
>>> a "hot standby" to work. Think of it more like a "hot spare" in disk RAID 
>>> terms. That is, if *any* worker is unavailable, the hot spare will be used 
>>> (or at least added to the list of potential workers...still to be 
>>> determined by the lbmethod implementation).
>>> 
>>> Example:
>>> 
>>> 
>>> BalancerMember "http://192.168.1.50:80;
>>> BalancerMember "http://192.168.1.51:80;
>>> BalancerMember "http://192.168

Re: Ideas from ApacheCon

[Jim Riggs]

> On 1 Jun 2017, at 15:25, Yann Ylavic <ylavic@gmail.com> wrote:
> 
> On Thu, Jun 1, 2017 at 10:22 PM, Yann Ylavic <ylavic@gmail.com> wrote:
>> On Thu, Jun 1, 2017 at 7:29 PM, Jim Riggs <apache-li...@riggs.me> wrote:
>>>> On 1 Jun 2017, at 07:55, Jim Jagielski <j...@jagunet.com> wrote:
>>>> 2. I understand the logic behind creating the arrays, but doesn't
>>>>this increase the overhead. We go thru the full list of workers
>>>>one time, and then go thru the list of avail works and spares
>>>>right after that. Assume that all workers are available; doesn't
>>>>it mean we go thru that last 2x?
>> []
>>> 
>>> The only way I can think of to avoid this without going back to
>>> duplicating code across lbmethods, which I would argue is worse, is
>>> to maybe have the lbmethod provide a callback function and context
>>> pointer to ap_proxy_balancer_usable_workers() that gets called during
>>> the loop iteration to pick the best member in flight.
>> 
>> Couldn't a simple 'best' for ap_proxy_balancer_usable_workers() make
>> it return a single entry?
> 
> ... a simple 'best' *flag* (as argument) ...

I'm not sure I follow what this flag would be. The lbmethod would somehow have 
to tell ap_proxy_balancer_usable_workers() how to pick the best worker (e.g. by 
comparing the number of bytes sent or the number of requests processed). I'm 
not sure how that information could be passed as a flag unless we baked the 
behavior of byrequests, bybusyness, and bytraffic into 
ap_proxy_balancer_usable_workers(). But then how would we allow for plugging in 
additional lbmethods?


>> 
>> By the way, retrieving ap_proxy_retry_worker() via an OPTIONAL_FN
>> looks unnecessary since it's implemented in the exact same
>> "proxy_util.c" file.

LOL! That's a copy-paste remnant from when I had that function in a different 
file originally. Patch updated. Thanks for catching it!

Re: svn commit: r20021 - /dev/httpd/

[Jim Riggs]

I don't know that it really matters, but this guy is in there twice (in each 
CHANGES doc), once with the PR # and once without:

> +  *) mod_proxy: Allow the per-request environment variable "no-proxy" to
> + be used as an alternative to ProxyPass /path !. This is primarily
> + to set exceptions for ProxyPass specified in  context.
> +Use SetEnvIf, not SetEnv. [Eric Covener]


> +  *) mod_proxy: Allow the per-request environment variable "no-proxy" to
> + be used as an alternative to ProxyPass /path !. This is primarily
> + to set exceptions for ProxyPass specified in  context.
> + Use SetEnvIf, not SetEnv. PR 60458.  [Eric Covener]
> +

ACNA Miami: Who? When?

[Jim Riggs]

So, who all will be in Miami? From what I've seen on Sched and messages here:

Yes : jimjag, rich, jfc, ruggeri, me
No  : rowe, covener

There are several other ACNA regulars who have been quiet around here lately. 
Anyone else coming in?

I'll be there Sunday evening through Friday morning. I would update 
https://wiki.apache.org/httpd/Face2Face like usual, but it got locked down for 
spam.

I'm up for a meal with any of you anytime. Also, contrary to the seeming 
consensus on the hackathon thread a couple weeks ago (that I missed), I have 
had a lot of luck pounding things out during random hacking sessions or down 
times at the conference, most notably that mod_cache-thundering-herd-lock-file 
issue that a group of us really hammered on a couple years back. (It was 
absolutely killing me at work!) That said, I'm always up for hacking on stuff 
with people, especially when it's something nagging like that.

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

[Jim Riggs]

> On 11 Apr 2018, at 08:28, Yann Ylavic  wrote:
> 
> On Wed, Apr 11, 2018 at 2:11 PM,   wrote:
>> Author: jhriggs
>> Date: Wed Apr 11 12:11:05 2018
>> New Revision: 1828890
>> 
>> URL: http://svn.apache.org/viewvc?rev=1828890=rev
>> Log:
>> mod_proxy_balancer: Add hot spare member type and corresponding flag (R). 
>> Hot spare members are
>> used as drop-in replacements for unusable workers in the same load balancer 
>> set. This differs
>> from hot standbys which are only used when all workers in a set are 
>> unusable. PR 61140.
> 
> Nice ap_proxy_balancer_get_best_worker() simplification Jim.
> 
> Maybe it could have been a separate commit than the spare members
> addition though, not mixing refactoring and features.
> Staging helps review IHMO.

Thanks for the feedback, Yann!

I never really viewed ap_proxy_balancer_get_best_worker() as a refactor 
separate from the hot spare change that required it, though I suppose it 
definitely could have stood alone. It did remove a lot of duplicate code. 
Basically, that commit is just an existing patch that has been out there 
floating around since ACNA last year that I just updated to the current 
codebase. In the dev@ thread at the time, we never discussed splitting them 
either.

Regardless, I'm glad to have the hot spare functionality that several of us 
have always thought was missing, and I'm glad I didn't appear to b0rk the 
entire repo in the process.

Thanks for taking it easy on the new guy. ;-)

- Jim

Future of hot standby in balancers [was Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/]

[Jim Riggs]

On 11 Apr 2018, at 07:11, jhri...@apache.org wrote:
> 
> Author: jhriggs
> Date: Wed Apr 11 12:11:05 2018
> New Revision: 1828890
> 
> URL: http://svn.apache.org/viewvc?rev=1828890=rev
> Log:
> mod_proxy_balancer: Add hot spare member type and corresponding flag (R). Hot 
> spare members are
> used as drop-in replacements for unusable workers in the same load balancer 
> set. This differs
> from hot standbys which are only used when all workers in a set are unusable. 
> PR 61140.

Speaking of balancer member types, hot spares (new), and hot standbys 
(existing), is there really a need for hot standbys? When I first started using 
mod_proxy_balancer many years ago, I actually thought/assumed that "hot 
standbys" had the behavior that the new spares now have (i.e. drop-in 
replacements for unavailable workers) until I studied the documentation more 
carefully.

Standbys are really superfluous, since the same behavior can be achieved by 
sets. For example, the behavior of the following will be exactly the same:


  ProxySet lbmethod=bytraffic
  BalancerMember "http://server1/; lbset=0
  BalancerMember "http://server2/; lbset=0
  BalancerMember "http://server3/; lbset=0 status=+H
  BalancerMember "http://server4/; lbset=1
  BalancerMember "http://server5/; lbset=1
  BalancerMember "http://server6/; lbset=1 status=+H



  ProxySet lbmethod=bytraffic
  BalancerMember "http://server1/; lbset=0
  BalancerMember "http://server2/; lbset=0
  BalancerMember "http://server3/; lbset=1
  BalancerMember "http://server4/; lbset=2
  BalancerMember "http://server5/; lbset=2
  BalancerMember "http://server6/; lbset=3


So, is there really any reason to keep hot standbys? Do we consider the concept 
of the +H flag to be easier to understand or use than LB sets? If so, do we 
just need better docs? Am I missing any nuances of the H flag?

I would love to see standbys go away, personally.

- Jim

Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch

[Jim Riggs]

I didn't think of this before, but there is one edge case this would miss: if 
someone (for whatever reason) wants a relative ErrorLog *file* named `syslog*', 
for example `ErrorLog "syslog-httpd.log"' or `ErrorLog "syslog.log"'. It 
appears that this is already broken in server/log.c, though. Also, log.c is 
using str(n)casecmp. The following would make things consistent and handle this 
weird edge case. Thoughts?

Index: server/core.c
===
--- server/core.c   (revision 1829431)
+++ server/core.c   (working copy)
@@ -4867,7 +4867,8 @@
 static int check_errorlog_dir(apr_pool_t *p, server_rec *s)
 {
 if (!s->error_fname || s->error_fname[0] == '|'
-|| strcmp(s->error_fname, "syslog") == 0) {
+|| strcasecmp(s->error_fname, "syslog") == 0
+|| strncasecmp(s->error_fname, "syslog:", 7) == 0) {
 return APR_SUCCESS;
 }
 else {
@@ -5281,7 +5282,9 @@
 apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root);
 tmp = ap_server_root_relative(p, sconf->ap_document_root);
 apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp);
-if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0)
+if (s->error_fname[0] != '|'
+&& strcasecmp(s->error_fname, "syslog") != 0
+&& strncasecmp(s->error_fname, "syslog:", 7) != 0)
 tmp = ap_server_root_relative(p, s->error_fname);
 else
 tmp = s->error_fname;
@@ -5456,4 +5459,3 @@
 core_cmds,/* command apr_table_t */
 register_hooks/* register hooks */
 };
-
Index: server/log.c
===
--- server/log.c(revision 1829431)
+++ server/log.c(working copy)
@@ -396,7 +396,8 @@
 }

 #ifdef HAVE_SYSLOG
-else if (!strncasecmp(s->error_fname, "syslog", 6)) {
+else if (strcasecmp(s->error_fname, "syslog") == 0
+ || strncasecmp(s->error_fname, "syslog:", 7) == 0) {
 if ((fname = strchr(s->error_fname, ':'))) {
 /* s->error_fname could be [level]:[tag] (see #60525) */
 const char *tag;



> On 18 Apr 2018, at 04:57, elu...@apache.org wrote:
> 
> Author: elukey
> Date: Wed Apr 18 09:57:08 2018
> New Revision: 1829430
> 
> URL: http://svn.apache.org/viewvc?rev=1829430=rev
> Log:
> core-check_errorlog_dir_syslog.patch: add suggestions from STATUS
> 
> Credis to jhriggs and jailletc36
> 
> 
> Modified:
>httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
> 
> Modified: httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch?rev=1829430=1829429=1829430=diff
> ==
> --- httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch (original)
> +++ httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch Wed Apr 18 
> 09:57:08 2018
> @@ -1,14 +1,23 @@
> Index: server/core.c
> ===
>  server/core.c   (revision 1829090)
> +--- server/core.c   (revision 1829429)
> +++ server/core.c   (working copy)
> @@ -4867,7 +4867,7 @@
>  static int check_errorlog_dir(apr_pool_t *p, server_rec *s)
>  {
>  if (!s->error_fname || s->error_fname[0] == '|'
> -|| strcmp(s->error_fname, "syslog") == 0) {
> -+|| !strncmp(s->error_fname, "syslog", 6)) {
> ++|| strncmp(s->error_fname, "syslog", 6) == 0) {
>  return APR_SUCCESS;
>  }
>  else {
> +@@ -5281,7 +5281,7 @@
> + apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root);
> + tmp = ap_server_root_relative(p, sconf->ap_document_root);
> + apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp);
> +-if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0)
> ++if (s->error_fname[0] != '|' && strncmp(s->error_fname, "syslog", 6) != 
> 0)
> + tmp = ap_server_root_relative(p, s->error_fname);
> + else
> + tmp = s->error_fname;
> 

Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch

[Jim Riggs]

Fair enough. I'm fine standardizing either way. strn?cmp() is probably more 
"correct". As it stands, though, the check in core is not actually checking 
everything that log.c will handle.


> On 18 Apr 2018, at 10:15, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> 
> On Wed, Apr 18, 2018 at 7:17 AM, Jim Riggs <jim...@riggs.me> wrote:
>> I didn't think of this before, but there is one edge case this would miss: 
>> if someone (for whatever reason) wants a relative ErrorLog *file* named 
>> `syslog*', for example `ErrorLog "syslog-httpd.log"' or `ErrorLog 
>> "syslog.log"'. It appears that this is already broken in server/log.c, 
>> though. Also, log.c is using str(n)casecmp. The following would make things 
>> consistent and handle this weird edge case. Thoughts?
>> 
>> Index: server/core.c
>> ===
>> --- server/core.c   (revision 1829431)
>> +++ server/core.c   (working copy)
>> @@ -4867,7 +4867,8 @@
>> static int check_errorlog_dir(apr_pool_t *p, server_rec *s)
>> {
>> if (!s->error_fname || s->error_fname[0] == '|'
>> -|| strcmp(s->error_fname, "syslog") == 0) {
>> +|| strcasecmp(s->error_fname, "syslog") == 0
>> +|| strncasecmp(s->error_fname, "syslog:", 7) == 0) {
>> return APR_SUCCESS;
>> }
>> else {
>> @@ -5281,7 +5282,9 @@
>> apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root);
>> tmp = ap_server_root_relative(p, sconf->ap_document_root);
>> apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp);
>> -if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0)
>> +if (s->error_fname[0] != '|'
>> +&& strcasecmp(s->error_fname, "syslog") != 0
>> +&& strncasecmp(s->error_fname, "syslog:", 7) != 0)
>> tmp = ap_server_root_relative(p, s->error_fname);
>> else
>> tmp = s->error_fname;
>> @@ -5456,4 +5459,3 @@
>> core_cmds,/* command apr_table_t */
>> register_hooks/* register hooks */
>> };
>> -
>> Index: server/log.c
>> ===
>> --- server/log.c(revision 1829431)
>> +++ server/log.c(working copy)
>> @@ -396,7 +396,8 @@
>> }
>> 
>> #ifdef HAVE_SYSLOG
>> -else if (!strncasecmp(s->error_fname, "syslog", 6)) {
>> +else if (strcasecmp(s->error_fname, "syslog") == 0
>> + || strncasecmp(s->error_fname, "syslog:", 7) == 0) {
>> if ((fname = strchr(s->error_fname, ':'))) {
>> /* s->error_fname could be [level]:[tag] (see #60525) */
>> const char *tag;
> 
> Shouldn't we normalize the use of strcmp instead of strcasecmp?
> In any case it must be entirely normalized to one or the other.
> 
> Linux is a case-sensitive OS in the first place, and if configured
> with SYSLOG: today it is broken when you hit core, which ignores
> that code path. Since the behavior has always been syslog: I'm
> not seeing a benefit to case insensitivity.

Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch

[Jim Riggs]

> On 20 Apr 2018, at 09:52, Luca Toscano <toscano.l...@gmail.com> wrote:
> 2018-04-20 16:27 GMT+02:00 Jim Riggs <jim...@riggs.me>:
> > On 20 Apr 2018, at 08:53, Jim Jagielski <j...@jagunet.com> wrote:
> > 
> > Sorry for coming in late, but what is the exact issue we are trying to 
> > solve again? My understanding was that if someone wanted something like
> > 
> >   ErrorLog "syslog-httpd.log"
> > 
> > that the current implementation would, incorrectly, send the log data to 
> > syslogd. Is that right?
> 
> Luca is working PR 62102 which has to do with "syslog:...:...", but that 
> unveiled an inconsistency between core.c and log.c. Before his proposed patch 
> in STATUS, core.c is doing a strcmp() for "syslog" whereas log.c is doing 
> strncasecmp(). We have been discussing standardizing both on strn?cmp(), but 
> that would potentially lead to "breaking" configs that use "SYSLOG" rather 
> than "syslog".
> 
> 
> I don't believe that they two things are separate (core/log.c), since the 
> former checks for ErrorLog's parameter sanity and the latter sets it, so it 
> would be weird in my opinion if the two logic were different. This is why I 
> liked your consistency proposal Jim, and applied to my patch. In our docs we 
> clearly specify to use "syslog" in lowercase, so as far as I can see it using 
> "SYSLOG" would not be something that people should use..

Agreed, and I believe we should go forward with this. I just wanted to at least 
bring it up, since regressions and breaking existing configs has been a touchy 
subject lately. :-)

Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch

[Jim Riggs]

Luca -

Here's the same thing standardizing on strn?cmp(). Not that you couldn't have 
done it yourself, but since I had it up, maybe this will save you 30 seconds. 
;-)


Index: server/core.c
===
--- server/core.c   (revision 1829439)
+++ server/core.c   (working copy)
@@ -4867,7 +4867,8 @@
static int check_errorlog_dir(apr_pool_t *p, server_rec *s)
{
if (!s->error_fname || s->error_fname[0] == '|'
-|| strcmp(s->error_fname, "syslog") == 0) {
+|| strcmp(s->error_fname, "syslog") == 0
+|| strncmp(s->error_fname, "syslog:", 7) == 0) {
return APR_SUCCESS;
}
else {
@@ -5281,7 +5282,9 @@
apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root);
tmp = ap_server_root_relative(p, sconf->ap_document_root);
apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp);
-if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0)
+if (s->error_fname[0] != '|'
+&& strcmp(s->error_fname, "syslog") != 0
+&& strncmp(s->error_fname, "syslog:", 7) != 0)
tmp = ap_server_root_relative(p, s->error_fname);
else
tmp = s->error_fname;
@@ -5456,4 +5459,3 @@
core_cmds,/* command apr_table_t */
register_hooks/* register hooks */
};
-
Index: server/log.c
===
--- server/log.c(revision 1829439)
+++ server/log.c(working copy)
@@ -396,7 +396,8 @@
}

#ifdef HAVE_SYSLOG
-else if (!strncasecmp(s->error_fname, "syslog", 6)) {
+else if (strcmp(s->error_fname, "syslog") == 0
+ || strncmp(s->error_fname, "syslog:", 7) == 0) {
if ((fname = strchr(s->error_fname, ':'))) {
/* s->error_fname could be [level]:[tag] (see #60525) */
const char *tag;


> On 18 Apr 2018, at 13:32, Luca Toscano <toscano.l...@gmail.com> wrote:
> 
> Thanks a lot Jim! I like your code change and the extra checks, but I'd 
> prefer to use strncmp if possible, also in log.c. 
> Feel free to amend the patch, or I'll do it tomorrow (I forgot the entry in 
> CHANGES too, your name should be on it :).
> 
> Luca
> 
> 2018-04-18 18:36 GMT+02:00 Jim Riggs <j...@riggs.me>:
> Fair enough. I'm fine standardizing either way. strn?cmp() is probably more 
> "correct". As it stands, though, the check in core is not actually checking 
> everything that log.c will handle.
> 
> 
>> On 18 Apr 2018, at 10:15, William A Rowe Jr <wr...@rowe-clan.net> wrote:
>> 
>> On Wed, Apr 18, 2018 at 7:17 AM, Jim Riggs <jim...@riggs.me> wrote:
>>> I didn't think of this before, but there is one edge case this would miss: 
>>> if someone (for whatever reason) wants a relative ErrorLog *file* named 
>>> `syslog*', for example `ErrorLog "syslog-httpd.log"' or `ErrorLog 
>>> "syslog.log"'. It appears that this is already broken in server/log.c, 
>>> though. Also, log.c is using str(n)casecmp. The following would make things 
>>> consistent and handle this weird edge case. Thoughts?
>>> 
>>> Index: server/core.c
>>> ===
>>> --- server/core.c   (revision 1829431)
>>> +++ server/core.c   (working copy)
>>> @@ -4867,7 +4867,8 @@
>>> static int check_errorlog_dir(apr_pool_t *p, server_rec *s)
>>> {
>>>if (!s->error_fname || s->error_fname[0] == '|'
>>> -|| strcmp(s->error_fname, "syslog") == 0) {
>>> +|| strcasecmp(s->error_fname, "syslog") == 0
>>> +|| strncasecmp(s->error_fname, "syslog:", 7) == 0) {
>>>return APR_SUCCESS;
>>>}
>>>else {
>>> @@ -5281,7 +5282,9 @@
>>>apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root);
>>>tmp = ap_server_root_relative(p, sconf->ap_document_root);
>>>apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp);
>>> -if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0)
>>> +if (s->error_fname[0] != '|'
>>> +&& strcasecmp(s->error_fname, "syslog") != 0
>>> +&& strncasecmp(s->error_fname, "syslog:", 7) != 0)
>>>tmp = ap_server_root_relative(p, s->error_fname);
>>>else
>>>tmp = s-

Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch

[Jim Riggs]

Luca -

Here's the same thing standardizing on strn?cmp(). Not that you couldn't have 
done it yourself, but since I had it up, maybe this will save you 30 seconds. 
;-)


Index: server/core.c
===
--- server/core.c   (revision 1829439)
+++ server/core.c   (working copy)
@@ -4867,7 +4867,8 @@
 static int check_errorlog_dir(apr_pool_t *p, server_rec *s)
 {
 if (!s->error_fname || s->error_fname[0] == '|'
-|| strcmp(s->error_fname, "syslog") == 0) {
+|| strcmp(s->error_fname, "syslog") == 0
+|| strncmp(s->error_fname, "syslog:", 7) == 0) {
 return APR_SUCCESS;
 }
 else {
@@ -5281,7 +5282,9 @@
 apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root);
 tmp = ap_server_root_relative(p, sconf->ap_document_root);
 apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp);
-if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0)
+if (s->error_fname[0] != '|'
+&& strcmp(s->error_fname, "syslog") != 0
+&& strncmp(s->error_fname, "syslog:", 7) != 0)
 tmp = ap_server_root_relative(p, s->error_fname);
 else
 tmp = s->error_fname;
@@ -5456,4 +5459,3 @@
 core_cmds,/* command apr_table_t */
 register_hooks/* register hooks */
 };
-
Index: server/log.c
===
--- server/log.c(revision 1829439)
+++ server/log.c(working copy)
@@ -396,7 +396,8 @@
 }

 #ifdef HAVE_SYSLOG
-else if (!strncasecmp(s->error_fname, "syslog", 6)) {
+else if (strcmp(s->error_fname, "syslog") == 0
+ || strncmp(s->error_fname, "syslog:", 7) == 0) {
 if ((fname = strchr(s->error_fname, ':'))) {
 /* s->error_fname could be [level]:[tag] (see #60525) */
 const char *tag;


> On 18 Apr 2018, at 13:32, Luca Toscano <toscano.l...@gmail.com> wrote:
> 
> Thanks a lot Jim! I like your code change and the extra checks, but I'd 
> prefer to use strncmp if possible, also in log.c. 
> Feel free to amend the patch, or I'll do it tomorrow (I forgot the entry in 
> CHANGES too, your name should be on it :).
> 
> Luca
> 
> 2018-04-18 18:36 GMT+02:00 Jim Riggs <j...@riggs.me>:
> Fair enough. I'm fine standardizing either way. strn?cmp() is probably more 
> "correct". As it stands, though, the check in core is not actually checking 
> everything that log.c will handle.
> 
> 
> > On 18 Apr 2018, at 10:15, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> > 
> > On Wed, Apr 18, 2018 at 7:17 AM, Jim Riggs <jim...@riggs.me> wrote:
> >> I didn't think of this before, but there is one edge case this would miss: 
> >> if someone (for whatever reason) wants a relative ErrorLog *file* named 
> >> `syslog*', for example `ErrorLog "syslog-httpd.log"' or `ErrorLog 
> >> "syslog.log"'. It appears that this is already broken in server/log.c, 
> >> though. Also, log.c is using str(n)casecmp. The following would make 
> >> things consistent and handle this weird edge case. Thoughts?
> >> 
> >> Index: server/core.c
> >> ===
> >> --- server/core.c   (revision 1829431)
> >> +++ server/core.c   (working copy)
> >> @@ -4867,7 +4867,8 @@
> >> static int check_errorlog_dir(apr_pool_t *p, server_rec *s)
> >> {
> >> if (!s->error_fname || s->error_fname[0] == '|'
> >> -|| strcmp(s->error_fname, "syslog") == 0) {
> >> +|| strcasecmp(s->error_fname, "syslog") == 0
> >> +|| strncasecmp(s->error_fname, "syslog:", 7) == 0) {
> >> return APR_SUCCESS;
> >> }
> >> else {
> >> @@ -5281,7 +5282,9 @@
> >> apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root);
> >> tmp = ap_server_root_relative(p, sconf->ap_document_root);
> >> apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp);
> >> -if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0)
> >> +if (s->error_fname[0] != '|'
> >> +&& strcasecmp(s->error_fname, "syslog") != 0
> >> +&& strncasecmp(s->error_fname, "syslog:", 7) != 0)
> >> tmp = ap_serve

Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch

[Jim Riggs]

> On 20 Apr 2018, at 01:42, Luca Toscano <toscano.l...@gmail.com> wrote:
> 2018-04-19 17:49 GMT+02:00 Jim Riggs <j...@riggs.me>:
> Luca -
> 
> Here's the same thing standardizing on strn?cmp(). Not that you couldn't have 
> done it yourself, but since I had it up, maybe this will save you 30 seconds. 
> ;-)
> 
> 
> Thanks a lot! I added your last suggestions to r1829626 and also a CHANGES 
> entry. I tested it on my local environment and it seems working fine, let me 
> know if everything looks good or if anything is missing.

I'm fine with it, *BUT* in light of our recent discussions about 
backports/fixes causing regressions in existing configs, this could cause 
another. I agree with what we have done in this patch, but we could potentially 
break existing configs, no? For example:

ErrorLog "SYSLOG"

This will send to syslog before the patch but will send to file 
/SYSLOG after. Ugh. So, should we not touch server/log.c???

Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch

[Jim Riggs]

> On 20 Apr 2018, at 08:53, Jim Jagielski  wrote:
> 
> Sorry for coming in late, but what is the exact issue we are trying to solve 
> again? My understanding was that if someone wanted something like
> 
>   ErrorLog "syslog-httpd.log"
> 
> that the current implementation would, incorrectly, send the log data to 
> syslogd. Is that right?

Luca is working PR 62102 which has to do with "syslog:...:...", but that 
unveiled an inconsistency between core.c and log.c. Before his proposed patch 
in STATUS, core.c is doing a strcmp() for "syslog" whereas log.c is doing 
strncasecmp(). We have been discussing standardizing both on strn?cmp(), but 
that would potentially lead to "breaking" configs that use "SYSLOG" rather than 
"syslog".