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 child, so for non-threaded MPMs,
+ * we do not need the locking.*/
+ int rv = ap_mpm_query(AP_MPMQ_IS_THREADED, &threaded);
+ if (rv == APR_SUCCESS && threaded == 0) {
+ return APR_SUCCESS;
+ }
+
/* create the lockfile */
/* XXX See if there are any rewrite map programs before creating
* the mutex.
*/
- rc = ap_global_mutex_create(&rewrite_mapr_lock_acquire, NULL,
- rewritemap_mutex_type, NULL, s, p, 0);
+ rc = apr_thread_mutex_create(&rewrite_mapr_lock_acquire,
+ APR_THREAD_MUTEX_DEFAULT, p);
if (rc != APR_SUCCESS) {
return rc;
}
@@ -2657,7 +2665,7 @@
{
/* destroy the rewritelock */
if (rewrite_mapr_lock_acquire) {
- apr_global_mutex_destroy(rewrite_mapr_lock_acquire);
+ apr_thread_mutex_destroy(rewrite_mapr_lock_acquire);
rewrite_mapr_lock_acquire = NULL;
}
return APR_SUCCESS;
@@ -4416,8 +4424,6 @@
{
APR_OPTIONAL_FN_TYPE(ap_register_rewrite_mapfunc) *map_pfn_register;
- ap_mutex_register(pconf, rewritemap_mutex_type, NULL, APR_LOCK_DEFAULT, 0);
-
/* register int: rewritemap handlers */
map_pfn_register = APR_RETRIEVE_OPTIONAL_FN(ap_register_rewrite_mapfunc);
if (map_pfn_register) {
@@ -4441,25 +4447,6 @@
/* check if proxy module is available */
proxy_available = (ap_find_linked_module("mod_proxy.c") != NULL);
- rv = rewritelock_create(s, p);
- if (rv != APR_SUCCESS) {
- return HTTP_INTERNAL_SERVER_ERROR;
- }
-
- apr_pool_cleanup_register(p, (void *)s, rewritelock_remove,
- apr_pool_cleanup_null);
-
- /* if we are not doing the initial config, step through the servers and
- * open the RewriteMap prg:xxx programs,
- */
- if (ap_state_query(AP_SQ_MAIN_STATE) == AP_SQ_MS_CREATE_CONFIG) {
- for (; s; s = s->next) {
- if (run_rewritemap_programs(s, p) != APR_SUCCESS) {
- return HTTP_INTERNAL_SERVER_ERROR;
- }
- }
- }
-
rewrite_ssl_lookup = APR_RETRIEVE_OPTIONAL_FN(ssl_var_lookup);
rewrite_is_https = APR_RETRIEVE_OPTIONAL_FN(ssl_is_https);
@@ -4470,21 +4457,26 @@
{
apr_status_t rv = 0; /* get a rid of gcc warning (REWRITELOG_DISABLED) */
- if (rewrite_mapr_lock_acquire) {
- rv = apr_global_mutex_child_init(&rewrite_mapr_lock_acquire,
- apr_global_mutex_lockfile(rewrite_mapr_lock_acquire), p);
- if (rv != APR_SUCCESS) {
- ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s, APLOGNO(00666)
- "mod_rewrite: could not init rewrite_mapr_lock_acquire"
- " in child");
- }
+ rv = rewritelock_create(s, p);
+ if (rv != APR_SUCCESS) {
+ ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s, APLOGNO(00666)
+ "mod_rewrite: could not init rewrite_mapr_lock_acquire"
+ " in child");
}
+ apr_pool_cleanup_register(p, (void *)s, rewritelock_remove,
+ apr_pool_cleanup_null);
+
/* create the lookup cache */
if (!init_cache(p)) {
ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s, APLOGNO(00667)
"mod_rewrite: could not init map cache in child");
}
+
+ /* step through the servers and open the RewriteMap prg:xxx programs */
+ for (; s; s = s->next) {
+ run_rewritemap_programs(s, p);
+ }
}
