Re: error_time reset in proxy_util.c

[Ruediger Pluem]

On 03/05/2015 05:29 PM, Eric Covener wrote:
> On Thu, Mar 5, 2015 at 11:14 AM, Ruediger Pluem  wrote:
>> I suspect that the worker was already set to error by a parallel thread / 
>> process and hence
>> PROXY_WORKER_IS_USABLE(worker) is false and causes worker->s->error_time to 
>> be reset which causes the worker to be open
>> for retry immediately. This has been the case since r104624
>> (http://svn.apache.org/viewvc?view=revision&revision=r104624) 10,5 years ago 
>> and the commit messages offers no hint at
>> least to be why we reset these values.
>> Can anybody think of a good reason why we do this?
> 
> that sequence sounds plausible

So do you see any reason why we do this, or should we just stop doing so?

Another thing:

else {
if (worker->s->retries) {
/*
 * A worker came back. So here is where we need to
 * either reset all params to initial conditions or
 * apply some sort of aging
 */
}

The comment makes a wrong assumption as we might be here despite the connection 
failed because the worker was already in
error. So we might need to split into two ifs above and separate the conditions 
where the connection failed and where we
need to do something with the worker because it wasn't in error yet. How about:

Index: proxy_util.c
===
--- proxy_util.c(revision 1664261)
+++ proxy_util.c(working copy)
@@ -2887,33 +2887,48 @@

 connected= 1;
 }
-/*
- * Put the entire worker to error state if
- * the PROXY_WORKER_IGNORE_ERRORS flag is not set.
- * Altrough some connections may be alive
- * no further connections to the worker could be made
- */
-if (!connected && PROXY_WORKER_IS_USABLE(worker) &&
-!(worker->s->status & PROXY_WORKER_IGNORE_ERRORS)) {
-worker->s->error_time = apr_time_now();
-worker->s->status |= PROXY_WORKER_IN_ERROR;
-ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(00959)
-"ap_proxy_connect_backend disabling worker for (%s) for %"
-APR_TIME_T_FMT "s",
-worker->s->hostname, apr_time_sec(worker->s->retry));
+if (PROXY_WORKER_IS_USABLE(worker)) {
+/*
+ * Put the entire worker to error state if
+ * the PROXY_WORKER_IGNORE_ERRORS flag is not set.
+ * Altrough some connections may be alive
+ * no further connections to the worker could be made
+ */
+if (!connected) {
+if (!(worker->s->status & PROXY_WORKER_IGNORE_ERRORS)) {
+worker->s->error_time = apr_time_now();
+worker->s->status |= PROXY_WORKER_IN_ERROR;
+ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(00959)
+"ap_proxy_connect_backend disabling worker for (%s) for %"
+APR_TIME_T_FMT "s",
+worker->s->hostname, apr_time_sec(worker->s->retry));
+}
+}
+else {
+if (worker->s->retries) {
+/*
+ * A worker came back. So here is where we need to
+ * either reset all params to initial conditions or
+ * apply some sort of aging
+ */
+}
+worker->s->error_time = 0;
+worker->s->retries = 0;
+}
+return connected ? OK : DECLINED;
 }
 else {
-if (worker->s->retries) {
-/*
- * A worker came back. So here is where we need to
- * either reset all params to initial conditions or
- * apply some sort of aging
- */
+/*
+ * The worker is in error likely done by a different thread / processi
+ * e.g. for a timeout or bad status. We should respect this and should
+ * not continue with a connection via this worker even if we got one.
+ */
+if (connected) {
+apr_socket_close(conn->sock );
+conn->sock = NULL;
 }
-worker->s->error_time = 0;
-worker->s->retries = 0;
+return DECLINED;
 }
-return connected ? OK : DECLINED;
 }

 static apr_status_t connection_shutdown(void *theconn)


> 
>> Another question is if we shouldn't do
>>
>> worker->s->error_time = apr_time_now();
>>
>> also in case the worker is already in error state to restart the retry clock 
>> as we just faced an error with connecting
>> to the backend.
> 
> Isn't this already handled by the thread that put the worker in error
> during the race? If there's no race, we already have this line of code
> for the main path.
> 

Valid point, but we might have an error situation here that is more recent then 
the one by the thread that put it into
error. OTOH as I suspect a race the difference is likely to be rather slim, so 
it might not be worth the effort.

Regards

Rüdiger

Re: error_time reset in proxy_util.c

[Eric Covener]

On Thu, Mar 5, 2015 at 11:14 AM, Ruediger Pluem  wrote:
> I suspect that the worker was already set to error by a parallel thread / 
> process and hence
> PROXY_WORKER_IS_USABLE(worker) is false and causes worker->s->error_time to 
> be reset which causes the worker to be open
> for retry immediately. This has been the case since r104624
> (http://svn.apache.org/viewvc?view=revision&revision=r104624) 10,5 years ago 
> and the commit messages offers no hint at
> least to be why we reset these values.
> Can anybody think of a good reason why we do this?

that sequence sounds plausible

> Another question is if we shouldn't do
>
> worker->s->error_time = apr_time_now();
>
> also in case the worker is already in error state to restart the retry clock 
> as we just faced an error with connecting
> to the backend.

Isn't this already handled by the thread that put the worker in error
during the race? If there's no race, we already have this line of code
for the main path.

error_time reset in proxy_util.c

[Ruediger Pluem]

I am currently hunting down an issue where a balancer member that is set to 
error is reused before the retry time runs out.
I think the reason is some race condition around line 2900 in proxy_util.c

/*
 * Put the entire worker to error state if
 * the PROXY_WORKER_IGNORE_ERRORS flag is not set.
 * Altrough some connections may be alive
 * no further connections to the worker could be made
 */
if (!connected && PROXY_WORKER_IS_USABLE(worker) &&
!(worker->s->status & PROXY_WORKER_IGNORE_ERRORS)) {
worker->s->error_time = apr_time_now();
worker->s->status |= PROXY_WORKER_IN_ERROR;
ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(00959)
"ap_proxy_connect_backend disabling worker for (%s) for %"
APR_TIME_T_FMT "s",
worker->s->hostname, apr_time_sec(worker->s->retry));
}
else {
if (worker->s->retries) {
/*
 * A worker came back. So here is where we need to
 * either reset all params to initial conditions or
 * apply some sort of aging
 */
}
worker->s->error_time = 0;
worker->s->retries = 0;
}

I suspect that the worker was already set to error by a parallel thread / 
process and hence
PROXY_WORKER_IS_USABLE(worker) is false and causes worker->s->error_time to be 
reset which causes the worker to be open
for retry immediately. This has been the case since r104624
(http://svn.apache.org/viewvc?view=revision&revision=r104624) 10,5 years ago 
and the commit messages offers no hint at
least to be why we reset these values.
Can anybody think of a good reason why we do this?
Another question is if we shouldn't do

worker->s->error_time = apr_time_now();

also in case the worker is already in error state to restart the retry clock as 
we just faced an error with connecting
to the backend.


Regards

Rüdiger

AW: Run external RewriteMap program as non-root

[Plüm , Rüdiger , Vodafone Group]

> -Ursprüngliche Nachricht-
> Von: Jan Kaluža [mailto:jkal...@redhat.com]
> Gesendet: Donnerstag, 5. März 2015 14:08
> An: dev@httpd.apache.org
> Betreff: Re: Run external RewriteMap program as non-root
> 
> On 03/05/2015 12:53 PM, Yann Ylavic wrote:
> > On Thu, Mar 5, 2015 at 12:08 PM, Jan Kaluža 
> wrote:
> >> On 03/05/2015 07:55 AM, Jan Kaluža wrote:
> >>>
> >>> 3. Execute it where it is now (post_config), but set user/group
> using
> >>> apr_procattr_t. So far I think this would duplicate the code of
> >>> mod_unixd and would probably have to also handle the windows
> equivalent
> >>> of that module (if there's any).
> >>
> >>
> >> I've been thinking about this one more and with introduction of third
> >> argument to RewriteMap, it could be possible with patch similar to
> attached
> >> one.
> >>
> >> You can do "RewriteMap MapName prg:/path user:group" with the patch.
> >>
> >> This could be even backported to 2.4.x.
> >
> > I'm fine with this one too (unix only?).
> 
> Still thinking about good RewriteMap syntax to pass "password" for
> Windows. But If people don't mind, having this unix only is also
> solution :).
> 

The password issue for Windows was also on my mind :-). Having it in cleartext 
in the config seems ugly.
So Unix only should be fine at least for the start.

Regards

Rüdiger

Re: Run external RewriteMap program as non-root

[Jan Kaluža]

On 03/05/2015 12:53 PM, Yann Ylavic wrote:

On Thu, Mar 5, 2015 at 12:08 PM, Jan Kaluža  wrote:

On 03/05/2015 07:55 AM, Jan Kaluža wrote:


3. Execute it where it is now (post_config), but set user/group using
apr_procattr_t. So far I think this would duplicate the code of
mod_unixd and would probably have to also handle the windows equivalent
of that module (if there's any).



I've been thinking about this one more and with introduction of third
argument to RewriteMap, it could be possible with patch similar to attached
one.

You can do "RewriteMap MapName prg:/path user:group" with the patch.

This could be even backported to 2.4.x.


I'm fine with this one too (unix only?).


Still thinking about good RewriteMap syntax to pass "password" for 
Windows. But If people don't mind, having this unix only is also 
solution :).

Re: Run external RewriteMap program as non-root

[Eric Covener]

On Thu, Mar 5, 2015 at 4:48 AM, André Malo  wrote:
> 5) Let it drop the privileges by itself.
>
> I actually tend to 5 :-)


+1 (as a new option as described in a followup)

-- 
Eric Covener
cove...@gmail.com

Re: Run external RewriteMap program as non-root

[Yann Ylavic]

On Thu, Mar 5, 2015 at 12:08 PM, Jan Kaluža  wrote:
> On 03/05/2015 07:55 AM, Jan Kaluža wrote:
>>
>> 3. Execute it where it is now (post_config), but set user/group using
>> apr_procattr_t. So far I think this would duplicate the code of
>> mod_unixd and would probably have to also handle the windows equivalent
>> of that module (if there's any).
>
>
> I've been thinking about this one more and with introduction of third
> argument to RewriteMap, it could be possible with patch similar to attached
> one.
>
> You can do "RewriteMap MapName prg:/path user:group" with the patch.
>
> This could be even backported to 2.4.x.

I'm fine with this one too (unix only?).

Re: Run external RewriteMap program as non-root

[Jan Kaluža]

On 03/05/2015 07:55 AM, Jan Kaluža wrote:

Hi,

currently, the External Rewriting Program (RewriteMap "prg:") is run as
root. I would like to change it but I see three ways how to do it:

1. Execute it right after drop_privileges hook. This looks like best
way, but I haven't found any hook which could be used for that (except
drop_privileges with APR_HOOK_REALLY_LAST, which does not seem as proper
place to me).

2. Execute it in child_init. This is done after drop_privileges, so the
user/group is good. The "problem" here is that it would execute one
rewrite program per child. Right now I'm not sure if it's really
problem. It could be useful to have more instances of rewriting program
to make its bottleneck lower.

3. Execute it where it is now (post_config), but set user/group using
apr_procattr_t. So far I think this would duplicate the code of
mod_unixd and would probably have to also handle the windows equivalent
of that module (if there's any).


I've been thinking about this one more and with introduction of third 
argument to RewriteMap, it could be possible with patch similar to 
attached one.


You can do "RewriteMap MapName prg:/path user:group" with the patch.

This could be even backported to 2.4.x.

Jan Kaluza


What way do you think is the best, or would you do it differently?

I'm attaching patch for number 2.

Regards,
Jan Kaluza


Index: modules/mappers/mod_rewrite.c
===
--- modules/mappers/mod_rewrite.c	(revision 1663642)
+++ modules/mappers/mod_rewrite.c	(working copy)
@@ -267,6 +267,8 @@
 const char *dbdq;  /* SQL SELECT statement for rewritemap */
 const char *checkfile2;/* filename to check for map existence
   NULL if only one file   */
+const char *user;
+const char *group;
 } rewritemap_entry;
 
 /* special pattern types for RewriteCond */
@@ -1171,6 +1173,7 @@
 
 static apr_status_t rewritemap_program_child(apr_pool_t *p,
  const char *progname, char **argv,
+ const char *user, const char *group,
  apr_file_t **fpout,
  apr_file_t **fpin)
 {
@@ -1183,6 +1186,8 @@
   APR_FULL_BLOCK, APR_NO_PIPE))
 && APR_SUCCESS == (rc=apr_procattr_dir_set(procattr,
  ap_make_dirstr_parent(p, argv[0])))
+&& (!user || APR_SUCCESS == (rc=apr_procattr_user_set(procattr, user, "")))
+&& (!group || APR_SUCCESS == (rc=apr_procattr_group_set(procattr, group)))
 && APR_SUCCESS == (rc=apr_procattr_cmdtype_set(procattr, APR_PROGRAM))
 && APR_SUCCESS == (rc=apr_procattr_child_errfn_set(procattr,
rewrite_child_errfn))
@@ -1240,6 +1245,7 @@
 }
 
 rc = rewritemap_program_child(p, map->argv[0], map->argv,
+  map->user, map->group,
   &fpout, &fpin);
 if (rc != APR_SUCCESS || fpin == NULL || fpout == NULL) {
 ap_log_error(APLOG_MARK, APLOG_ERR, rc, s, APLOGNO(00654)
@@ -3018,7 +3024,7 @@
 }
 
 static const char *cmd_rewritemap(cmd_parms *cmd, void *dconf, const char *a1,
-  const char *a2)
+  const char *a2, const char *a3)
 {
 rewrite_server_conf *sconf;
 rewritemap_entry *newmap;
@@ -3124,6 +3130,11 @@
 
 newmap->type  = MAPTYPE_PRG;
 newmap->checkfile = newmap->argv[0];
+if (a3) {
+char *tok_cntx;
+newmap->user = apr_strtok(apr_pstrdup(cmd->pool, a3), ":", &tok_cntx);
+newmap->group = apr_strtok(NULL, ":", &tok_cntx);
+}
 }
 else if (strncasecmp(a2, "int:", 4) == 0) {
 newmap->type  = MAPTYPE_INT;
@@ -5205,8 +5216,8 @@
  "an input string and a to be applied regexp-pattern"),
 AP_INIT_RAW_ARGS("RewriteRule", cmd_rewriterule, NULL, OR_FILEINFO,
  "an URL-applied regexp-pattern and a substitution URL"),
-AP_INIT_TAKE2(   "RewriteMap",  cmd_rewritemap,  NULL, RSRC_CONF,
- "a mapname and a filename"),
+AP_INIT_TAKE23(   "RewriteMap",  cmd_rewritemap,  NULL, RSRC_CONF,
+ "a mapname and a filename and options"),
 { NULL }
 };
 

Re: Run external RewriteMap program as non-root

[Yann Ylavic]

On Thu, Mar 5, 2015 at 10:48 AM, André Malo  wrote:
>
> 5) Let it drop the privileges by itself.
>
> I actually tend to 5 :-)

+1

Re: Run external RewriteMap program as non-root

[André Malo]

* Jan Kaluža wrote:

> Hi,
>
> currently, the External Rewriting Program (RewriteMap "prg:") is run as
> root. I would like to change it but I see three ways how to do it:
>
> 1. Execute it right after drop_privileges hook. This looks like best
> way, but I haven't found any hook which could be used for that (except
> drop_privileges with APR_HOOK_REALLY_LAST, which does not seem as proper
> place to me).
>
> 2. Execute it in child_init. This is done after drop_privileges, so the
> user/group is good. The "problem" here is that it would execute one
> rewrite program per child. Right now I'm not sure if it's really
> problem. It could be useful to have more instances of rewriting program
> to make its bottleneck lower.
>
> 3. Execute it where it is now (post_config), but set user/group using
> apr_procattr_t. So far I think this would duplicate the code of
> mod_unixd and would probably have to also handle the windows equivalent
> of that module (if there's any).

May be

4) Invoke suexec somehow

and

5) Let it drop the privileges by itself.

I actually tend to 5 :-)

nd
-- 
Winnetous Erbe: 

Re: Run external RewriteMap program as non-root

[Jan Kaluža]

On 03/05/2015 09:54 AM, Jan Kaluža wrote:

On 03/05/2015 09:03 AM, Ruediger Pluem wrote:



On 03/05/2015 07:55 AM, Jan Kaluža wrote:

Hi,

currently, the External Rewriting Program (RewriteMap "prg:") is run
as root. I would like to change it but I see three
ways how to do it:

1. Execute it right after drop_privileges hook. This looks like best
way, but I haven't found any hook which could be
used for that (except drop_privileges with APR_HOOK_REALLY_LAST,
which does not seem as proper place to me).

2. Execute it in child_init. This is done after drop_privileges, so
the user/group is good. The "problem" here is that
it would execute one rewrite program per child. Right now I'm not
sure if it's really problem. It could be useful to
have more instances of rewriting program to make its bottleneck lower.

3. Execute it where it is now (post_config), but set user/group using
apr_procattr_t. So far I think this would
duplicate the code of mod_unixd and would probably have to also
handle the windows equivalent of that module (if there's
any).

What way do you think is the best, or would you do it differently?

I'm attaching patch for number 2.


I would tend to 2. as well, but as far as I remember using the
rewritemap program is synchronized across all processes.
This raises two questions:

1. Does rewriting still work with the current patch?


It does work for me. I've done some tests with curl and ab with
prefork/event/worker MPMs.


2. If it does can stuff be optimized to move from a server wide lock
to a process wide lock (or even no lock for
prefork) to remove the contention here?


This could be possible, I will look at it.


Attached patch does it and works for me. RewriteMap with external 
program is also 24% faster with prefork with this patch.


Jan Kaluza


OTOH looking at the topic of backwards compatibility existing rewrite
programs
might rely on not working in parallel. Some may even have an issue if
more then one copy of them is running in parallel,
albeit not processing stuff in parallel which of course would cause an
issue with the proposed patch. Furthermore
existing setups might expect to be run as root. But this stuff only
needs to be considered when we think about
backporting and is moot for trunk.


Right, I'm currently thinking only about trunk. For the 2.4.x, we would
have to do it differently with backward compatibility in mind. I think
something like option 1 with configuration directive to enable new
behaviour would be more acceptable for 2.4.x. We would have single
rewritemap program in this case running as an apache user only if admin
wants it.


Regards

Rüdiger



Regards,
Jan Kaluza



Index: modules/mappers/mod_rewrite.c
===
--- modules/mappers/mod_rewrite.c	(revision 1663642)
+++ modules/mappers/mod_rewrite.c	(working copy)
@@ -52,7 +52,7 @@
 #include "apr_user.h"
 #include "apr_lib.h"
 #include "apr_signal.h"
-#include "apr_global_mutex.h"
+#include "apr_thread_mutex.h"
 #include "apr_dbm.h"
 #include "apr_dbd.h"
 #include "mod_dbd.h"
@@ -100,6 +100,7 @@
 
 #include "mod_rewrite.h"
 #include "ap_expr.h"
+#include "ap_mpm.h"
 
 #if APR_CHARSET_EBCDIC
 #include "util_charset.h"
@@ -416,8 +417,7 @@
 static int proxy_available;
 
 /* Locks/Mutexes */
-static apr_global_mutex_t *rewrite_mapr_lock_acquire = NULL;
-static const char *rewritemap_mutex_type = "rewrite-map";
+static apr_thread_mutex_t *rewrite_mapr_lock_acquire = NULL;
 
 /* Optional functions imported from mod_ssl when loaded: */
 static APR_OPTIONAL_FN_TYPE(ssl_var_lookup) *rewrite_ssl_lookup = NULL;
@@ -1437,10 +1437,10 @@
 
 /* take the lock */
 if (rewrite_mapr_lock_acquire) {
-rv = apr_global_mutex_lock(rewrite_mapr_lock_acquire);
+rv = apr_thread_mutex_lock(rewrite_mapr_lock_acquire);
 if (rv != APR_SUCCESS) {
 ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(00659)
-  "apr_global_mutex_lock(rewrite_mapr_lock_acquire) "
+  "apr_thread_mutex_lock(rewrite_mapr_lock_acquire) "
   "failed");
 return NULL; /* Maybe this should be fatal? */
 }
@@ -1551,10 +1551,10 @@
 
 /* give the lock back */
 if (rewrite_mapr_lock_acquire) {
-rv = apr_global_mutex_unlock(rewrite_mapr_lock_acquire);
+rv = apr_thread_mutex_unlock(rewrite_mapr_lock_acquire);
 if (rv != APR_SUCCESS) {
 ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(00660)
-  "apr_global_mutex_unlock(rewrite_mapr_lock_acquire) "
+  "apr_thread_mutex_unlock(rewrite_mapr_lock_acquire) "
   "failed");
 return NULL; /* Maybe this should be fatal? */
 }
@@ -2639,13 +2639,21 @@
 static apr_status_t rewritelock_create(server_rec *s, apr_pool_t *p)
 {
 apr_status_t rc;
+int threaded;
 
+/* RewriteMap program is spawned per chil

Re: Event and RINGs

[Yann Ylavic]

On Wed, Mar 4, 2015 at 7:59 PM, Jim Jagielski  wrote:
> I am wondering if we are continuing to use RINGs in places
> where we should really migrate to using skiplists. afaict, we
> used RINGs initially because it was the only valid and available
> data structure we could use, but it was not idea.

Using a single skiplist for all timers in event would indeed make the
code cleaner and ease insertion and maintenance (to cleanup *all*
expired timers in a single loop), but would probably complexify
removal of individual timers (here some keepalive timer beacuse the
connection becomes active, there a write completion timer because the
filter chain is flushed, ...).
Currently with RINGs all these operations are O(1), skiplist would
make insertions and removals O(log n) (and still maintenance O(1)).

Btw, short term (which is not what you are talking about I guess),
skiplists in APR-1.5 are missing apr_skiplist_add(), so timers may
collide and apr_skiplist_insert() won't add them (which does not help
moving to them in several cases).
(That reminds me that things need to be backported to APR-1.6 before
it gets released ...)

> Even
> now, we aren't really using it as a FIFO queue and whenever
> we access it, we appear to be trying to do so in a sorted
> way (as related to time stamps)...

This is indeed not ideal, and it won't work as soon as we need to
handle dynamic timeouts (unknown at init time, so that we can't create
a RING per timeout)...

> I may end up doing
> some tests to see what the performance diffs would be if
> we instead just used skiplists.

+1

> Besides, I think all that RING macro junk is ugly :) :)

I personnaly love RINGs (despite pointer-type puning and terrific
code!), that's an excellent intrusive container :)
Not applicable everywhere though...

Regards,
Yann.

Re: svn commit: r1664205 - in /httpd/httpd/trunk: CHANGES server/protocol.c

[Yann Ylavic]

On Thu, Mar 5, 2015 at 10:09 AM, Ruediger Pluem  wrote:
>
>
> On 03/05/2015 10:01 AM, Yann Ylavic wrote:
>> On Thu, Mar 5, 2015 at 9:38 AM, Ruediger Pluem  wrote:
>>>
>>> Don't we need to have the following in addition to avoid a crash in another 
>>> path?
>>>
>>>
>>> Index: protocol.c
>>> ===
>>> --- protocol.c  (revision 1664261)
>>> +++ protocol.c  (working copy)
>>> @@ -674,6 +674,8 @@
>>>  ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02418)
>>>"Invalid protocol '%s'", r->protocol);
>>>  if (enforce_strict) {
>>> +r->protocol  = apr_pstrdup(r->pool, "HTTP/1.0");
>>> +r->proto_num = HTTP_VERSION(1,0);
>>>  r->status = HTTP_BAD_REQUEST;
>>>  return 0;
>>>  }
>>>
>>
>> r->protocol is initialized above in any case (with an invalid value
>> but it is), so it won't crash.
>> But r->proto_num is still 0 (which, if I read output filter chain
>> correctly, will lead to a HTTP/1.1 response), though.
>> So we probably should keep the original value in r->protocol (for
>> logging), but force proto_num = HTTP_VERSION(1,0).
>
> I saw that r->protocol is initialized in this case, but I wanted to keep it 
> consistent with r->proto_num in this case.
> Hence the reset to "HTTP/1.0". If we are interested in logging we might 
> should increase the severity of the error
> message above from DEBUG to something more visible e.g. INFO at least in the 
> enforce_strict case.

I meant *custom* logging of the original request fields, but
r->the_request will do the job in this case.
So it's indeed probably better to keep things consistent internally.

Re: svn commit: r1664205 - in /httpd/httpd/trunk: CHANGES server/protocol.c

[Ruediger Pluem]

On 03/05/2015 10:01 AM, Yann Ylavic wrote:
> On Thu, Mar 5, 2015 at 9:38 AM, Ruediger Pluem  wrote:
>>
>> Don't we need to have the following in addition to avoid a crash in another 
>> path?
>>
>>
>> Index: protocol.c
>> ===
>> --- protocol.c  (revision 1664261)
>> +++ protocol.c  (working copy)
>> @@ -674,6 +674,8 @@
>>  ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02418)
>>"Invalid protocol '%s'", r->protocol);
>>  if (enforce_strict) {
>> +r->protocol  = apr_pstrdup(r->pool, "HTTP/1.0");
>> +r->proto_num = HTTP_VERSION(1,0);
>>  r->status = HTTP_BAD_REQUEST;
>>  return 0;
>>  }
>>
> 
> r->protocol is initialized above in any case (with an invalid value
> but it is), so it won't crash.
> But r->proto_num is still 0 (which, if I read output filter chain
> correctly, will lead to a HTTP/1.1 response), though.
> So we probably should keep the original value in r->protocol (for
> logging), but force proto_num = HTTP_VERSION(1,0).

I saw that r->protocol is initialized in this case, but I wanted to keep it 
consistent with r->proto_num in this case.
Hence the reset to "HTTP/1.0". If we are interested in logging we might should 
increase the severity of the error
message above from DEBUG to something more visible e.g. INFO at least in the 
enforce_strict case.

Regards

Rüdiger

Re: svn commit: r1664205 - in /httpd/httpd/trunk: CHANGES server/protocol.c

[Yann Ylavic]

On Thu, Mar 5, 2015 at 9:38 AM, Ruediger Pluem  wrote:
>
> Don't we need to have the following in addition to avoid a crash in another 
> path?
>
>
> Index: protocol.c
> ===
> --- protocol.c  (revision 1664261)
> +++ protocol.c  (working copy)
> @@ -674,6 +674,8 @@
>  ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02418)
>"Invalid protocol '%s'", r->protocol);
>  if (enforce_strict) {
> +r->protocol  = apr_pstrdup(r->pool, "HTTP/1.0");
> +r->proto_num = HTTP_VERSION(1,0);
>  r->status = HTTP_BAD_REQUEST;
>  return 0;
>  }
>

r->protocol is initialized above in any case (with an invalid value
but it is), so it won't crash.
But r->proto_num is still 0 (which, if I read output filter chain
correctly, will lead to a HTTP/1.1 response), though.
So we probably should keep the original value in r->protocol (for
logging), but force proto_num = HTTP_VERSION(1,0).

Regards,
Yann.

Re: Run external RewriteMap program as non-root

[Jan Kaluža]

On 03/05/2015 09:03 AM, Ruediger Pluem wrote:



On 03/05/2015 07:55 AM, Jan Kaluža wrote:

Hi,

currently, the External Rewriting Program (RewriteMap "prg:") is run as root. I 
would like to change it but I see three
ways how to do it:

1. Execute it right after drop_privileges hook. This looks like best way, but I 
haven't found any hook which could be
used for that (except drop_privileges with APR_HOOK_REALLY_LAST, which does not 
seem as proper place to me).

2. Execute it in child_init. This is done after drop_privileges, so the user/group is 
good. The "problem" here is that
it would execute one rewrite program per child. Right now I'm not sure if it's 
really problem. It could be useful to
have more instances of rewriting program to make its bottleneck lower.

3. Execute it where it is now (post_config), but set user/group using 
apr_procattr_t. So far I think this would
duplicate the code of mod_unixd and would probably have to also handle the 
windows equivalent of that module (if there's
any).

What way do you think is the best, or would you do it differently?

I'm attaching patch for number 2.


I would tend to 2. as well, but as far as I remember using the rewritemap 
program is synchronized across all processes.
This raises two questions:

1. Does rewriting still work with the current patch?


It does work for me. I've done some tests with curl and ab with 
prefork/event/worker MPMs.



2. If it does can stuff be optimized to move from a server wide lock to a 
process wide lock (or even no lock for
prefork) to remove the contention here?


This could be possible, I will look at it.


OTOH looking at the topic of backwards compatibility existing rewrite programs
might rely on not working in parallel. Some may even have an issue if more then 
one copy of them is running in parallel,
albeit not processing stuff in parallel which of course would cause an issue 
with the proposed patch. Furthermore
existing setups might expect to be run as root. But this stuff only needs to be 
considered when we think about
backporting and is moot for trunk.


Right, I'm currently thinking only about trunk. For the 2.4.x, we would 
have to do it differently with backward compatibility in mind. I think 
something like option 1 with configuration directive to enable new 
behaviour would be more acceptable for 2.4.x. We would have single 
rewritemap program in this case running as an apache user only if admin 
wants it.



Regards

Rüdiger



Regards,
Jan Kaluza

Re: svn commit: r1664205 - in /httpd/httpd/trunk: CHANGES server/protocol.c

[Ruediger Pluem]

On 03/05/2015 03:33 AM, cove...@apache.org wrote:
> Author: covener
> Date: Thu Mar  5 02:33:16 2015
> New Revision: 1664205
> 
> URL: http://svn.apache.org/r1664205
> Log:
>   *) SECURITY: CVE-2015-0253 (cve.mitre.org)
>  core: Fix a crash introduced in with ErrorDocument 400 pointing
>  to a local URL-path with the INCLUDES filter active, introduced
>  in 2.4.11. PR 57531. [Yann Ylavic]
> 
> 
> Submitted By: ylavic
> Committed By: covener
> 
> 
> 
> 
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/server/protocol.c
> 

> Modified: httpd/httpd/trunk/server/protocol.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1664205&r1=1664204&r2=1664205&view=diff
> ==
> --- httpd/httpd/trunk/server/protocol.c (original)
> +++ httpd/httpd/trunk/server/protocol.c Thu Mar  5 02:33:16 2015
> @@ -606,8 +606,6 @@ static int read_request_line(request_rec
>   */
>  if (APR_STATUS_IS_ENOSPC(rv)) {
>  r->status= HTTP_REQUEST_URI_TOO_LARGE;
> -r->proto_num = HTTP_VERSION(1,0);
> -r->protocol  = apr_pstrdup(r->pool, "HTTP/1.0");
>  }
>  else if (APR_STATUS_IS_TIMEUP(rv)) {
>  r->status = HTTP_REQUEST_TIME_OUT;
> @@ -615,6 +613,8 @@ static int read_request_line(request_rec
>  else if (APR_STATUS_IS_EINVAL(rv)) {
>  r->status = HTTP_BAD_REQUEST;
>  }
> +r->proto_num = HTTP_VERSION(1,0);
> +r->protocol  = apr_pstrdup(r->pool, "HTTP/1.0");
>  return 0;
>  }
>  } while ((len <= 0) && (++num_blank_lines < max_blank_lines));
> 
> 
> 

Don't we need to have the following in addition to avoid a crash in another 
path?


Index: protocol.c
===
--- protocol.c  (revision 1664261)
+++ protocol.c  (working copy)
@@ -674,6 +674,8 @@
 ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02418)
   "Invalid protocol '%s'", r->protocol);
 if (enforce_strict) {
+r->protocol  = apr_pstrdup(r->pool, "HTTP/1.0");
+r->proto_num = HTTP_VERSION(1,0);
 r->status = HTTP_BAD_REQUEST;
 return 0;
 }



Regards

Rüdiger

Re: svn commit: r1664118 - in /httpd/httpd/branches/2.4.x: ./ CHANGES STATUS modules/lua/lua_request.c

[Ruediger Pluem]

On 03/04/2015 08:18 PM, cove...@apache.org wrote:
> Author: covener
> Date: Wed Mar  4 19:18:27 2015
> New Revision: 1664118
> 
> URL: http://svn.apache.org/r1664118
> Log:
> Merge r1657261 from trunk:
> 
>   *) SECURITY: CVE-2015-0228 (cve.mitre.org)
>  mod_lua: A maliciously crafted websockets PING after a script
>  calls r:wsupgrade() can cause a child process crash.
>  [Edward Lu ]
> 
> Discovered by Guido Vranken 
> 
> Submitted by: Edward Lu
> Committed by: covener
> 
> 
> 
> Modified:
> httpd/httpd/branches/2.4.x/   (props changed)
> httpd/httpd/branches/2.4.x/CHANGES
> httpd/httpd/branches/2.4.x/STATUS
> httpd/httpd/branches/2.4.x/modules/lua/lua_request.c
> 

> Modified: httpd/httpd/branches/2.4.x/modules/lua/lua_request.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/lua/lua_request.c?rev=1664118&r1=1664117&r2=1664118&view=diff
> ==
> --- httpd/httpd/branches/2.4.x/modules/lua/lua_request.c (original)
> +++ httpd/httpd/branches/2.4.x/modules/lua/lua_request.c Wed Mar  4 19:18:27 
> 2015
> @@ -2229,6 +2229,7 @@ static int lua_websocket_read(lua_State
>  {
>  apr_socket_t *sock;
>  apr_status_t rv;
> +int do_read = 1;
>  int n = 0;
>  apr_size_t len = 1;
>  apr_size_t plen = 0;
> @@ -2246,6 +2247,8 @@ static int lua_websocket_read(lua_State
>  mask_bytes = apr_pcalloc(r->pool, 4);
>  sock = ap_get_conn_socket(r->connection);
>  
> +while (do_read) { 
> +do_read = 0;
>  /* Get opcode and FIN bit */
>  if (plaintext) {
>  rv = apr_socket_recv(sock, &byte, &len);
> @@ -2379,10 +2382,11 @@ static int lua_websocket_read(lua_State
>  frame[0] = 0x8A;
>  frame[1] = 0;
>  apr_socket_send(sock, frame, &plen); /* Pong! */
> -lua_websocket_read(L); /* read the next frame instead */
> +do_read = 1;
>  }
>  }
>  }
> +}
>  return 0;
>  }
>  

In order to stay in sync with trunk shouldn't we apply the needed whitespace 
changes as well?
The applied patch was the one without whitespace changes to keep it more 
readable.

Regards

Rüdiger

Re: Run external RewriteMap program as non-root

[Ruediger Pluem]

On 03/05/2015 07:55 AM, Jan Kaluža wrote:
> Hi,
> 
> currently, the External Rewriting Program (RewriteMap "prg:") is run as root. 
> I would like to change it but I see three
> ways how to do it:
> 
> 1. Execute it right after drop_privileges hook. This looks like best way, but 
> I haven't found any hook which could be
> used for that (except drop_privileges with APR_HOOK_REALLY_LAST, which does 
> not seem as proper place to me).
> 
> 2. Execute it in child_init. This is done after drop_privileges, so the 
> user/group is good. The "problem" here is that
> it would execute one rewrite program per child. Right now I'm not sure if 
> it's really problem. It could be useful to
> have more instances of rewriting program to make its bottleneck lower.
> 
> 3. Execute it where it is now (post_config), but set user/group using 
> apr_procattr_t. So far I think this would
> duplicate the code of mod_unixd and would probably have to also handle the 
> windows equivalent of that module (if there's
> any).
> 
> What way do you think is the best, or would you do it differently?
> 
> I'm attaching patch for number 2.

I would tend to 2. as well, but as far as I remember using the rewritemap 
program is synchronized across all processes.
This raises two questions:

1. Does rewriting still work with the current patch?
2. If it does can stuff be optimized to move from a server wide lock to a 
process wide lock (or even no lock for
prefork) to remove the contention here? OTOH looking at the topic of backwards 
compatibility existing rewrite programs
might rely on not working in parallel. Some may even have an issue if more then 
one copy of them is running in parallel,
albeit not processing stuff in parallel which of course would cause an issue 
with the proposed patch. Furthermore
existing setups might expect to be run as root. But this stuff only needs to be 
considered when we think about
backporting and is moot for trunk.

Regards

Rüdiger